Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2114963002:
Create Java infrastructure for UI Render Tests. (Closed)
|
Created:
4 years, 5 months ago by PEConn Modified:
4 years, 4 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, ntp-dev+reviews_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate Java infrastructure for UI Render Tests.
Creates the first part of the infrastructure that will allow us to test for
any regressions to the Chrome on Android UI. This includes utilities to save
Views to file, an annotation (RenderTest) to identify the instrumentation
tests that use this and two examples of their use.
The aim is to follow this up with host side code that grabs the images after
the tests have been run and performs pixel comparisons on them against golden
images stored in the repository.
BUG=614305
Committed: https://crrev.com/324e06c25c137f097e6f001b12abb54d3e8bf268
Cr-Commit-Position: refs/heads/master@{#412905}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Client side comparison. #
Total comments: 12
Patch Set 3 : Primarily nits. #
Total comments: 6
Patch Set 4 : Update folders and error reporting. #Patch Set 5 : Missing files only cause failure on certain devices. #Patch Set 6 : Add REPORT_ONLY_DO_NOT_FAIL. #
Total comments: 21
Patch Set 7 : SnippetsBridge -> SnippetsSource in NewTabPageAdapterTest. #
Total comments: 2
Patch Set 8 : Add regenerate goldens flag. #
Total comments: 7
Patch Set 9 : Nits. #
Total comments: 4
Patch Set 10 : Merge and rename. #Patch Set 11 : Merge and fix Findbugs error. #Patch Set 12 : Throw when mkdir fails. #Messages
Total messages: 60 (20 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, jbudorick@chromium.org
PTAL at this work in progress of the Java side of the Android UI Render Tests. You can see an example of the images produced at http://imgur.com/a/83tcP (some of the images are on transparent backgrounds, but this doesn't come across well on imgur).
These tests can all be run using '-A RenderTest' or '--annotation RenderTest'.
Additionally, jbudorick will be most interested in: base/test/android/javatests/src/org/chromium/base/test/util/RenderTest.java chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java The changes to build/android/pylib/instrumentation/instrumentation_test_instance.py build/android/pylib/local/device/local_device_instrumentation_test_run.py build/android/test_runner.py Are just to add the 'RenderTest' annotation to the pipeline.
You win the award for scariest CL I've received this week, although I was expecting this one. On 2016/07/01 15:14:13, PEConn1 wrote: > Additionally, jbudorick will be most interested in: Oh, I'm interested in all of it. > > base/test/android/javatests/src/org/chromium/base/test/util/RenderTest.java > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java > > The changes to > build/android/pylib/instrumentation/instrumentation_test_instance.py > build/android/pylib/local/device/local_device_instrumentation_test_run.py > build/android/test_runner.py > > Are just to add the 'RenderTest' annotation to the pipeline.
I looked at Espresso a bit since Wednesday and don't think it alone can address what you're looking to verify, so I'm ok with moving forward with screenshot comparison tests to some degree. That said, I'm not sure I'm sold on the host-side comparison plan. https://codereview.chromium.org/2114963002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2114963002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:26: ('RenderTest', 3 * 60), I don't think RenderTest should have its own timeout implications, i.e., a typical render test should look like: @MediumTest @RenderTest('path/to/golden/directory') public void testRenderFoo() { ... https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:92: viewRenderer.save(mNtp.getView(), "new_tab_page_scrolled"); The more I think about it, the more I think that we should really be doing the golden comparison in the test rather than on the host. I know that may make things a little trickier for rebaselining, but I think we can come up with something reasonable to do for that case. https://codereview.chromium.org/2114963002/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:23: public class RenderUtils { @RenderTest and any supporting java code should be in the same package.
https://codereview.chromium.org/2114963002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2114963002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:26: ('RenderTest', 3 * 60), On 2016/07/01 15:34:21, jbudorick wrote: > I don't think RenderTest should have its own timeout implications, i.e., a > typical render test should look like: > > @MediumTest > @RenderTest('path/to/golden/directory') > public void testRenderFoo() { > ... Done. https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:92: viewRenderer.save(mNtp.getView(), "new_tab_page_scrolled"); On 2016/07/01 15:34:21, jbudorick wrote: > The more I think about it, the more I think that we should really be doing the > golden comparison in the test rather than on the host. I know that may make > things a little trickier for rebaselining, but I think we can come up with > something reasonable to do for that case. OK, I can see the benefits of that - it requires much less change to the host infrastructure. In regards to the tests I've written, all I really need to do is change ViewRenderer.save to ViewRenderer.compare and put all the logic there. https://codereview.chromium.org/2114963002/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:23: public class RenderUtils { On 2016/07/01 15:34:22, jbudorick wrote: > @RenderTest and any supporting java code should be in the same package. So the main reason I put RenderUtils in here instead of in base/ was because it relied on UiUtils (in ui/). Should I move @RenderTest over to here then?
https://codereview.chromium.org/2114963002/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:23: public class RenderUtils { On 2016/07/01 16:46:13, PEConn1 wrote: > On 2016/07/01 15:34:22, jbudorick wrote: > > @RenderTest and any supporting java code should be in the same package. > > So the main reason I put RenderUtils in here instead of in base/ was because it > relied on UiUtils (in ui/). Should I move @RenderTest over to here then? I'm not sure. I could certainly see the case for having @RenderTest in base/, but if it lives there, I imagine that some of the supporting code (i.e., whatever doesn't depend on ui/) should also live there.
https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:92: viewRenderer.save(mNtp.getView(), "new_tab_page_scrolled"); On 2016/07/01 16:46:13, PEConn1 wrote: > On 2016/07/01 15:34:21, jbudorick wrote: > > The more I think about it, the more I think that we should really be doing the > > golden comparison in the test rather than on the host. I know that may make > > things a little trickier for rebaselining, but I think we can come up with > > something reasonable to do for that case. > > OK, I can see the benefits of that - it requires much less change to the host > infrastructure. In regards to the tests I've written, all I really need to do is > change ViewRenderer.save to ViewRenderer.compare and put all the logic there. Also, if we do device-side comparison, I'm not sure if we need @RenderTest at all (and we certainly don't need the directory in the annotation data).
On 2016/07/01 16:50:20, jbudorick wrote: > https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... > File > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java > (right): > > https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:92: > viewRenderer.save(mNtp.getView(), "new_tab_page_scrolled"); > On 2016/07/01 16:46:13, PEConn1 wrote: > > On 2016/07/01 15:34:21, jbudorick wrote: > > > The more I think about it, the more I think that we should really be doing > the > > > golden comparison in the test rather than on the host. I know that may make > > > things a little trickier for rebaselining, but I think we can come up with > > > something reasonable to do for that case. > > > > OK, I can see the benefits of that - it requires much less change to the host > > infrastructure. In regards to the tests I've written, all I really need to do > is > > change ViewRenderer.save to ViewRenderer.compare and put all the logic there. > > Also, if we do device-side comparison, I'm not sure if we need @RenderTest at > all (and we certainly don't need the directory in the annotation data). How would we specify where to get the golden images from? Would we just copy every single golden image to the device?
On 2016/07/12 16:34:48, PEConn1 wrote: > On 2016/07/01 16:50:20, jbudorick wrote: > > > https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... > > File > > > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java > > (right): > > > > > https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... > > > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:92: > > viewRenderer.save(mNtp.getView(), "new_tab_page_scrolled"); > > On 2016/07/01 16:46:13, PEConn1 wrote: > > > On 2016/07/01 15:34:21, jbudorick wrote: > > > > The more I think about it, the more I think that we should really be doing > > the > > > > golden comparison in the test rather than on the host. I know that may > make > > > > things a little trickier for rebaselining, but I think we can come up with > > > > something reasonable to do for that case. > > > > > > OK, I can see the benefits of that - it requires much less change to the > host > > > infrastructure. In regards to the tests I've written, all I really need to > do > > is > > > change ViewRenderer.save to ViewRenderer.compare and put all the logic > there. > > > > Also, if we do device-side comparison, I'm not sure if we need @RenderTest at > > all (and we certainly don't need the directory in the annotation data). > > How would we specify where to get the golden images from? Would we just copy > every single golden image to the device? We'd specify a path relative to the chromium root + add it to the data in the relevant gn target. It'd then be pushed to the device and available at UrlUtils.getIsolatedTestRoot() + "/path/relative/to/chromium/root" This is what we do with all of our other test data, as well.
On 2016/07/12 16:38:57, jbudorick wrote: > On 2016/07/12 16:34:48, PEConn1 wrote: > > On 2016/07/01 16:50:20, jbudorick wrote: > > > > > > https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... > > > File > > > > > > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... > > > > > > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:92: > > > viewRenderer.save(mNtp.getView(), "new_tab_page_scrolled"); > > > On 2016/07/01 16:46:13, PEConn1 wrote: > > > > On 2016/07/01 15:34:21, jbudorick wrote: > > > > > The more I think about it, the more I think that we should really be > doing > > > the > > > > > golden comparison in the test rather than on the host. I know that may > > make > > > > > things a little trickier for rebaselining, but I think we can come up > with > > > > > something reasonable to do for that case. > > > > > > > > OK, I can see the benefits of that - it requires much less change to the > > host > > > > infrastructure. In regards to the tests I've written, all I really need to > > do > > > is > > > > change ViewRenderer.save to ViewRenderer.compare and put all the logic > > there. > > > > > > Also, if we do device-side comparison, I'm not sure if we need @RenderTest > at > > > all (and we certainly don't need the directory in the annotation data). > > > > How would we specify where to get the golden images from? Would we just copy > > every single golden image to the device? > > We'd specify a path relative to the chromium root + add it to the data in the > relevant gn target. It'd then be pushed to the device and available at > UrlUtils.getIsolatedTestRoot() + "/path/relative/to/chromium/root" > > This is what we do with all of our other test data, as well. OK. And finally - where shall the golden images live?
On 2016/07/12 16:43:14, PEConn1 wrote: > On 2016/07/12 16:38:57, jbudorick wrote: > > On 2016/07/12 16:34:48, PEConn1 wrote: > > > On 2016/07/01 16:50:20, jbudorick wrote: > > > > > > > > > > https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... > > > > File > > > > > > > > > > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2114963002/diff/1/chrome/android/javatests/sr... > > > > > > > > > > chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:92: > > > > viewRenderer.save(mNtp.getView(), "new_tab_page_scrolled"); > > > > On 2016/07/01 16:46:13, PEConn1 wrote: > > > > > On 2016/07/01 15:34:21, jbudorick wrote: > > > > > > The more I think about it, the more I think that we should really be > > doing > > > > the > > > > > > golden comparison in the test rather than on the host. I know that may > > > make > > > > > > things a little trickier for rebaselining, but I think we can come up > > with > > > > > > something reasonable to do for that case. > > > > > > > > > > OK, I can see the benefits of that - it requires much less change to the > > > host > > > > > infrastructure. In regards to the tests I've written, all I really need > to > > > do > > > > is > > > > > change ViewRenderer.save to ViewRenderer.compare and put all the logic > > > there. > > > > > > > > Also, if we do device-side comparison, I'm not sure if we need @RenderTest > > at > > > > all (and we certainly don't need the directory in the annotation data). > > > > > > How would we specify where to get the golden images from? Would we just copy > > > every single golden image to the device? > > > > We'd specify a path relative to the chromium root + add it to the data in the > > relevant gn target. It'd then be pushed to the device and available at > > UrlUtils.getIsolatedTestRoot() + "/path/relative/to/chromium/root" > > > > This is what we do with all of our other test data, as well. > > OK. And finally - where shall the golden images live? For tests in chrome/, probably in a subdirectory of chrome/test/data/android/
PTAL - I've updated the code to do the comparison client side. I am aware I need to change the directory the golden files will be stored in.
https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:65: @MediumTest Should this be a @RenderTest now? https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java (right): https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:77: int firstSnippet = 0; Oh, I see, you want this outside of the loop so you can get the value? Hm, I wonder whether we should just extract this to a function that can return the index from inside the loop. Or just use the existing method we have for this? :) https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:95: "", // Amp Url Nit: "AMP" and "URL". Otherwise the capitalization is probably not necessary. https://codereview.chromium.org/2114963002/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:57: * the rendered View matches the golden, so should be used in an assertTrue(). Hm, if this method already has code to handle a failure, should we move the assert in there as well? https://codereview.chromium.org/2114963002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:74: Log.w(TAG, "Image comparison failed. Golden image: %s . Rendered image: %s .", I would remove the space before the period (optionally put the filename in quotes as delimiters). https://codereview.chromium.org/2114963002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:97: private static void saveBmp(Bitmap bitmap, File file) throws IOException { Nit: use "Bitmap" instead of "Bmp"? It's not much longer, and it doesn't sound like Windows .bmp files.
https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:65: @MediumTest On 2016/07/13 09:20:13, Bernhard Bauer wrote: > Should this be a @RenderTest now? No, we decided that we did not need the RenderTest annotation anymore. https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java (right): https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:95: "", // Amp Url On 2016/07/13 09:20:13, Bernhard Bauer wrote: > Nit: "AMP" and "URL". Otherwise the capitalization is probably not necessary. Done. https://codereview.chromium.org/2114963002/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:57: * the rendered View matches the golden, so should be used in an assertTrue(). On 2016/07/13 09:20:13, Bernhard Bauer wrote: > Hm, if this method already has code to handle a failure, should we move the > assert in there as well? Done. https://codereview.chromium.org/2114963002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:74: Log.w(TAG, "Image comparison failed. Golden image: %s . Rendered image: %s .", On 2016/07/13 09:20:13, Bernhard Bauer wrote: > I would remove the space before the period (optionally put the filename in > quotes as delimiters). Done. https://codereview.chromium.org/2114963002/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:97: private static void saveBmp(Bitmap bitmap, File file) throws IOException { On 2016/07/13 09:20:13, Bernhard Bauer wrote: > Nit: use "Bitmap" instead of "Bmp"? It's not much longer, and it doesn't sound > like Windows .bmp files. Done.
https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java (right): https://codereview.chromium.org/2114963002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:77: int firstSnippet = 0; On 2016/07/13 09:20:13, Bernhard Bauer wrote: > Oh, I see, you want this outside of the loop so you can get the value? Hm, I > wonder whether we should just extract this to a function that can return the > index from inside the loop. Or just use the existing method we have for this? :) ^^^ https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:49: * but can be used during development to generate the golden images for tests. I wonder whether we should simply fail a bit more gracefully if the golden file doesn't exist on disk. Then the developer workflow would consist of writing the test, pulling the failed screenshots from the device, and adding them. Test-driven development and all :) https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:95: Assert.assertTrue(!mFailed); It would probably be good to record which check(s) failed (we have the Log.w, but that will only show up in a logcat together with all the logspam).
https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:125: File path = new File(Environment.getExternalStorageDirectory() + "/" + folder); Environment.getExternalStorageDirectory already returns a file, so you don't need to stringify it and then parse it again as a File. Just use the two-argument constructor File(File, String). Same below.
@jbudorick - What do I need to do with regards to build files to get the *.pngs working correctly. As far as I can tell, whenever you run an instrumentation test, all of chrome/test/data is copied. I consider this draft to be near completion - the only remaining task I have is to modify the command line flags for the NewTabPageTest to force cards UI. https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:49: * but can be used during development to generate the golden images for tests. On 2016/07/13 11:24:01, Bernhard Bauer wrote: > I wonder whether we should simply fail a bit more gracefully if the golden file > doesn't exist on disk. Then the developer workflow would consist of writing the > test, pulling the failed screenshots from the device, and adding them. > Test-driven development and all :) Done. With the change to how check() works (so now a test runs all the way through and only fails at the end), this seems unnecessary. https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:95: Assert.assertTrue(!mFailed); On 2016/07/13 11:24:01, Bernhard Bauer wrote: > It would probably be good to record which check(s) failed (we have the Log.w, > but that will only show up in a logcat together with all the logspam). Done. https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:125: File path = new File(Environment.getExternalStorageDirectory() + "/" + folder); On 2016/07/13 13:32:01, Bernhard Bauer wrote: > Environment.getExternalStorageDirectory already returns a file, so you don't > need to stringify it and then parse it again as a File. Just use the > two-argument constructor File(File, String). Same below. Acknowledged.
On 2016/07/14 15:51:11, PEConn1 wrote: > @jbudorick - What do I need to do with regards to build files to get the *.pngs > working correctly. As far as I can tell, whenever you run an instrumentation > test, all of chrome/test/data is copied. For now, what's pushed when running chrome_public_test_apk is controlled by //chrome/chrome_public_test_apk.isolate. As that already contains //chrome/test/data/android, you shouldn't have to modify anything. (We'll eventually control this via data fields within gn targets, but even if that were happening now, we'd still already be pushing //chrome/test/data/android.) > > I consider this draft to be near completion - the only remaining task I have is > to modify the command line flags for the NewTabPageTest to force cards UI. > > https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... > File > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java > (right): > > https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:49: > * but can be used during development to generate the golden images for tests. > On 2016/07/13 11:24:01, Bernhard Bauer wrote: > > I wonder whether we should simply fail a bit more gracefully if the golden > file > > doesn't exist on disk. Then the developer workflow would consist of writing > the > > test, pulling the failed screenshots from the device, and adding them. > > Test-driven development and all :) > > Done. > > With the change to how check() works (so now a test runs all the way through and > only fails at the end), this seems unnecessary. > > https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:95: > Assert.assertTrue(!mFailed); > On 2016/07/13 11:24:01, Bernhard Bauer wrote: > > It would probably be good to record which check(s) failed (we have the Log.w, > > but that will only show up in a logcat together with all the logspam). > > Done. > > https://codereview.chromium.org/2114963002/diff/40001/chrome/test/android/jav... > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:125: > File path = new File(Environment.getExternalStorageDirectory() + "/" + folder); > On 2016/07/13 13:32:01, Bernhard Bauer wrote: > > Environment.getExternalStorageDirectory already returns a file, so you don't > > need to stringify it and then parse it again as a File. Just use the > > two-argument constructor File(File, String). Same below. > > Acknowledged.
I think this patch (#5) contains all the features that I was planning on including.
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Can I ping you two on this please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
partial comments. I'll look a bit more later today. I have significant concerns about the implementation of ViewRenderer. https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:33: private static final String GOLDEN_FOLDER = "android/render_tests"; Ideally, these would not be hard-coded. https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:70: * recommended way of using the ViewRenderer is to call multiple checks and then call I don't think this is a good idea -- it trades expediency of regenerating expectations for clarity of error messaging. This really wants something like gtest's EXPECT_* macros, but I don't know of something comparable in junit. I can see why you'd be interested in running everything, but we need traces on each failure that point to the exact failure (rather than to the assertAllChecksPassed call site). https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:81: public boolean check(View view, String id) throws IOException { Instead of being a function that returns a boolean (making it suitable for assertTrue), WDYT about having this be an assertion? "assertViewMatches(view, image)" or some such. This would allow you to have more descriptive error messaging. https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:151: private static String imageName(Activity activity, String testClass, String desc) { I think that, between this and createPath, this class knows more than it should (and hard-codes more than it should). https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:173: File path = new File(UrlUtils.getTestFilePath(folder)); Please don't add new uses of getTestFilePath (https://codesearch.chromium.org/chromium/src/base/test/android/javatests/src/...); prefer getIsolatedTestFilePath. You'll need to use //chrome/test/data explicitly.
https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:70: * recommended way of using the ViewRenderer is to call multiple checks and then call On 2016/07/18 15:59:26, jbudorick wrote: > I don't think this is a good idea -- it trades expediency of regenerating > expectations for clarity of error messaging. This really wants something like > gtest's EXPECT_* macros, but I don't know of something comparable in junit. Yeah, EXPECT_EQ was what I was thinking of here when I suggested that. > I can see why you'd be interested in running everything, but we need traces on > each failure that point to the exact failure (rather than to the > assertAllChecksPassed call site). We should be able to do this, right? We could just create a new exception and then store it rather than throwing it (and then throw an error at the end, with all the exceptions).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2114963002/diff/120001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/120001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:33: private static final String GOLDEN_FOLDER = "android/render_tests"; w.r.t. not hard-coding this -- what about having ViewRenderer take the golden directory in its constructor?
On 2016/07/18 16:41:44, Bernhard Bauer wrote: > https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... > File > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java > (right): > > https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... > chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:70: > * recommended way of using the ViewRenderer is to call multiple checks and then > call > On 2016/07/18 15:59:26, jbudorick wrote: > > I don't think this is a good idea -- it trades expediency of regenerating > > expectations for clarity of error messaging. This really wants something like > > gtest's EXPECT_* macros, but I don't know of something comparable in junit. > > Yeah, EXPECT_EQ was what I was thinking of here when I suggested that. > > > I can see why you'd be interested in running everything, but we need traces on > > each failure that point to the exact failure (rather than to the > > assertAllChecksPassed call site). > > We should be able to do this, right? We could just create a new exception and > then store it rather than throwing it (and then throw an error at the end, with > all the exceptions). I think we could, and I guess I could be ok with it. I'm worried about needing a separate function call after the assertions/checks for failures to show up, though.
https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:33: private static final String GOLDEN_FOLDER = "android/render_tests"; On 2016/07/18 15:59:26, jbudorick wrote: > Ideally, these would not be hard-coded. Where should they go? I can see having subdirectories being provided to the ViewRenderer be useful (so each test can store its image in its own directory), but the '/chrome/test/data/android/golden' should be the same for every test. https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:70: * recommended way of using the ViewRenderer is to call multiple checks and then call On 2016/07/18 16:41:44, Bernhard Bauer wrote: > On 2016/07/18 15:59:26, jbudorick wrote: > > I don't think this is a good idea -- it trades expediency of regenerating > > expectations for clarity of error messaging. This really wants something like > > gtest's EXPECT_* macros, but I don't know of something comparable in junit. > > Yeah, EXPECT_EQ was what I was thinking of here when I suggested that. > > > I can see why you'd be interested in running everything, but we need traces on > > each failure that point to the exact failure (rather than to the > > assertAllChecksPassed call site). > > We should be able to do this, right? We could just create a new exception and > then store it rather than throwing it (and then throw an error at the end, with > all the exceptions). The primary usecase that I'm worried about is there being a lot of checks in each render test, the dev changing the UI and then having to repeat the following loop multiple times: - Run Render tests - Render tests throw on the first mismatch - Dev has to update the goldens to get to the next failure Bernhard's solution would prevent this, but that would keep any assertions in the 'assertAllChecksPassed' method. Though it does improve the error reporting. I do dislike the fact that 'check' itself doesn't cause a failure and so the dev needs to remember to call 'assertAllChecksPassed'. https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:81: public boolean check(View view, String id) throws IOException { On 2016/07/18 15:59:26, jbudorick wrote: > Instead of being a function that returns a boolean (making it suitable for > assertTrue), WDYT about having this be an assertion? "assertViewMatches(view, > image)" or some such. This would allow you to have more descriptive error > messaging. I had only kept the boolean return value for this function in case anyone wanted to do some conditional execution - only perform test B if test A succeeds. https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:151: private static String imageName(Activity activity, String testClass, String desc) { On 2016/07/18 15:59:26, jbudorick wrote: > I think that, between this and createPath, this class knows more than it should > (and hard-codes more than it should). I thought it would be better to have a single hard-coded image name format than ending up with different naming formats being used all over the place. The implementation is very tightly tied to the name format so I didn't extract a format string.
Just some drive-by comments - not adding myself as a formal reviewer. https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:111: * @param newTabPageLayout the layout encapsulating all the above-the-fold elements nit: update this param name to aboveTheFoldView https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:113: * @param snippetsBridge the bridge to interact with the snippets service. nit: update this param name to snippetsSource https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:53: private final SnippetsSource mSnippetsBridge; nit: update this member name to mSnippetsSource https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:74: SnippetsSource snippetsBridge) { nit: update this param name to snippetsSource https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsSource.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsSource.java:36: public void fetchSnippetImage(SnippetArticle snippet, Callback<Bitmap> callback); Ah sorry for the churn, SnippetArticle has been renamed to SnippetArticleListItem for consistency with all the other NewTabPageListItem subclasses.
PTAL - I've made less stuff hardcoded and simplified the ViewRenderer doing away with the whole 'assertAllChecksPassed' by using the --regenerate-goldens flag. https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:111: * @param newTabPageLayout the layout encapsulating all the above-the-fold elements On 2016/07/20 16:44:10, Michael van Ouwerkerk wrote: > nit: update this param name to aboveTheFoldView Done. https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:113: * @param snippetsBridge the bridge to interact with the snippets service. On 2016/07/20 16:44:10, Michael van Ouwerkerk wrote: > nit: update this param name to snippetsSource Done. https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:53: private final SnippetsSource mSnippetsBridge; On 2016/07/20 16:44:10, Michael van Ouwerkerk wrote: > nit: update this member name to mSnippetsSource Done. https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:74: SnippetsSource snippetsBridge) { On 2016/07/20 16:44:10, Michael van Ouwerkerk wrote: > nit: update this param name to snippetsSource Done. https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsSource.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsSource.java:36: public void fetchSnippetImage(SnippetArticle snippet, Callback<Bitmap> callback); On 2016/07/20 16:44:10, Michael van Ouwerkerk wrote: > Ah sorry for the churn, SnippetArticle has been renamed to > SnippetArticleListItem for consistency with all the other NewTabPageListItem > subclasses. Done. https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:173: File path = new File(UrlUtils.getTestFilePath(folder)); On 2016/07/18 15:59:26, jbudorick wrote: > Please don't add new uses of getTestFilePath > (https://codesearch.chromium.org/chromium/src/base/test/android/javatests/src/...); > prefer getIsolatedTestFilePath. You'll need to use //chrome/test/data > explicitly. Done. https://codereview.chromium.org/2114963002/diff/120001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/120001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:33: private static final String GOLDEN_FOLDER = "android/render_tests"; On 2016/07/20 15:14:46, jbudorick wrote: > w.r.t. not hard-coding this -- what about having ViewRenderer take the golden > directory in its constructor? Done.
https://codereview.chromium.org/2114963002/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/2114963002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:71: ViewRenderer viewRenderer = new ViewRenderer(getActivity(), I like this significantly more than the previous version. https://codereview.chromium.org/2114963002/diff/140001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/140001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:84: * device where goldens are expected to be present, otherwise we fail silently. I wouldn't categorize this as a silent failure so much as a skip. In fact, unless there's some reason to partially run these on unsupported devices, you could hook into the skip mechanism by adding a RenderTestSkipCheck (or similar) to ChromeInstrumentationTestRunner that skips the test entirely if onRenderTestDevice() is false. https://codereview.chromium.org/2114963002/diff/140001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:92: public void check(View view, String id) throws IOException { I would name this more descriptively than just "check"
https://codereview.chromium.org/2114963002/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java (right): https://codereview.chromium.org/2114963002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java:71: ViewRenderer viewRenderer = new ViewRenderer(getActivity(), On 2016/08/16 00:45:29, jbudorick wrote: > I like this significantly more than the previous version. Acknowledged. https://codereview.chromium.org/2114963002/diff/140001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/140001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:84: * device where goldens are expected to be present, otherwise we fail silently. On 2016/08/16 00:45:29, jbudorick wrote: > I wouldn't categorize this as a silent failure so much as a skip. In fact, > unless there's some reason to partially run these on unsupported devices, you > could hook into the skip mechanism by adding a RenderTestSkipCheck (or similar) > to ChromeInstrumentationTestRunner that skips the test entirely if > onRenderTestDevice() is false. We may want to run this on unsupported devices for developers working on them. The workflow would be as follows: 1. Developer runs tests on trunk on unsupported device with --regenerate-goldens. 2. Developer pulls the goldens to src/. 3. Developer runs tests on feature branch (without --regenerate-goldens). 4. Because the goldens are found |result| is either MATCH or MISMATCH, giving feedback. (I'll reword the comment to not indicate silent failure)
https://codereview.chromium.org/2114963002/diff/140001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/140001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:92: public void check(View view, String id) throws IOException { On 2016/08/16 00:45:29, jbudorick wrote: > I would name this more descriptively than just "check" How about 'assertEquals'?
Looking good! https://codereview.chromium.org/2114963002/diff/160001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:116: Log.i(TAG, "%s - generated image saved to %s.", id, failureFile.getAbsolutePath()); I would maybe return here as well.
generally fine with this https://codereview.chromium.org/2114963002/diff/140001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/140001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:84: * device where goldens are expected to be present, otherwise we fail silently. On 2016/08/16 15:33:54, PEConn wrote: > On 2016/08/16 00:45:29, jbudorick wrote: > > I wouldn't categorize this as a silent failure so much as a skip. In fact, > > unless there's some reason to partially run these on unsupported devices, you > > could hook into the skip mechanism by adding a RenderTestSkipCheck (or > similar) > > to ChromeInstrumentationTestRunner that skips the test entirely if > > onRenderTestDevice() is false. > > We may want to run this on unsupported devices for developers working on them. > The workflow would be as follows: > > 1. Developer runs tests on trunk on unsupported device with > --regenerate-goldens. > 2. Developer pulls the goldens to src/. > 3. Developer runs tests on feature branch (without --regenerate-goldens). > 4. Because the goldens are found |result| is either MATCH or MISMATCH, giving > feedback. > > (I'll reword the comment to not indicate silent failure) Hm, ok, sgtm. https://codereview.chromium.org/2114963002/diff/160001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:92: public void assertEquals(View view, String id) throws IOException { nit: I'm worried that "assertEquals" (like "check") doesn't convey that this is multipurpose; it fails in normal operation, but it also can regenerate goldens if we're instead running in that mode. You've made that clear in the comment (which is good), but it should also be evident from the name.
https://codereview.chromium.org/2114963002/diff/160001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java (right): https://codereview.chromium.org/2114963002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:92: public void assertEquals(View view, String id) throws IOException { On 2016/08/17 16:55:16, jbudorick wrote: > nit: I'm worried that "assertEquals" (like "check") doesn't convey that this is > multipurpose; it fails in normal operation, but it also can regenerate goldens > if we're instead running in that mode. You've made that clear in the comment > (which is good), but it should also be evident from the name. Done. https://codereview.chromium.org/2114963002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/RenderUtils.java:116: Log.i(TAG, "%s - generated image saved to %s.", id, failureFile.getAbsolutePath()); On 2016/08/17 16:45:10, Bernhard Bauer wrote: > I would maybe return here as well. Done.
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
LGTM if John is happy.
On 2016/08/17 20:25:51, Bernhard Bauer wrote: > LGTM if John is happy. lgtm
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2114963002/#ps210001 (title: "Throw when mkdir fails.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== Create Java infrastructure for UI Render Tests. Creates the first part of the infrastructure that will allow us to test for any regressions to the Chrome on Android UI. This includes utilities to save Views to file, an annotation (RenderTest) to identify the instrumentation tests that use this and two examples of their use. The aim is to follow this up with host side code that grabs the images after the tests have been run and performs pixel comparisons on them against golden images stored in the repository. BUG=614305 ========== to ========== Create Java infrastructure for UI Render Tests. Creates the first part of the infrastructure that will allow us to test for any regressions to the Chrome on Android UI. This includes utilities to save Views to file, an annotation (RenderTest) to identify the instrumentation tests that use this and two examples of their use. The aim is to follow this up with host side code that grabs the images after the tests have been run and performs pixel comparisons on them against golden images stored in the repository. BUG=614305 Committed: https://crrev.com/324e06c25c137f097e6f001b12abb54d3e8bf268 Cr-Commit-Position: refs/heads/master@{#412905} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/324e06c25c137f097e6f001b12abb54d3e8bf268 Cr-Commit-Position: refs/heads/master@{#412905} |
