Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854823007:
Move screenshot capture to Java-side. (Closed)
|
Created:
3 years, 7 months ago by mikecase (-- gone --) Modified:
3 years, 7 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, danakj+watch_chromium.org, jbudorick+watch_chromium.org, mikecase+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove screenshot capture to Java-side.
Currently, the post-test screenshots are taken on the python after
the test has finished (and Activity destroyed). Moving the
screenshot capture into Android code so we can snap a pic before
Activity is gone.
Review-Url: https://codereview.chromium.org/2854823007
Cr-Commit-Position: refs/heads/master@{#470816}
Committed: https://chromium.googlesource.com/chromium/src/+/fc8efc725e151ffc2f2584b75d9c52fd16cf3721
Patch Set 1 #Patch Set 2 : Move screenshot capture to Java-side. #Patch Set 3 : Move screenshot capture to Java-side. #Patch Set 4 : Move screenshot capture to Java-side. #
Total comments: 15
Patch Set 5 : Move screenshot capture to Java-side. #
Total comments: 2
Patch Set 6 : Move screenshot capture to Java-side. #
Total comments: 6
Patch Set 7 : John's comments #Patch Set 8 : Move screenshot capture to Java-side. #
Total comments: 7
Patch Set 9 : Yoland's comments #
Messages
Total messages: 39 (16 generated)
mikecase@chromium.org changed reviewers: + jbudorick@chromium.org, yolandyan@chromium.org
Super excited about this. https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:164: return super.withAfters(method, test, base); nit: return super.withAfters(method, test, new ScreenshotOnFailureStatement(base)); ? https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java (right): https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:42: String subdir = CommandLine.isInitialized() This may be more readable/cleaner if you split the screenshot capture logic into a new function, have it return early in the failure conditions, and then just have this catch block be } catch (Throwable e) { takeScreenshot(); throw e; } https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:52: UiDevice uiDevice = UiDevice.getInstance(InstrumentationRegistry.getInstrumentation()); I assume this was the only way to do this? https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:53: File path = new File(UrlUtils.getIsolatedTestFilePath(subdir)); While I'm ok with using the isolated test file path for now, I'd like to eventually switch the test output directory to something other than what we use for the test input directory. To do so, we should create a function that introduces some indirection here -- calling it, which then calls UrlUtils.getIsolatedTestFilePath. https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:657: if args.gs_results_bucket: The logical connection here between gs_results_bucket and screenshot-dir is not obvious. https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:393: 'chromium_tests_root', If I don't like hard-coding things, I *really* don't like hard-coding things in multiple places. This depends on the implementation of getIsolatedTestFilePath referring to *exactly* <external storage>/chromium_tests_root *and* on ScreenshotOnFailureStatement concatenating the provided --screenshot-dir to that path. Can we reduce the number of places in which this is hard-coded? Maybe we can have the script tell the Statement exactly where it should write the screenshot? https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:403: device.RemovePath(screenshot_device_file) We should try to remove the file even if pulling it fails for some reason. Maybe a try/finally here? https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:413: bucket=self._test_instance.gs_results_bucket + '/screenshots') nit: '%s/screenshots' % self._test_instance.gs_results_bucket
https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:164: return super.withAfters(method, test, base); On 2017/05/05 at 01:21:58, jbudorick wrote: > nit: > > return super.withAfters(method, test, new ScreenshotOnFailureStatement(base)); > > ? Done! https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java (right): https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:42: String subdir = CommandLine.isInitialized() On 2017/05/05 at 01:21:58, jbudorick wrote: > This may be more readable/cleaner if you split the screenshot capture logic into a new function, have it return early in the failure conditions, and then just have this catch block be > > } catch (Throwable e) { > takeScreenshot(); > throw e; > } Done https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:52: UiDevice uiDevice = UiDevice.getInstance(InstrumentationRegistry.getInstrumentation()); On 2017/05/05 at 01:21:58, jbudorick wrote: > I assume this was the only way to do this? I looked at basically every other option. So there is some PixelCopy class used elsewhere... https://developer.android.com/reference/android/view/PixelCopy.html but you need an Activity context as far as I know to use it. Also only available in higher SDK levels. Basically it seems like UiAutomator has extra priviledges in Android framework, so its able to screencap when/where most things could not. I did not want to use UiAutomator here...but it seems like best way. https://codereview.chromium.org/2854823007/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:53: File path = new File(UrlUtils.getIsolatedTestFilePath(subdir)); On 2017/05/05 at 01:21:58, jbudorick wrote: > While I'm ok with using the isolated test file path for now, I'd like to eventually switch the test output directory to something other than what we use for the test input directory. To do so, we should create a function that introduces some indirection here -- calling it, which then calls UrlUtils.getIsolatedTestFilePath. Added UrlUtils.getTestFileOutputPath https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:657: if args.gs_results_bucket: On 2017/05/05 at 01:21:58, jbudorick wrote: > The logical connection here between gs_results_bucket and screenshot-dir is not obvious. nope! You are correct! https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:393: 'chromium_tests_root', On 2017/05/05 at 01:21:58, jbudorick wrote: > If I don't like hard-coding things, I *really* don't like hard-coding things in multiple places. > > This depends on the implementation of getIsolatedTestFilePath referring to *exactly* <external storage>/chromium_tests_root *and* on ScreenshotOnFailureStatement concatenating the provided --screenshot-dir to that path. Can we reduce the number of places in which this is hard-coded? Maybe we can have the script tell the Statement exactly where it should write the screenshot? Changed --screenshot-dir arg to just --screenshot-path so fewer things have to be hard-coded. Currently, other things depend on getIsolatedTestFilePath referring to exactly <external storage>/chromium_tests_root....at least I think so. Like, we push the data to the device to chromium_tests_root, and then use UrlUtils to find the test data. https://codereview.chromium.org/2854823007/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:403: device.RemovePath(screenshot_device_file) On 2017/05/05 at 01:21:58, jbudorick wrote: > We should try to remove the file even if pulling it fails for some reason. Maybe a try/finally here? sounds good. Done
https://codereview.chromium.org/2854823007/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2854823007/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:657: if args.gs_results_bucket: I think I actually have to change this to "args.gs_results_bucket or args.screenshot_dir" in case people want local screenshots
https://codereview.chromium.org/2854823007/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2854823007/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:657: if args.gs_results_bucket: On 2017/05/05 at 18:19:30, mikecase wrote: > I think I actually have to change this to "args.gs_results_bucket or args.screenshot_dir" in case people want local screenshots Fixed! And tested running with --screenshot-dir locally and it works very nicely.
friendly ping, would like to land this and have some other CLs that depend on it.
https://codereview.chromium.org/2854823007/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java (right): https://codereview.chromium.org/2854823007/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java:55: return getTestFileOutputRoot() + "/" + path; Should this check that path doesn't start with "/"? https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:658: self._flags.append('--screenshot-file=%s' % FAILURE_SCREENSHOT_FILE) 1) I'm concerned that we may not want to hard-code the screenshot file (maybe a DeviceTempFile?) and thus local_device_instrumentation_test_run may be a better place to handle this than the test instance. 2) Add a comment here explaining the relationship between gs_results_bucket and --screenshot-file https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:383: with contextlib_ext.Optional( At this point, perhaps we should pull the screenshot logic into its own function that gets called here.
CL is perfect now. PTAL https://codereview.chromium.org/2854823007/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java (right): https://codereview.chromium.org/2854823007/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java:55: return getTestFileOutputRoot() + "/" + path; On 2017/05/08 at 17:08:39, jbudorick wrote: > Should this check that path doesn't start with "/"? This is all gone now. So...fixed https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:658: self._flags.append('--screenshot-file=%s' % FAILURE_SCREENSHOT_FILE) On 2017/05/08 at 17:08:39, jbudorick wrote: > 1) I'm concerned that we may not want to hard-code the screenshot file (maybe a DeviceTempFile?) and thus local_device_instrumentation_test_run may be a better place to handle this than the test instance. > 2) Add a comment here explaining the relationship between gs_results_bucket and --screenshot-file Done, (1) moved it to same place that coverage-files are handled. (2) made screenshot file a DeviceTempFile. (3) Now FULL-path of screenshot is passed so not path construction is done on either side (removing all changes to UrlUtils.java) https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:383: with contextlib_ext.Optional( On 2017/05/08 at 17:08:39, jbudorick wrote: > At this point, perhaps we should pull the screenshot logic into its own function that gets called here. Done, moved screenshot logic into new function.
lgtm https://codereview.chromium.org/2854823007/diff/140001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2854823007/diff/140001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:264: screenshot_device_file = None optional nit: could do something here similar to what we do in local_device_gtest_run w/ a null context manager. I need to move that up to contextlib_ext, though, so I'm ok with you holding off & landing this.
On 2017/05/08 18:35:32, mikecase wrote: > CL is perfect now. PTAL :) > > https://codereview.chromium.org/2854823007/diff/100001/base/test/android/java... > File base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java > (right): > > https://codereview.chromium.org/2854823007/diff/100001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java:55: > return getTestFileOutputRoot() + "/" + path; > On 2017/05/08 at 17:08:39, jbudorick wrote: > > Should this check that path doesn't start with "/"? > > This is all gone now. So...fixed > > https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/in... > File build/android/pylib/instrumentation/instrumentation_test_instance.py > (right): > > https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/in... > build/android/pylib/instrumentation/instrumentation_test_instance.py:658: > self._flags.append('--screenshot-file=%s' % FAILURE_SCREENSHOT_FILE) > On 2017/05/08 at 17:08:39, jbudorick wrote: > > 1) I'm concerned that we may not want to hard-code the screenshot file (maybe > a DeviceTempFile?) and thus local_device_instrumentation_test_run may be a > better place to handle this than the test instance. > > 2) Add a comment here explaining the relationship between gs_results_bucket > and --screenshot-file > > Done, (1) moved it to same place that coverage-files are handled. (2) made > screenshot file a DeviceTempFile. (3) Now FULL-path of screenshot is passed so > not path construction is done on either side (removing all changes to > UrlUtils.java) > > https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/lo... > File build/android/pylib/local/device/local_device_instrumentation_test_run.py > (right): > > https://codereview.chromium.org/2854823007/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:383: > with contextlib_ext.Optional( > On 2017/05/08 at 17:08:39, jbudorick wrote: > > At this point, perhaps we should pull the screenshot logic into its own > function that gets called here. > > Done, moved screenshot logic into new function.
The CQ bit was checked by mikecase@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mikecase@chromium.org changed reviewers: + agrieve@chromium.org
+ agrieve@ for review of base/BUILD.gn
https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:163: return super.withAfters(method, test, new ScreenshotOnFailureStatement(base)); nit: would be great to add some comments here to note that screenshot on failure statement https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java (right): https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:37: takeScreenshot(); we should support AssumptionViolatedException here (ignore AssumptionViolatedException), though our runner currently does not recognized its result, but in the future we might. https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:43: String screenshotFilePath = CommandLine.isInitialized() hmm, there is potentially a better way to do this, by directly passing in argument to the java runner. I think communication to test apk should not go though commandline flags, the base runner might be used for a test apk that doesn't use commandline we can chat about that in person :)
Done, and retested. yoland please take another quick look. agrieve@, when you get the chance, please stamp for base/BUILD.gn if it looks good to you. ty! https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:163: return super.withAfters(method, test, new ScreenshotOnFailureStatement(base)); On 2017/05/09 at 15:23:28, the real yoland wrote: > nit: would be great to add some comments here to note that screenshot on failure statement Done https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java (right): https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:37: takeScreenshot(); On 2017/05/09 at 15:23:28, the real yoland wrote: > we should support AssumptionViolatedException here (ignore AssumptionViolatedException), though our runner currently does not recognized its result, but in the future we might. Ack https://codereview.chromium.org/2854823007/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/ScreenshotOnFailureStatement.java:43: String screenshotFilePath = CommandLine.isInitialized() On 2017/05/09 at 15:23:28, the real yoland wrote: > hmm, there is potentially a better way to do this, by directly passing in argument to the java runner. > > I think communication to test apk should not go though commandline flags, the base runner might be used for a test apk that doesn't use commandline > > we can chat about that in person :) Done! I agree. I have another CL for render tests where I use command-line flags. Can't switch that one to use Bundle arguments yet since RenderTests could theoretically be JUnit3 or 4. These screenshots will always be taken on 4.
lgtm
On 2017/05/10 18:10:32, the real yoland wrote: > lgtm base lgtm
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2854823007/#ps160001 (title: "Yoland's comments")
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_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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": 160001, "attempt_start_ts": 1494475807079440,
"parent_rev": "506c3df2dd4d83224aa15f8a99570d1eb4bf64df", "commit_rev":
"fc8efc725e151ffc2f2584b75d9c52fd16cf3721"}
Message was sent while issue was closed.
Description was changed from ========== Move screenshot capture to Java-side. Currently, the post-test screenshots are taken on the python after the test has finished (and Activity destroyed). Moving the screenshot capture into Android code so we can snap a pic before Activity is gone. ========== to ========== Move screenshot capture to Java-side. Currently, the post-test screenshots are taken on the python after the test has finished (and Activity destroyed). Moving the screenshot capture into Android code so we can snap a pic before Activity is gone. Review-Url: https://codereview.chromium.org/2854823007 Cr-Commit-Position: refs/heads/master@{#470816} Committed: https://chromium.googlesource.com/chromium/src/+/fc8efc725e151ffc2f2584b75d9c... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/fc8efc725e151ffc2f2584b75d9c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
