Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1981043004:
Added the --screenshot option. (Closed)
|
Created:
4 years, 7 months ago by BigBossZhiling Modified:
4 years, 7 months ago CC:
chromium-reviews, jbudorick+watch_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. |
DescriptionAdded the --screenshot option.
--screenshot option was broken for instrumentation tests. In this cl,
the option is enabled. With argument --screenshot, the device will take
a screenshot whenever an instrumentation test fails.
BUG=611538
Committed: https://crrev.com/09891bc02742f34a11a6d8ffd8993066f6c1b62c
Cr-Commit-Position: refs/heads/master@{#394843}
Patch Set 1 #
Total comments: 10
Patch Set 2 : added directory #
Total comments: 4
Patch Set 3 : made screenshot and screenshot-dir both work #
Total comments: 4
Patch Set 4 : changed file name #
Total comments: 4
Patch Set 5 : changed file name format #
Total comments: 4
Patch Set 6 : fixed get time stamp #
Total comments: 4
Patch Set 7 : corrected format #
Messages
Total messages: 26 (6 generated)
Description was changed from ========== Added the --screenshot option. --screenshot option was broken for instrumentation tests. In this cl, the option is enabled. With argument --screenshot, the device will take a screenshot whenever an instrumentation test fails. BUG=611538 ========== to ========== Added the --screenshot option. --screenshot option was broken for instrumentation tests. In this cl, the option is enabled. With argument --screenshot, the device will take a screenshot whenever an instrumentation test fails. BUG=611538 ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
Please take a look at this cl.
On 2016/05/16 23:32:28, BigBossZhiling wrote: > Please take a look at this cl. For example, if we call the command like this: 'out-gn/Debug/bin/run_chrome_public_test_apk --screenshot', screenshot should be taken when failure.
2 questions. But looks good otherwise https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:267: if self._test_instance.screenshot_failures: I can't imagine this takes a screenshot before Chrome closes after the test failure. Will these screenshots be mostly of the Android homescreen https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: device.TakeScreenshot() This should probably specify the host_path arg for TakeScreenshot even if it is just something like out/failure_screenshots/. Or maybe change the arg from screenshot_failures to screenshot_failures_dir?
https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:390: self._screenshot_failures = args.screenshot_failures nit: alphabetize https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:457: def screenshot_failures(self): nit: alphabetize https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: device.TakeScreenshot() On 2016/05/16 23:40:50, mikecase wrote: > This should probably specify the host_path arg for TakeScreenshot even if it is > just something like > out/failure_screenshots/. > Or maybe change the arg from screenshot_failures to screenshot_failures_dir? +1, we should be able to specify this.
https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:390: self._screenshot_failures = args.screenshot_failures On 2016/05/17 00:01:16, jbudorick wrote: > nit: alphabetize Done. https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:457: def screenshot_failures(self): On 2016/05/17 00:01:16, jbudorick wrote: > nit: alphabetize Done. https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:267: if self._test_instance.screenshot_failures: On 2016/05/16 23:40:50, mikecase wrote: > I can't imagine this takes a screenshot before Chrome closes after the test > failure. Will these screenshots be mostly of the Android homescreen You are right. lol. It is only useful in a few occasions, but taking a screenshot in java is not trivial, will upgrade the feature in another cl. https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: device.TakeScreenshot() On 2016/05/16 23:40:50, mikecase wrote: > This should probably specify the host_path arg for TakeScreenshot even if it is > just something like > out/failure_screenshots/. > Or maybe change the arg from screenshot_failures to screenshot_failures_dir? Done. https://codereview.chromium.org/1981043004/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: device.TakeScreenshot() On 2016/05/16 23:40:50, mikecase wrote: > This should probably specify the host_path arg for TakeScreenshot even if it is > just something like > out/failure_screenshots/. > Or maybe change the arg from screenshot_failures to screenshot_failures_dir? Done.
https://codereview.chromium.org/1981043004/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: device.TakeScreenshot(self._test_instance.screenshot_dir) TakeScreenshot returns the host path of the screenshot it took. We should log that at info level. https://codereview.chromium.org/1981043004/diff/20001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/1981043004/diff/20001/build/android/test_runn... build/android/test_runner.py:340: '--screenshot', dest='screenshot_dir', I think this should also accept --screenshot-dir (or --screenshot-directory), and we should work to deprecate --screenshot.
https://codereview.chromium.org/1981043004/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: device.TakeScreenshot(self._test_instance.screenshot_dir) On 2016/05/18 17:52:21, jbudorick wrote: > TakeScreenshot returns the host path of the screenshot it took. We should log > that at info level. Done. https://codereview.chromium.org/1981043004/diff/20001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/1981043004/diff/20001/build/android/test_runn... build/android/test_runner.py:340: '--screenshot', dest='screenshot_dir', On 2016/05/18 17:52:21, jbudorick wrote: > I think this should also accept --screenshot-dir (or --screenshot-directory), > and we should work to deprecate --screenshot. Done.
https://codereview.chromium.org/1981043004/diff/40001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:267: saved_dir = device.TakeScreenshot(self._test_instance.screenshot_dir) Thought about this a bit more. Let's instead specify the full path here -- directory & file name -- and include the both the name of the first failed test in results and the timestamp in the file name. https://codereview.chromium.org/1981043004/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: logging.info('saved screenshot at %s', saved_dir) nit: this should be logging.info('Saved screenshot for %s to %s', <test name>, <file path>)
https://codereview.chromium.org/1981043004/diff/40001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:267: saved_dir = device.TakeScreenshot(self._test_instance.screenshot_dir) On 2016/05/18 19:31:17, jbudorick wrote: > Thought about this a bit more. Let's instead specify the full path here -- > directory & file name -- and include the both the name of the first failed test > in results and the timestamp in the file name. Done. https://codereview.chromium.org/1981043004/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: logging.info('saved screenshot at %s', saved_dir) On 2016/05/18 19:31:17, jbudorick wrote: > nit: this should be > > logging.info('Saved screenshot for %s to %s', <test name>, <file path>) Done.
https://codereview.chromium.org/1981043004/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: file_name = test_display_name + str(time_ms()) + '.png' file_name = '%s%s.png' % (test_display_name, time) also, let's keep a similar time format as the existing generated file name in TakeScreenshot https://codereview.chromium.org/1981043004/diff/60001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/1981043004/diff/60001/build/android/test_runn... build/android/test_runner.py:340: '--screenshot', '--screenshot-dir', dest='screenshot_dir', sorry to flip-flop on this. let's do --screenshot-directory
https://codereview.chromium.org/1981043004/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: file_name = test_display_name + str(time_ms()) + '.png' On 2016/05/18 23:34:42, jbudorick wrote: > file_name = '%s%s.png' % (test_display_name, time) > > also, let's keep a similar time format as the existing generated file name in > TakeScreenshot Done. https://codereview.chromium.org/1981043004/diff/60001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/1981043004/diff/60001/build/android/test_runn... build/android/test_runner.py:340: '--screenshot', '--screenshot-dir', dest='screenshot_dir', On 2016/05/18 23:34:42, jbudorick wrote: > sorry to flip-flop on this. let's do --screenshot-directory Done. https://codereview.chromium.org/1981043004/diff/80001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:12: from devil.android.device_utils import _GetTimeStamp If I just do, from devil.android import device_utils, and then call it by 'device_utils._GetTimeStamp()', presubmit checks will fail, saying calling protected method blah blah blah.
https://codereview.chromium.org/1981043004/diff/80001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:12: from devil.android.device_utils import _GetTimeStamp On 2016/05/19 at 00:43:10, BigBossZhiling wrote: > If I just do, from devil.android import device_utils, and then call it by 'device_utils._GetTimeStamp()', presubmit checks will fail, saying calling protected method blah blah blah. You should change the name to GetTimeStamp if you are going to use that function. The underscore is supposed to indicate that it is a private function. But tbh, that GetTimeStamp function probably shouldnt be a public api in device_utils since it doesnt get the time from the device or anything related to teh device. You probably should move it somewhere in devil/utils/ when making it public. After you change the name you should change the import to just from devil.android import device_utils
https://codereview.chromium.org/1981043004/diff/80001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:12: from devil.android.device_utils import _GetTimeStamp On 2016/05/19 00:49:27, mikecase wrote: > On 2016/05/19 at 00:43:10, BigBossZhiling wrote: > > If I just do, from devil.android import device_utils, and then call it by > 'device_utils._GetTimeStamp()', presubmit checks will fail, saying calling > protected method blah blah blah. > > You should change the name to GetTimeStamp if you are going to use that > function. > The underscore is supposed to indicate that it is a private function. > > But tbh, that GetTimeStamp function probably shouldnt be a public api in > device_utils since it doesnt get the time from the device or anything related to > teh device. You probably should move it somewhere in devil/utils/ when > making it public. > > After you change the name you should change the import to just > from devil.android import device_utils No, don't do this, and don't use _GetTimeStamp. Just do something similar to what it does w/ strftime
https://codereview.chromium.org/1981043004/diff/80001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:12: from devil.android.device_utils import _GetTimeStamp On 2016/05/19 18:11:42, jbudorick wrote: > On 2016/05/19 00:49:27, mikecase wrote: > > On 2016/05/19 at 00:43:10, BigBossZhiling wrote: > > > If I just do, from devil.android import device_utils, and then call it by > > 'device_utils._GetTimeStamp()', presubmit checks will fail, saying calling > > protected method blah blah blah. > > > > You should change the name to GetTimeStamp if you are going to use that > > function. > > The underscore is supposed to indicate that it is a private function. > > > > But tbh, that GetTimeStamp function probably shouldnt be a public api in > > device_utils since it doesnt get the time from the device or anything related > to > > teh device. You probably should move it somewhere in devil/utils/ when > > making it public. > > > > After you change the name you should change the import to just > > from devil.android import device_utils > > No, don't do this, and don't use _GetTimeStamp. Just do something similar to > what it does w/ strftime Done.
lgtm w/ nits https://codereview.chromium.org/1981043004/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1981043004/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: file_name = '%s-%s.png' % (test_display_name, nit: drop this onto its own line + four space indent https://codereview.chromium.org/1981043004/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:271: os.path.join(self._test_instance.screenshot_dir, file_name)) nit: four space indent https://codereview.chromium.org/1981043004/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:272: logging.info('Saved screenshot for %s to %s.', nit: drop this onto its own line https://codereview.chromium.org/1981043004/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:273: test_display_name, saved_dir) nit: four space indent
The CQ bit was checked by hzl@google.com
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/1981043004/#ps120001 (title: "corrected format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1981043004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1981043004/120001
Message was sent while issue was closed.
Description was changed from ========== Added the --screenshot option. --screenshot option was broken for instrumentation tests. In this cl, the option is enabled. With argument --screenshot, the device will take a screenshot whenever an instrumentation test fails. BUG=611538 ========== to ========== Added the --screenshot option. --screenshot option was broken for instrumentation tests. In this cl, the option is enabled. With argument --screenshot, the device will take a screenshot whenever an instrumentation test fails. BUG=611538 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Added the --screenshot option. --screenshot option was broken for instrumentation tests. In this cl, the option is enabled. With argument --screenshot, the device will take a screenshot whenever an instrumentation test fails. BUG=611538 ========== to ========== Added the --screenshot option. --screenshot option was broken for instrumentation tests. In this cl, the option is enabled. With argument --screenshot, the device will take a screenshot whenever an instrumentation test fails. BUG=611538 Committed: https://crrev.com/09891bc02742f34a11a6d8ffd8993066f6c1b62c Cr-Commit-Position: refs/heads/master@{#394843} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/09891bc02742f34a11a6d8ffd8993066f6c1b62c Cr-Commit-Position: refs/heads/master@{#394843}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1998903002/ by jbudorick@chromium.org. The reason for reverting is: breaks downstream. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
