Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2866103002:
Add render test results to the results_details webpage. (Closed)
|
Created:
3 years, 7 months ago by mikecase (-- gone --) Modified:
3 years, 7 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. |
DescriptionAdd render test results to the results_details webpage.
This will (1) make render tests perform pixel-wise
comparison and generate diff images, (2) allow you
to specify output directory for where to dump the
result images on the device, and (3) upload the results
to be visible from buildbot status page.
Review-Url: https://codereview.chromium.org/2866103002
Cr-Commit-Position: refs/heads/master@{#471144}
Committed: https://chromium.googlesource.com/chromium/src/+/49c5485e248597a4afe07101241bda3c3060cad8
Patch Set 1 #Patch Set 2 : (Reland) Add failure screenshots and images to results detail. #
Total comments: 15
Patch Set 3 : (Reland) Add failure screenshots and images to results detail. #Patch Set 4 : Addressed Comments #
Total comments: 4
Patch Set 5 : John's nits #Patch Set 6 : Fixed jinja2 imports #Patch Set 7 : Fix missing markupsafe import #Patch Set 8 : REBASE #Patch Set 9 : (Reland) Add failure screenshots and images to results detail. #Patch Set 10 : Fixed bug in _IsRenderTest where it would always return True... #Patch Set 11 #
Messages
Total messages: 54 (28 generated)
mikecase@chromium.org changed reviewers: + jbudorick@chromium.org
Just update on Render test stuff... The infra changes blocking render tests from being displayed on our bots were committed last week. So we can go ahead with this. Once this CL lands, result webpages such as... https://storage.cloud.google.com/chromium-render-tests/html/bar_foo_123_2017_... ...will show up on our bots. If you click the render_test on the above link you can view the failed render tests directly from the webpage. Changes of this CL are basically... - Allow python test_runner to specify where results are saved (so we don't have to have a bunch of hard-coded paths in Java and Python that have to match up) - Move diffing logic in Java - Change it so failure images are saved even with DO_NOT_FAIL flag is set to true.
On 2017/05/08 at 22:41:52, mikecase wrote: > Just update on Render test stuff... > > The infra changes blocking render tests from being displayed on our bots were committed last week. So we can go ahead with this. > > Once this CL lands, result webpages such as... > https://storage.cloud.google.com/chromium-render-tests/html/bar_foo_123_2017_... > > ...will show up on our bots. If you click the render_test on the above link you can view the failed render tests directly from the webpage. > > > Changes of this CL are basically... > - Allow python test_runner to specify where results are saved (so we don't have to have a bunch of hard-coded paths in Java and Python that have to match up) > - Move diffing logic in Java > - Change it so failure images are saved even with DO_NOT_FAIL flag is set to true. Something got messed up in the example linked above. This is a better example of what the results will look like... https://storage.cloud.google.com/chromium-render-tests/html/bar_foo_123_2017_...
peconn@chromium.org changed reviewers: + peconn@chromium.org
Thanks so much for your work on this - it looks brilliant! The comments I made on RenderUtils.java are more general thoughts - I'm not bothered if you address them. Once we get using the RenderTests I'm sure we'll figure out how to deal with them elegantly. https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:46: RE_RENDER_IMAGE_NAME = re.compile( Could you please put a comment here pointing to RenderUtils.imageName and vice versa saying that if you update one you should also update the other? https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:52: RENDER_TESTS_HTML_TEMPLATE = ''' Could you move this into a separate file (or Jinja template for single renders)? I was hoping I'd be able to change the final display webpage and that would mean I don't have to touch big python infrastructure files. https://codereview.chromium.org/2866103002/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2866103002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:144: saveBitmap(goldenBitmap, goldenOutputFile); What happens here if the golden isn't found? saveBitmap has a try/catch so we shouldn't crash but is an image generated? https://codereview.chromium.org/2866103002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:245: return ComparisonResult.MISMATCH; So if the golden doesn't match the dimensions of the test, we still save the diff (which I'm assuming is just a blank bitmap the same size as test). This seems pointless.
Can you split each of the links in test_results_presentation onto its own line? Otherwise, this looks pretty good. https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:46: RE_RENDER_IMAGE_NAME = re.compile( On 2017/05/09 08:52:39, PEConn wrote: > Could you please put a comment here pointing to RenderUtils.imageName and vice > versa saying that if you update one you should also update the other? Might be good to add a few tests on both sides as well. https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:61: <td><img src="%s"/></td> Are these ok with an empty URL? (It's been a while...) https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:275: flags_to_add = [] This change was in your other CL? https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:525: device.RemovePath(render_tests_device_output_dir, recursive=True) I think this should be the responsibility of _RunTest. Creating the directory in that scope and deleting it in this one is liable to be confusing.
New example here... https://storage.cloud.google.com/chromium-render-tests/html/bar_foo_123_2017_... very similar, except the failures now show up on their own line in the "logs" table element. Also, the images scale to the page width. https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:46: RE_RENDER_IMAGE_NAME = re.compile( On 2017/05/09 at 08:52:39, PEConn wrote: > Could you please put a comment here pointing to RenderUtils.imageName and vice versa saying that if you update one you should also update the other? Done https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:52: RENDER_TESTS_HTML_TEMPLATE = ''' On 2017/05/09 at 08:52:39, PEConn wrote: > Could you move this into a separate file (or Jinja template for single renders)? I was hoping I'd be able to change the final display webpage and that would mean I don't have to touch big python infrastructure files. Done. Moved into own file so it will be easier to change later on. Also made a very minor improvement to make the images scale to the page width https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:61: <td><img src="%s"/></td> On 2017/05/09 at 14:33:26, jbudorick wrote: > Are these ok with an empty URL? (It's been a while...) Yeah, I doubled checked, it just doesnt load anything. Works fine. https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:275: flags_to_add = [] On 2017/05/09 at 14:33:26, jbudorick wrote: > This change was in your other CL? Yeah, have to make the same change here as well. Will rebase this CL once the other lands. https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:525: device.RemovePath(render_tests_device_output_dir, recursive=True) On 2017/05/09 at 14:33:26, jbudorick wrote: > I think this should be the responsibility of _RunTest. Creating the directory in that scope and deleting it in this one is liable to be confusing. Done https://codereview.chromium.org/2866103002/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2866103002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:144: saveBitmap(goldenBitmap, goldenOutputFile); On 2017/05/09 at 08:52:39, PEConn wrote: > What happens here if the golden isn't found? saveBitmap has a try/catch so we shouldn't crash but is an image generated? Changed it so that if golden is not found, it does not attempt to save the golden or diff images. https://codereview.chromium.org/2866103002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:245: return ComparisonResult.MISMATCH; On 2017/05/09 at 08:52:39, PEConn wrote: > So if the golden doesn't match the dimensions of the test, we still save the diff (which I'm assuming is just a blank bitmap the same size as test). This seems pointless. Making the diff Bitmap be max_width by max_height. Pixels coords that occur in one Bitmap but not the other will show up as RED
On 2017/05/10 01:05:01, mikecase wrote: > New example here... > https://storage.cloud.google.com/chromium-render-tests/html/bar_foo_123_2017_... > > very similar, except the failures now show up on their own line in the "logs" > table element. Also, the images scale to the page width. > > https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... > File build/android/pylib/local/device/local_device_instrumentation_test_run.py > (right): > > https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:46: > RE_RENDER_IMAGE_NAME = re.compile( > On 2017/05/09 at 08:52:39, PEConn wrote: > > Could you please put a comment here pointing to RenderUtils.imageName and vice > versa saying that if you update one you should also update the other? > > Done > > https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:52: > RENDER_TESTS_HTML_TEMPLATE = ''' > On 2017/05/09 at 08:52:39, PEConn wrote: > > Could you move this into a separate file (or Jinja template for single > renders)? I was hoping I'd be able to change the final display webpage and that > would mean I don't have to touch big python infrastructure files. > > Done. Moved into own file so it will be easier to change later on. Also made a > very minor improvement to make the images scale to the page width > > https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:61: > <td><img src="%s"/></td> > On 2017/05/09 at 14:33:26, jbudorick wrote: > > Are these ok with an empty URL? (It's been a while...) > > Yeah, I doubled checked, it just doesnt load anything. Works fine. > > https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:275: > flags_to_add = [] > On 2017/05/09 at 14:33:26, jbudorick wrote: > > This change was in your other CL? > > Yeah, have to make the same change here as well. Will rebase this CL once the > other lands. > > https://codereview.chromium.org/2866103002/diff/20001/build/android/pylib/loc... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:525: > device.RemovePath(render_tests_device_output_dir, recursive=True) > On 2017/05/09 at 14:33:26, jbudorick wrote: > > I think this should be the responsibility of _RunTest. Creating the directory > in that scope and deleting it in this one is liable to be confusing. > > Done > > https://codereview.chromium.org/2866103002/diff/20001/chrome/test/android/jav... > File > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java > (right): > > https://codereview.chromium.org/2866103002/diff/20001/chrome/test/android/jav... > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:144: > saveBitmap(goldenBitmap, goldenOutputFile); > On 2017/05/09 at 08:52:39, PEConn wrote: > > What happens here if the golden isn't found? saveBitmap has a try/catch so we > shouldn't crash but is an image generated? > > Changed it so that if golden is not found, it does not attempt to save the > golden or diff images. > > https://codereview.chromium.org/2866103002/diff/20001/chrome/test/android/jav... > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:245: > return ComparisonResult.MISMATCH; > On 2017/05/09 at 08:52:39, PEConn wrote: > > So if the golden doesn't match the dimensions of the test, we still save the > diff (which I'm assuming is just a blank bitmap the same size as test). This > seems pointless. > > Making the diff Bitmap be max_width by max_height. Pixels coords that occur in > one Bitmap but not the other will show up as RED Great! LGTM
lgtm w/ nit https://codereview.chromium.org/2866103002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2866103002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:332: # TODO(mikecase): Add DeviceTempDirectory class and use that instead. This would be a pretty nice addition to device_temp_file. :) https://codereview.chromium.org/2866103002/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2866103002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:254: private static ComparisonResult compareBitmapToGolden(Bitmap test, Bitmap golden, Bitmap diff) { Once we get java-side test tracing, I'd be interested to see how this performs. https://codereview.chromium.org/2866103002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:269: private static int comparePixels(Bitmap testImage, Bitmap goldenImage, Bitmap diffImage, nit: This doesn't seem particularly useful.
Description was changed from ========== Add render test results to the results_details webpage.. ========== to ========== Add render test results to the results_details webpage. This will (1) make render tests perform pixel-wise comparison and generate diff images, (2) allow you to specify output directory for where to dump the result images on the device, and (3) upload the results to be visible from buildbot status page. ==========
This CL will take effect once --gs-results-bucket arg is passed to out bots. I have another CL out to add that. https://codereview.chromium.org/2866103002/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2866103002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:269: private static int comparePixels(Bitmap testImage, Bitmap goldenImage, Bitmap diffImage, On 2017/05/10 at 15:10:05, jbudorick wrote: > nit: This doesn't seem particularly useful. Deleted!
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2866103002/#ps80001 (title: "John's nits")
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by mikecase@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed jinja2 pydeps. Had to "pip uninstall jinja2" to remove the jinja2 system module. Then the pydeps script actually found third_party/jinja2. My question is now, if other people have jinja2 installed, hopefully they dont undo the change when generating pydeps in the future! :-O
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2866103002/#ps100001 (title: "Fixed jinja2 imports")
On 2017/05/10 18:13:08, mikecase wrote: > Fixed jinja2 pydeps. Had to "pip uninstall jinja2" to remove the jinja2 system > module. Then the pydeps script actually found third_party/jinja2. > > My question is now, if other people have jinja2 installed, hopefully they dont > undo the change when generating pydeps in the future! :-O they'll have a hard time getting that kind of change through the trybots.
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2866103002/#ps120001 (title: "Fix missing markupsafe import")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2866103002/#ps140001 (title: "REBASE")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2866103002/#ps160001 (title: "(Reland) Add failure screenshots and images to results detail.")
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 mikecase@chromium.org
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2866103002/#ps180001 (title: "Fixed bug in _IsRenderTest where it would always return True...")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2866103002/#ps200001 (title: "")
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mikecase@chromium.org
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": 200001, "attempt_start_ts": 1494542181313020,
"parent_rev": "e0ecce46642e1d001deae09f41429c44517d84b9", "commit_rev":
"49c5485e248597a4afe07101241bda3c3060cad8"}
Message was sent while issue was closed.
Description was changed from ========== Add render test results to the results_details webpage. This will (1) make render tests perform pixel-wise comparison and generate diff images, (2) allow you to specify output directory for where to dump the result images on the device, and (3) upload the results to be visible from buildbot status page. ========== to ========== Add render test results to the results_details webpage. This will (1) make render tests perform pixel-wise comparison and generate diff images, (2) allow you to specify output directory for where to dump the result images on the device, and (3) upload the results to be visible from buildbot status page. Review-Url: https://codereview.chromium.org/2866103002 Cr-Commit-Position: refs/heads/master@{#471144} Committed: https://chromium.googlesource.com/chromium/src/+/49c5485e248597a4afe07101241b... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/49c5485e248597a4afe07101241b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
