Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2786773002:
(Reland) Add failure screenshots and images to results detail. (Closed)
|
Created:
3 years, 8 months ago by mikecase (-- gone --) Modified:
3 years, 7 months ago CC:
agrieve+watch_chromium.org, Bernhard Bauer, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description(Reland) Add post-test screenshots to results detail.
Review-Url: https://codereview.chromium.org/2786773002
Cr-Commit-Position: refs/heads/master@{#469237}
Committed: https://chromium.googlesource.com/chromium/src/+/2bb1cf11d22d50c5750d8d3b1356f0caa43d359e
Patch Set 1 #Patch Set 2 : (Reland) Add failure screenshots and images to results detail. #Patch Set 3 : (Reland) Add failure screenshots and images to results detail. #Patch Set 4 : (Reland) Add failure screenshots and images to results detail. #
Total comments: 12
Patch Set 5 : Updated! #Patch Set 6 : (Reland) Add failure screenshots and images to results detail. #
Total comments: 21
Patch Set 7 : Java-side diffing #Patch Set 8 : Removed render tests code. Moving that to a knew CL #Patch Set 9 : (Reland) Add failure screenshots and images to results detail. #
Total comments: 11
Patch Set 10 : (Reland) Add failure screenshots and images to results detail. #Patch Set 11 : (Reland) Add failure screenshots and images to results detail. #
Total comments: 2
Patch Set 12 : yolands nit #
Messages
Total messages: 28 (7 generated)
Initial commit: https://codereview.chromium.org/2701473003 Reason for revert is that "find_depot_tools" doesnt work on some of the swarming bots. Only purpose of find_depot_tools was to get path to gsutil.py. Now, just using path to third_party/catapult/third_party/gsutil instead.
mikecase@chromium.org changed reviewers: + hzl@chromium.org, jbudorick@chromium.org, yolandyan@chromium.org
Looking to reland this. Stealing Zhiling's idea to just hardcode path to the gsutil.py script in catapult. Removed depot_tools dependency and did some minor cleanup. Patch 1 is what was initially committed.
https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:687: self._should_save_images = bool(args.json_results_file) nit: alpha https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:35: can_compute_diffs = False I've usually seen (and written) this kind of thing by setting the package name to None and then checking against that at usage time, but this is nice. https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:393: bucket='chromium-result-details/images') We shouldn't be hardcoding the bucket. If we use should_save_images downstream, we want those to go to a different bucket. https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:466: bucket='chromium-render-tests') Again, I'm concerned about the hard-coded bucket names throughout this. https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/res... File build/android/pylib/results/presentation/template/main.html (right): https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/template/main.html:5: <link rel="stylesheet" href="https://storage.googleapis.com/{{bucket}}/css/default.css" type="text/css"> eh? https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/uti... File build/android/pylib/utils/google_storage_helper.py (right): https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/uti... build/android/pylib/utils/google_storage_helper.py:27: _URL_TEMPLATE = 'https://storage.googleapis.com/%s/' Zhiling, which one of these URLs were we having issues with? This one or storage.cloud.google.com?
hzl@google.com changed reviewers: + hzl@google.com
https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/uti... File build/android/pylib/utils/google_storage_helper.py (right): https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/uti... build/android/pylib/utils/google_storage_helper.py:27: _URL_TEMPLATE = 'https://storage.googleapis.com/%s/' On 2017/04/11 00:36:44, jbudorick wrote: > Zhiling, which one of these URLs were we having issues with? This one or > storage.cloud.google.com? storage.googleapis.com is the one that cannot have ?xxx=yyy at the end and it is not available for private/downstream files.
https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:687: self._should_save_images = bool(args.json_results_file) On 2017/04/11 at 00:36:43, jbudorick wrote: > nit: alpha done https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:393: bucket='chromium-result-details/images') On 2017/04/11 at 00:36:44, jbudorick wrote: > We shouldn't be hardcoding the bucket. If we use should_save_images downstream, we want those to go to a different bucket. Agreed. Added test runner arg --gs-results-bucket. Also should_save_images now depends on both specifying a bucket and a results_json_file https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:466: bucket='chromium-render-tests') On 2017/04/11 at 00:36:43, jbudorick wrote: > Again, I'm concerned about the hard-coded bucket names throughout this. Fixed https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/res... File build/android/pylib/results/presentation/template/main.html (right): https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/template/main.html:5: <link rel="stylesheet" href="https://storage.googleapis.com/{{bucket}}/css/default.css" type="text/css"> On 2017/04/11 at 00:36:44, jbudorick wrote: > eh? Changed a little, but there is very little reason not to just always fetch the css from... https://storage.googleapis.com/chromium-result-details/css/default.css instead of templating this. It simplifies things. https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/uti... File build/android/pylib/utils/google_storage_helper.py (right): https://codereview.chromium.org/2786773002/diff/60001/build/android/pylib/uti... build/android/pylib/utils/google_storage_helper.py:27: _URL_TEMPLATE = 'https://storage.googleapis.com/%s/' On 2017/04/11 at 16:21:33, BigBossZhiling wrote: > On 2017/04/11 00:36:44, jbudorick wrote: > > Zhiling, which one of these URLs were we having issues with? This one or > > storage.cloud.google.com? > > storage.googleapis.com is the one that cannot have ?xxx=yyy at the end and it is not available for private/downstream files. Ack. From... https://cloud.google.com/storage/docs/cloud-console It says that storage.cloud.google.com links authenticate using a Google account. storage.googleapis.com does not and only works for public buckets. I added an authenticated_link=bool arg to my upload function. We can probably just always use the authenticated links for the most part.
ping
https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:690: self._should_save_images = ( nit: do we need to save this separately? https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:58: 'ChromePublicTest': 'chrome/test/data/android/render_tests' Can we have the APK store this information in the manifest rather than having it hard-coded here? https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:412: if self._test_instance.should_save_images: We only get here if screenshot_dir is also set. Do we want to require that? Could we save to a temporary directory if we want to save images but we don't have a screenshot directory configured? https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:489: bucket=self._test_instance.gs_results_bucket + '/render_tests') You've got this in here four times. Should probably have a common way of dealing with it. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:504: (ImageChops.difference( We're already comparing the images in the APK. Should we be computing the diff there, too? https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:519: temp_html.write(''' nit: pull this into a constant at module scope. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... File build/android/pylib/results/presentation/template/main.html (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... 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"> Not sure if this is a rebase error, but this should not change. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/ut... File build/android/pylib/utils/google_storage_helper.py (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:35: Args: Add a returns section. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:75: '_%s' % time.strftime('%Y_%m_%d_T%H_%M_%S', time.localtime()) either pull this out of line or wrap it in parens. as-is, it's a bit hard to tell when scanning that the line below is part of this. https://codereview.chromium.org/2786773002/diff/100001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/test_run... build/android/test_runner.py:167: '--gs-results-bucket', It'd be nice if this was named the same thing in test_results_presentation and here. I think --bucket is a bit to nonspecific here given the number of arguments this has, though.
Example of results with screenshots... https://storage.cloud.google.com/chromium-render-tests/html/bar_foo_123_2017_... Moved all render_test stuff out of this CL. This CL just does screenshot stuff now. Moving diffing to Java would have made CL probably too big. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:690: self._should_save_images = ( On 2017/04/29 at 01:12:25, jbudorick wrote: > nit: do we need to save this separately? I guess I can just check for _gs_results_bucket. Should be fine. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:58: 'ChromePublicTest': 'chrome/test/data/android/render_tests' On 2017/04/29 at 01:12:26, jbudorick wrote: > Can we have the APK store this information in the manifest rather than having it hard-coded here? I think this is a great idea. Unfortunately, I cant think of a clean way to do it. I mean, we could pass the render_dir as an EXTRA or something to the instrumentation test? That might work. For now, will change it to a map from test_package_name: render_dir and add a big TODO. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:412: if self._test_instance.should_save_images: On 2017/04/29 at 01:12:26, jbudorick wrote: > We only get here if screenshot_dir is also set. Do we want to require that? Could we save to a temporary directory if we want to save images but we don't have a screenshot directory configured? I had a CL to do that I uploaded after the first time this landed.... https://codereview.chromium.org/2721323003 Will close that CL and merge it into this one. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:489: bucket=self._test_instance.gs_results_bucket + '/render_tests') On 2017/04/29 at 01:12:26, jbudorick wrote: > You've got this in here four times. Should probably have a common way of dealing with it. Fixed. But going to move render stuff stuff to new CL https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:504: (ImageChops.difference( On 2017/04/29 at 01:12:26, jbudorick wrote: > We're already comparing the images in the APK. Should we be computing the diff there, too? Changing to Java side diffing. Will make CL a bit bigger so going to split it up. Render stuff is going in new CL. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:519: temp_html.write(''' On 2017/04/29 at 01:12:26, jbudorick wrote: > nit: pull this into a constant at module scope. Done. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... File build/android/pylib/results/presentation/template/main.html (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... 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/04/29 at 01:12:26, jbudorick wrote: > Not sure if this is a rebase error, but this should not change. I responded to a comment before. This is not a rebase error. Im unsure why we cant just always grab the CSS file from here? We will *probably* never need different CSS files for upstream/downstream. https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/ut... File build/android/pylib/utils/google_storage_helper.py (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:35: Args: On 2017/04/29 at 01:12:26, jbudorick wrote: > Add a returns section. Done https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:75: '_%s' % time.strftime('%Y_%m_%d_T%H_%M_%S', time.localtime()) On 2017/04/29 at 01:12:26, jbudorick wrote: > either pull this out of line or wrap it in parens. as-is, it's a bit hard to tell when scanning that the line below is part of this. Done https://codereview.chromium.org/2786773002/diff/100001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/test_run... build/android/test_runner.py:167: '--gs-results-bucket', On 2017/04/29 at 01:12:26, jbudorick wrote: > It'd be nice if this was named the same thing in test_results_presentation and here. I think --bucket is a bit to nonspecific here given the number of arguments this has, though. Ack. Leaving as is unless you say otherwise.
On 2017/05/03 21:25:14, mikecase wrote: > Example of results with screenshots... > > https://storage.cloud.google.com/chromium-render-tests/html/bar_foo_123_2017_... This is (very) cool, but: 1) it's kinda surprising that they're all on the home screen. 2) where are the logcats? 3) was this a local run? > > Moved all render_test stuff out of this CL. This CL just does screenshot stuff > now. Moving diffing to Java would have made CL probably too big. > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/in... > File build/android/pylib/instrumentation/instrumentation_test_instance.py > (right): > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/in... > build/android/pylib/instrumentation/instrumentation_test_instance.py:690: > self._should_save_images = ( > On 2017/04/29 at 01:12:25, jbudorick wrote: > > nit: do we need to save this separately? > > I guess I can just check for _gs_results_bucket. Should be fine. > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... > File build/android/pylib/local/device/local_device_instrumentation_test_run.py > (right): > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:58: > 'ChromePublicTest': 'chrome/test/data/android/render_tests' > On 2017/04/29 at 01:12:26, jbudorick wrote: > > Can we have the APK store this information in the manifest rather than having > it hard-coded here? > > I think this is a great idea. Unfortunately, I cant think of a clean way to do > it. I mean, we could pass the render_dir as an EXTRA or something to the > instrumentation test? That might work. > > For now, will change it to a map from test_package_name: render_dir and add a > big TODO. > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:412: > if self._test_instance.should_save_images: > On 2017/04/29 at 01:12:26, jbudorick wrote: > > We only get here if screenshot_dir is also set. Do we want to require that? > Could we save to a temporary directory if we want to save images but we don't > have a screenshot directory configured? > > I had a CL to do that I uploaded after the first time this landed.... > https://codereview.chromium.org/2721323003 > > Will close that CL and merge it into this one. > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:489: > bucket=self._test_instance.gs_results_bucket + '/render_tests') > On 2017/04/29 at 01:12:26, jbudorick wrote: > > You've got this in here four times. Should probably have a common way of > dealing with it. > > Fixed. But going to move render stuff stuff to new CL > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:504: > (ImageChops.difference( > On 2017/04/29 at 01:12:26, jbudorick wrote: > > We're already comparing the images in the APK. Should we be computing the diff > there, too? > > Changing to Java side diffing. Will make CL a bit bigger so going to split it > up. Render stuff is going in new CL. > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:519: > temp_html.write(''' > On 2017/04/29 at 01:12:26, jbudorick wrote: > > nit: pull this into a constant at module scope. > > Done. > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... > File build/android/pylib/results/presentation/template/main.html (right): > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... > 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/04/29 at 01:12:26, jbudorick wrote: > > Not sure if this is a rebase error, but this should not change. > > I responded to a comment before. This is not a rebase error. Im unsure why we > cant just always grab the CSS file from here? We will *probably* never need > different CSS files for upstream/downstream. > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/ut... > File build/android/pylib/utils/google_storage_helper.py (right): > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/ut... > build/android/pylib/utils/google_storage_helper.py:35: Args: > On 2017/04/29 at 01:12:26, jbudorick wrote: > > Add a returns section. > > Done > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/ut... > build/android/pylib/utils/google_storage_helper.py:75: '_%s' % > time.strftime('%Y_%m_%d_T%H_%M_%S', time.localtime()) > On 2017/04/29 at 01:12:26, jbudorick wrote: > > either pull this out of line or wrap it in parens. as-is, it's a bit hard to > tell when scanning that the line below is part of this. > > Done > > https://codereview.chromium.org/2786773002/diff/100001/build/android/test_run... > File build/android/test_runner.py (right): > > https://codereview.chromium.org/2786773002/diff/100001/build/android/test_run... > build/android/test_runner.py:167: '--gs-results-bucket', > On 2017/04/29 at 01:12:26, jbudorick wrote: > > It'd be nice if this was named the same thing in test_results_presentation and > here. I think --bucket is a bit to nonspecific here given the number of > arguments this has, though. > > Ack. Leaving as is unless you say otherwise.
note, this CL won't do anything till we enable it with "--gs-results-bucket chromium-results-details" on the bots
On 2017/05/03 at 21:28:43, mikecase wrote: > note, this CL won't do anything till we enable it with "--gs-results-bucket chromium-results-details" on the bots Oh yeah @jbudorick, this is a local run where I just called assertEquals(1,2) in some random test. Wasnt really expecting interesting results from this. Just a demo it works I guess
https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:387: screenshot_dir = self._test_instance.screenshot_dir or screenshot_dir nit: screenshot_host_dir https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:391: time.strftime('%Y%m%dT%H%M%S', time.localtime())) same as below, should this just use UTC time? https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/ut... File build/android/pylib/utils/google_storage_helper.py (right): https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:28: _AUTHENICATED_URL = 'https://storage.cloud.google.com/%s/' super nit: similar as below https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:32: def upload(name, filepath, bucket, content_type=None, authenticated_link=True): super nit: maybe link_to_authenticate? authenticated_link sounds like a link that requires no further authentication https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:43: work when uploading to public storage buckets. nit: as you mentioned, we might want to always use link_to_authenticate. Maybe add a comment here https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:77: '_%s' % time.strftime('%Y_%m_%d_T%H_%M_%S', time.localtime()) hmm, should this use UTC time?
also changed link name to "post_test_screenshot" https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:387: screenshot_dir = self._test_instance.screenshot_dir or screenshot_dir On 2017/05/03 at 22:23:38, the real yoland wrote: > nit: screenshot_host_dir Done https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:391: time.strftime('%Y%m%dT%H%M%S', time.localtime())) On 2017/05/03 at 22:23:38, the real yoland wrote: > same as below, should this just use UTC time? Done https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/ut... File build/android/pylib/utils/google_storage_helper.py (right): https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:32: def upload(name, filepath, bucket, content_type=None, authenticated_link=True): On 2017/05/03 at 22:23:39, the real yoland wrote: > super nit: maybe link_to_authenticate? authenticated_link sounds like a link that requires no further authentication Ack. I think authenticated_link is still less awkward/confusing since link_to_authenticate makes it sound like its a new, separate link just to authenticate. When really its just a link that requires you to sign into Google account. https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:43: work when uploading to public storage buckets. On 2017/05/03 at 22:23:38, the real yoland wrote: > nit: as you mentioned, we might want to always use link_to_authenticate. Maybe add a comment here Improved comment. And have default for authenticated_link=True https://codereview.chromium.org/2786773002/diff/160001/build/android/pylib/ut... build/android/pylib/utils/google_storage_helper.py:77: '_%s' % time.strftime('%Y_%m_%d_T%H_%M_%S', time.localtime()) On 2017/05/03 at 22:23:38, the real yoland wrote: > hmm, should this use UTC time? probably! Done
Could always change link to emojis. Concise and descriptive "📱😵📷"
lgtm with nits https://codereview.chromium.org/2786773002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2786773002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:322: time.strftime('%Y%m%dT%H%M%S', time.gmtime()), nit: would be great to specify UTC time as well. e.g. '%Y%m%dT%H%M%S-UTC'
On 2017/05/03 22:47:05, mikecase wrote: > Could always change link to emojis. Concise and descriptive "📱😵📷" 😂
https://codereview.chromium.org/2786773002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2786773002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:322: time.strftime('%Y%m%dT%H%M%S', time.gmtime()), On 2017/05/03 at 22:49:00, the real yoland wrote: > nit: would be great to specify UTC time as well. > e.g. '%Y%m%dT%H%M%S-UTC' Done
Description was changed from ========== (Reland) Add failure screenshots and images to results detail. ========== to ========== (Reland) Add post-test screenshots to results detail. ==========
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yolandyan@chromium.org Link to the patchset: https://codereview.chromium.org/2786773002/#ps220001 (title: "yolands nit")
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": 220001, "attempt_start_ts": 1493853601800500,
"parent_rev": "7cea0dc7ee0ea84bde58808ff6d42d9ccc73f704", "commit_rev":
"2bb1cf11d22d50c5750d8d3b1356f0caa43d359e"}
Message was sent while issue was closed.
Description was changed from ========== (Reland) Add post-test screenshots to results detail. ========== to ========== (Reland) Add post-test screenshots to results detail. Review-Url: https://codereview.chromium.org/2786773002 Cr-Commit-Position: refs/heads/master@{#469237} Committed: https://chromium.googlesource.com/chromium/src/+/2bb1cf11d22d50c5750d8d3b1356... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/2bb1cf11d22d50c5750d8d3b1356...
Message was sent while issue was closed.
https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... File build/android/pylib/results/presentation/template/main.html (right): https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... 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/05/03 21:25:14, mikecase wrote: > On 2017/04/29 at 01:12:26, jbudorick wrote: > > Not sure if this is a rebase error, but this should not change. > > I responded to a comment before. This is not a rebase error. Im unsure why we > cant just always grab the CSS file from here? We will *probably* never need > different CSS files for upstream/downstream. gah, I missed that you landed this change :(
Message was sent while issue was closed.
On 2017/05/08 at 17:21:56, jbudorick wrote: > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... > File build/android/pylib/results/presentation/template/main.html (right): > > https://codereview.chromium.org/2786773002/diff/100001/build/android/pylib/re... > 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/05/03 21:25:14, mikecase wrote: > > On 2017/04/29 at 01:12:26, jbudorick wrote: > > > Not sure if this is a rebase error, but this should not change. > > > > I responded to a comment before. This is not a rebase error. Im unsure why we > > cant just always grab the CSS file from here? We will *probably* never need > > different CSS files for upstream/downstream. > > gah, I missed that you landed this change :( why is this bad? Why would we ever want different css? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
