Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2822573002:
Fix android_action_runner.InputSwipe and InputText (Closed)
DescriptionFix android_action_runner.InputSwipe and InputText
Fix typo in android_action_runner.InputSwipe.
The input text shell command does not accept spaces. So, split it and
run the command multiple times.
Add tests for android_action_runner.
BUG=chromium:708300, chromium:712590
Review-Url: https://codereview.chromium.org/2822573002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/6858510487e62fed33e685dc9b715a5c3e14618b
Patch Set 1 #
Total comments: 1
Patch Set 2 : remove devil/ change and add swipe test. #
Total comments: 6
Patch Set 3 : More fixes. #
Total comments: 8
Patch Set 4 : Fixes. #
Total comments: 2
Patch Set 5 : nit. #
Depends on Patchset: Messages
Total messages: 46 (23 generated)
The CQ bit was checked by ssid@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...
ssid@chromium.org changed reviewers: + nednguyen@google.com
+Ned ptal Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
Description was changed from ========== Fix device_utils.ForceStop and android_action_runner.Swipe The 'ps -e' call is similar to change in crrev.com/2680693003 which replaces ps to 'ps -e' for N_MR1. Also fix typo in android_action_runner.Swipe BUG=chromium:708300, chromium:686716 ========== to ========== Fix device_utils.ForceStop and android_action_runner.Swipe The 'ps -e' call is similar to change in crrev.com/2680693003 which replaces ps to 'ps -e' for N_MR1. Also fix typo in android_action_runner.Swipe BUG=chromium:708300, chromium:686716 ==========
nednguyen@google.com changed reviewers: + jbudorick@chromium.org
On 2017/04/13 20:39:48, ssid wrote: > +Ned ptal Thanks! +JOhn for devil/ owner
https://codereview.chromium.org/2822573002/diff/1/telemetry/telemetry/core/an... File telemetry/telemetry/core/android_action_runner.py (right): https://codereview.chromium.org/2822573002/diff/1/telemetry/telemetry/core/an... telemetry/telemetry/core/android_action_runner.py:97: top_end_coord, duration): Can you add smoke test coverage for this? Probably similar to tests in https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
Your change is failing to apply on the trybots because mikecase landed a similar fix yesterday: https://codereview.chromium.org/2815573003/
On 2017/04/13 21:04:51, jbudorick wrote: > Your change is failing to apply on the trybots because mikecase landed a similar > fix yesterday: https://codereview.chromium.org/2815573003/ Yes, just found this change! Thanks, will revert the devil/ changes.
Description was changed from ========== Fix device_utils.ForceStop and android_action_runner.Swipe The 'ps -e' call is similar to change in crrev.com/2680693003 which replaces ps to 'ps -e' for N_MR1. Also fix typo in android_action_runner.Swipe BUG=chromium:708300, chromium:686716 ========== to ========== Fix android_action_runner.InputSwipe Fix typo in android_action_runner.InputSwipe and add test BUG=chromium:708300 ==========
ssid@chromium.org changed reviewers: - jbudorick@chromium.org
Ned, ptal Thanks! -John
https://codereview.chromium.org/2822573002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/action_runner_unittest.py (right): https://codereview.chromium.org/2822573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner_unittest.py:402: @decorators.Enabled('android') This should be moved to android_action_runner_unittest.py
https://codereview.chromium.org/2822573002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/action_runner_unittest.py (right): https://codereview.chromium.org/2822573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner_unittest.py:402: @decorators.Enabled('android') On 2017/04/14 00:24:39, nednguyen wrote: > This should be moved to android_action_runner_unittest.py Also I don't see test coverage for android_action_runner.InputSwipe method? https://codereview.chromium.org/2822573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner_unittest.py:421: print action_runner.EvaluateJavaScript('window.scrollY') Remove debug print
Description was changed from ========== Fix android_action_runner.InputSwipe Fix typo in android_action_runner.InputSwipe and add test BUG=chromium:708300 ========== to ========== Fix android_action_runner.InputSwipe and InputText Fix typo in android_action_runner.InputSwipe. The input text shell command does not accept spaces. So, split it and run the command multiple times. Add tests for android_action_runner. BUG=chromium:708300 ==========
Patchset #3 (id:40001) has been deleted
Added some more fixed to android_action_runner and moved the tests. wdyt? https://codereview.chromium.org/2822573002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/action_runner_unittest.py (right): https://codereview.chromium.org/2822573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner_unittest.py:402: @decorators.Enabled('android') On 2017/04/14 00:29:08, nednguyen wrote: > On 2017/04/14 00:24:39, nednguyen wrote: > > This should be moved to android_action_runner_unittest.py > > Also I don't see test coverage for android_action_runner.InputSwipe method? Actually scrollby uses this in background. But added an extra test. https://codereview.chromium.org/2822573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner_unittest.py:402: @decorators.Enabled('android') On 2017/04/14 00:24:39, nednguyen wrote: > This should be moved to android_action_runner_unittest.py Added a new file. https://codereview.chromium.org/2822573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner_unittest.py:421: print action_runner.EvaluateJavaScript('window.scrollY') On 2017/04/14 00:29:08, nednguyen wrote: > Remove debug print Done.
Added some more fixed to android_action_runner and moved the tests. wdyt?
nednguyen@google.com changed reviewers: + rnephew@chromium.org
On 2017/04/25 23:58:27, ssid wrote: > Added some more fixed to android_action_runner and moved the tests. wdyt? +Randy: can you take a pass at reviewing this?
https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... File telemetry/telemetry/core/android_action_runner.py (right): https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner.py:73: # Spaces should be input as keyevents. Pass space character to account for Can this we slightly reworded? I dont think its currently clear that we have to do this because input text 'words_here' does not accept spaces as an input. Also, is it that it can't accept spaces as input or are we improperly formatting the string/list_of_strings being sent to RunShellCommand? This causes multiple back and forth messages with the devices via adb. We try to minimize these as much as possible because each time we do it it makes it less realistic and also increases friction points during runs. Just trying to get up to speed on the problem.
https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... File telemetry/telemetry/core/android_action_runner.py (right): https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner.py:73: # Spaces should be input as keyevents. Pass space character to account for On 2017/04/26 00:28:25, rnephew (Reviews Here) wrote: > Can this we slightly reworded? I dont think its currently clear that we have to > do this because input text 'words_here' does not accept spaces as an input. > > Also, is it that it can't accept spaces as input or are we improperly formatting > the string/list_of_strings being sent to RunShellCommand? This causes multiple > back and forth messages with the devices via adb. We try to minimize these as > much as possible because each time we do it it makes it less realistic and also > increases friction points during runs. > > Just trying to get up to speed on the problem. Sorry for not being clear. The input text command does not accept spaces as input. On adding spaces to the input the command fails. This is irrespective of the formatting. But, this used to work on Android O and some builds of N with spaces. That is why the benchmark was passing on the cq. But, we still have bots on the older version which are failing. Maybe a better solution would be to check for version here? I did not make a version check here because I can't tell which version the space input was fixed.
lgtm w/ a nit and a few comments. https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... File telemetry/telemetry/core/android_action_runner.py (right): https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner.py:73: # Spaces should be input as keyevents. Pass space character to account for On 2017/04/26 00:36:46, ssid wrote: > On 2017/04/26 00:28:25, rnephew (Reviews Here) wrote: > > Can this we slightly reworded? I dont think its currently clear that we have > to > > do this because input text 'words_here' does not accept spaces as an input. > > > > Also, is it that it can't accept spaces as input or are we improperly > formatting > > the string/list_of_strings being sent to RunShellCommand? This causes multiple > > back and forth messages with the devices via adb. We try to minimize these as > > much as possible because each time we do it it makes it less realistic and > also > > increases friction points during runs. > > > > Just trying to get up to speed on the problem. > > Sorry for not being clear. The input text command does not accept spaces as > input. On adding spaces to the input the command fails. This is irrespective of > the formatting. > > But, this used to work on Android O and some builds of N with spaces. That is > why the benchmark was passing on the cq. But, we still have bots on the older > version which are failing. Maybe a better solution would be to check for version > here? I did not make a version check here because I can't tell which version the > space input was fixed. I dont think the added code complexity for gating on version will be necessary. Unless you foresee us sending a lot of long sentences with many spaces in it via this command. We should just keep an eye out and if it causes regressions (seems highly highly unlikely) investigate further. I mostly just wanted to make we are fully thinking out the implications of changing InputText to work like this. Maybe just making the comment a little more concrete like: # Space must be input as key events since 'input text <text>' does not work with space. Pass space.... https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... File telemetry/telemetry/core/android_action_runner_unittest.py (right): https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner_unittest.py:4: from telemetry import decorators nit: line between copyright and imports. https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner_unittest.py:68: action_runner.PressKey('Home') # |That is boring. Is this part realistic? Are Home and End keys really used on android? Guess its moot as long as those keys work across all devices and os versions.
On 2017/04/26 01:01:26, rnephew (Reviews Here) wrote: > lgtm w/ a nit and a few comments. > > https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... > File telemetry/telemetry/core/android_action_runner.py (right): > > https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... > telemetry/telemetry/core/android_action_runner.py:73: # Spaces should be input > as keyevents. Pass space character to account for > On 2017/04/26 00:36:46, ssid wrote: > > On 2017/04/26 00:28:25, rnephew (Reviews Here) wrote: > > > Can this we slightly reworded? I dont think its currently clear that we have > > to > > > do this because input text 'words_here' does not accept spaces as an input. > > > > > > Also, is it that it can't accept spaces as input or are we improperly > > formatting > > > the string/list_of_strings being sent to RunShellCommand? This causes > multiple > > > back and forth messages with the devices via adb. We try to minimize these > as > > > much as possible because each time we do it it makes it less realistic and > > also > > > increases friction points during runs. > > > > > > Just trying to get up to speed on the problem. > > > > Sorry for not being clear. The input text command does not accept spaces as > > input. On adding spaces to the input the command fails. This is irrespective > of > > the formatting. > > > > But, this used to work on Android O and some builds of N with spaces. That is > > why the benchmark was passing on the cq. But, we still have bots on the older > > version which are failing. Maybe a better solution would be to check for > version > > here? I did not make a version check here because I can't tell which version > the > > space input was fixed. > > I dont think the added code complexity for gating on version will be necessary. > Unless you foresee us sending a lot of long sentences with many spaces in it via > this command. We should just keep an eye out and if it causes regressions (seems > highly highly unlikely) investigate further. I mostly just wanted to make we are > fully thinking out the implications of changing InputText to work like this. > > Maybe just making the comment a little more concrete like: > # Space must be input as key events since 'input text <text>' does not work with > space. Pass space.... > > https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... > File telemetry/telemetry/core/android_action_runner_unittest.py (right): > > https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... > telemetry/telemetry/core/android_action_runner_unittest.py:4: from telemetry > import decorators > nit: line between copyright and imports. > > https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... > telemetry/telemetry/core/android_action_runner_unittest.py:68: > action_runner.PressKey('Home') # |That is boring. > Is this part realistic? Are Home and End keys really used on android? Guess its > moot as long as those keys work across all devices and os versions. non-owner lgtm*
Thanks. fixed. https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... File telemetry/telemetry/core/android_action_runner.py (right): https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner.py:73: # Spaces should be input as keyevents. Pass space character to account for On 2017/04/26 01:01:26, rnephew (Reviews Here) wrote: > On 2017/04/26 00:36:46, ssid wrote: > > On 2017/04/26 00:28:25, rnephew (Reviews Here) wrote: > > > Can this we slightly reworded? I dont think its currently clear that we have > > to > > > do this because input text 'words_here' does not accept spaces as an input. > > > > > > Also, is it that it can't accept spaces as input or are we improperly > > formatting > > > the string/list_of_strings being sent to RunShellCommand? This causes > multiple > > > back and forth messages with the devices via adb. We try to minimize these > as > > > much as possible because each time we do it it makes it less realistic and > > also > > > increases friction points during runs. > > > > > > Just trying to get up to speed on the problem. > > > > Sorry for not being clear. The input text command does not accept spaces as > > input. On adding spaces to the input the command fails. This is irrespective > of > > the formatting. > > > > But, this used to work on Android O and some builds of N with spaces. That is > > why the benchmark was passing on the cq. But, we still have bots on the older > > version which are failing. Maybe a better solution would be to check for > version > > here? I did not make a version check here because I can't tell which version > the > > space input was fixed. > > I dont think the added code complexity for gating on version will be necessary. > Unless you foresee us sending a lot of long sentences with many spaces in it via > this command. We should just keep an eye out and if it causes regressions (seems > highly highly unlikely) investigate further. I mostly just wanted to make we are > fully thinking out the implications of changing InputText to work like this. > > Maybe just making the comment a little more concrete like: > # Space must be input as key events since 'input text <text>' does not work with > space. Pass space.... Done. https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... File telemetry/telemetry/core/android_action_runner_unittest.py (right): https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner_unittest.py:4: from telemetry import decorators On 2017/04/26 01:01:26, rnephew (Reviews Here) wrote: > nit: line between copyright and imports. Done. https://codereview.chromium.org/2822573002/diff/60001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner_unittest.py:68: action_runner.PressKey('Home') # |That is boring. On 2017/04/26 01:01:26, rnephew (Reviews Here) wrote: > Is this part realistic? Are Home and End keys really used on android? Guess its > moot as long as those keys work across all devices and os versions. Actually not realistic. I was just trying to replicate the action_runner test. Removed it.
The CQ bit was checked by ssid@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: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
Great tests, thanks ssid! lgtm https://codereview.chromium.org/2822573002/diff/80001/telemetry/telemetry/cor... File telemetry/telemetry/core/android_action_runner_unittest.py (right): https://codereview.chromium.org/2822573002/diff/80001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner_unittest.py:77: nits: remove this blank line
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rnephew@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2822573002/#ps100001 (title: "nit.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2822573002/diff/80001/telemetry/telemetry/cor... File telemetry/telemetry/core/android_action_runner_unittest.py (right): https://codereview.chromium.org/2822573002/diff/80001/telemetry/telemetry/cor... telemetry/telemetry/core/android_action_runner_unittest.py:77: On 2017/04/26 14:59:56, nednguyen wrote: > nits: remove this blank line Done.
Description was changed from ========== Fix android_action_runner.InputSwipe and InputText Fix typo in android_action_runner.InputSwipe. The input text shell command does not accept spaces. So, split it and run the command multiple times. Add tests for android_action_runner. BUG=chromium:708300 ========== to ========== Fix android_action_runner.InputSwipe and InputText Fix typo in android_action_runner.InputSwipe. The input text shell command does not accept spaces. So, split it and run the command multiple times. Add tests for android_action_runner. BUG=chromium:708300, chromium:712590 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by ssid@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": 100001, "attempt_start_ts": 1493233360191820,
"parent_rev": "abff3b4929c74e4eb90a877520b82570853ac5c5", "commit_rev":
"6858510487e62fed33e685dc9b715a5c3e14618b"}
Message was sent while issue was closed.
Description was changed from ========== Fix android_action_runner.InputSwipe and InputText Fix typo in android_action_runner.InputSwipe. The input text shell command does not accept spaces. So, split it and run the command multiple times. Add tests for android_action_runner. BUG=chromium:708300, chromium:712590 ========== to ========== Fix android_action_runner.InputSwipe and InputText Fix typo in android_action_runner.InputSwipe. The input text shell command does not accept spaces. So, split it and run the command multiple times. Add tests for android_action_runner. BUG=chromium:708300, chromium:712590 Review-Url: https://codereview.chromium.org/2822573002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
On 2017/04/26 19:17:54, commit-bot: I haz the power wrote: > Committed patchset #5 (id:100001) as > https://chromium.googlesource.com/external/github.com/catapult-project/catapu... Nice catch, thanks ssid for the fix! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
