Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2777353002:
Upload the test results html file to google bucket. (Closed)
|
Created:
3 years, 9 months ago by BigBossZhiling Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpload the test results html file to google bucket.
Previously we print the test results html to stdout. In this cl, we
upload the test results html to google bucket and print the link to the
file instead.
Sample link would be
'https://storage.googleapis.com/chromium-result-details/html/test_name/builder_name_builder_number_2017_03_27_T15_22_47.html'.
BUG=
Review-Url: https://codereview.chromium.org/2777353002
Cr-Commit-Position: refs/heads/master@{#460827}
Committed: https://chromium.googlesource.com/chromium/src/+/0046d99e3911d416d0b5cb6f6f0191317bf41711
Patch Set 1 #
Total comments: 5
Patch Set 2 : wait file to be finished writing before uploading + add bucket customization when searching for css #Patch Set 3 : changed arguments into mandatory #Patch Set 4 : add option to download the file #Patch Set 5 : fixes #
Total comments: 8
Patch Set 6 : addressing case's comments #
Total comments: 4
Patch Set 7 : john's comments #
Total comments: 1
Patch Set 8 : fixes url #
Total comments: 1
Patch Set 9 : fix url #
Messages
Total messages: 32 (8 generated)
Description was changed from ========== Upload the test results html file to google bucket. Previously we print the test results html to stdout. In this cl, we upload the test results html to google bucket and print the link to the file instead. Sample link would be 'https://storage.googleapis.com/chromium-result-details/html/test_name/builder_name_builder_number_2017_03_27_T15_22_47.html'. BUG= ========== to ========== Upload the test results html file to google bucket. Previously we print the test results html to stdout. In this cl, we upload the test results html to google bucket and print the link to the file instead. Sample link would be 'https://storage.googleapis.com/chromium-result-details/html/test_name/builder_name_builder_number_2017_03_27_T15_22_47.html'. BUG= ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2777353002/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/template/main.html (right): https://codereview.chromium.org/2777353002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/template/main.html:5: <link rel="stylesheet" href="https://storage.googleapis.com/chromium-result-details/css/default.css" type="text/css"> I may need to change the bucket here, chromium-result-details, to be {bucket} and let test_results_presentation.py to pass the right bucket in.
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
Uploading results detail on recipe side might be simpler
On 2017/03/27 22:43:54, mikecase wrote: > Uploading results detail on recipe side might be simpler by doing this, we'd wind up with a separate step, which I'd like to avoid.
https://codereview.chromium.org/2777353002/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2777353002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:284: args.test_name if args.test_name else 'test_name', I am not sure whether we want to make test_name, (chrome_public_test_apk) etc. to be mandatory here. If not, do you think that test_name, builder_name and builder_number is good default value?
https://codereview.chromium.org/2777353002/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/template/main.html (right): https://codereview.chromium.org/2777353002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/template/main.html:5: <link rel="stylesheet" href="https://storage.googleapis.com/chromium-result-details/css/default.css" type="text/css"> On 2017/03/27 22:37:36, BigBossZhiling wrote: > I may need to change the bucket here, chromium-result-details, to be {bucket} > and let test_results_presentation.py to pass the right bucket in. Yeah, that's a good idea. https://codereview.chromium.org/2777353002/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2777353002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:284: args.test_name if args.test_name else 'test_name', On 2017/03/27 22:54:54, BigBossZhiling wrote: > I am not sure whether we want to make test_name, (chrome_public_test_apk) etc. > to be mandatory here. If not, do you think that test_name, builder_name and > builder_number is good default value? I think they should all be mandatory. https://codereview.chromium.org/2777353002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:311: args) This should pass args.test_name, args.builder_name, args.bucket, and args.build_number separately.
done included logic to download the file
Just some comments. Looks pretty much good to me. :D https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:279: nit: add another empty line here https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:285: dest = 'html/%s/%s_%s_%s.html' % ( question, why not just %s_%s_%s_%s.html Why not have test name be part of file? https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:293: gsutil_path, content_type, temp_file.name, bucket, dest), shell=True) nit: this indentation looks off. I would... subprocess.check_call( 'python %s -h "Content-Type:%s" cp %s gs://%s/%s' % (gsutil_path, content_type, temp_file.name, bucket, dest), shell=True) https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:310: '--content-type', nit: 4 spaces indent
https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:279: On 2017/03/29 21:40:17, mikecase wrote: > nit: add another empty line here Done. https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:285: dest = 'html/%s/%s_%s_%s.html' % ( On 2017/03/29 21:40:16, mikecase wrote: > question, why not just %s_%s_%s_%s.html > > Why not have test name be part of file? Done. https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:293: gsutil_path, content_type, temp_file.name, bucket, dest), shell=True) On 2017/03/29 21:40:17, mikecase wrote: > nit: this indentation looks off. > > I would... > > subprocess.check_call( > 'python %s -h "Content-Type:%s" cp %s gs://%s/%s' % > (gsutil_path, content_type, temp_file.name, bucket, dest), > shell=True) Done. https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:310: '--content-type', On 2017/03/29 21:40:16, mikecase wrote: > nit: 4 spaces indent Done.
https://codereview.chromium.org/2777353002/diff/100001/build/android/pylib/re... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2777353002/diff/100001/build/android/pylib/re... build/android/pylib/results/presentation/test_results_presentation.py:292: 'python %s -h "Content-Type:%s" cp %s gs://%s/%s' % ( nit: pass a list and use sys.executable instead of 'python' https://codereview.chromium.org/2777353002/diff/100001/build/android/pylib/re... build/android/pylib/results/presentation/test_results_presentation.py:310: parser.add_argument( What is this for downloadable pages? I imagine we should restrict it to only the values we want.
On 2017/03/29 21:40:17, mikecase wrote: > Just some comments. Looks pretty much good to me. :D "lpmgtm" :P > > https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... > File build/android/pylib/results/presentation/test_results_presentation.py > (right): > > https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... > build/android/pylib/results/presentation/test_results_presentation.py:279: > nit: add another empty line here > > https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... > build/android/pylib/results/presentation/test_results_presentation.py:285: dest > = 'html/%s/%s_%s_%s.html' % ( > question, why not just %s_%s_%s_%s.html > > Why not have test name be part of file? > > https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... > build/android/pylib/results/presentation/test_results_presentation.py:293: > gsutil_path, content_type, temp_file.name, bucket, dest), shell=True) > nit: this indentation looks off. > > I would... > > subprocess.check_call( > 'python %s -h "Content-Type:%s" cp %s gs://%s/%s' % > (gsutil_path, content_type, temp_file.name, bucket, dest), > shell=True) > > https://codereview.chromium.org/2777353002/diff/80001/build/android/pylib/res... > build/android/pylib/results/presentation/test_results_presentation.py:310: > '--content-type', > nit: 4 spaces indent
https://codereview.chromium.org/2777353002/diff/100001/build/android/pylib/re... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2777353002/diff/100001/build/android/pylib/re... build/android/pylib/results/presentation/test_results_presentation.py:292: 'python %s -h "Content-Type:%s" cp %s gs://%s/%s' % ( On 2017/03/29 23:26:06, jbudorick wrote: > nit: pass a list and use sys.executable instead of 'python' Done. https://codereview.chromium.org/2777353002/diff/100001/build/android/pylib/re... build/android/pylib/results/presentation/test_results_presentation.py:310: parser.add_argument( On 2017/03/29 23:26:06, jbudorick wrote: > What is this for downloadable pages? > > I imagine we should restrict it to only the values we want. Done.
one more q https://codereview.chromium.org/2777353002/diff/120001/build/android/pylib/re... File build/android/pylib/results/presentation/template/main.html (right): https://codereview.chromium.org/2777353002/diff/120001/build/android/pylib/re... build/android/pylib/results/presentation/template/main.html:5: <link rel="stylesheet" href="https://storage.googleapis.com/{{bucket}}/css/default.css" type="text/css"> Should this always be storage.googleapis.com? Will that work if we upload to another url?
fixed the url. good catch
lgtm w/ reservations? https://codereview.chromium.org/2777353002/diff/140001/build/android/pylib/re... File build/android/pylib/results/presentation/template/main.html (right): https://codereview.chromium.org/2777353002/diff/140001/build/android/pylib/re... build/android/pylib/results/presentation/template/main.html:5: <link rel="stylesheet" href="https://storage.cloud.google.com/{{bucket}}/css/default.css" type="text/css"> I think this should be {{server_url}}/{{bucket}} ... but I suppose I'm ok with us trying it as-is.
lgtm w/o reservations :)
The CQ bit was checked by hzl@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by hzl@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490896079223330,
"parent_rev": "c383a4d067322844776e4029c86803748f0526d0", "commit_rev":
"0046d99e3911d416d0b5cb6f6f0191317bf41711"}
Message was sent while issue was closed.
Description was changed from ========== Upload the test results html file to google bucket. Previously we print the test results html to stdout. In this cl, we upload the test results html to google bucket and print the link to the file instead. Sample link would be 'https://storage.googleapis.com/chromium-result-details/html/test_name/builder_name_builder_number_2017_03_27_T15_22_47.html'. BUG= ========== to ========== Upload the test results html file to google bucket. Previously we print the test results html to stdout. In this cl, we upload the test results html to google bucket and print the link to the file instead. Sample link would be 'https://storage.googleapis.com/chromium-result-details/html/test_name/builder_name_builder_number_2017_03_27_T15_22_47.html'. BUG= Review-Url: https://codereview.chromium.org/2777353002 Cr-Commit-Position: refs/heads/master@{#460827} Committed: https://chromium.googlesource.com/chromium/src/+/0046d99e3911d416d0b5cb6f6f01... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0046d99e3911d416d0b5cb6f6f01...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2786003003/ by hzl@google.com. The reason for reverting is: Causing error in build bot. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
