Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2743033002:
[android] Add support for providing flags to instrumentation tests directly. (Closed)
Description[android] Add support for providing flags to instrumentation tests directly.
--device-flags/--device-flags-file both previously supported passing
flags for instrumentation tests in a file. There was no instrumentation
tests support for providing such flags directly. This CL adds that
support by adding --test-arguments (matching gtests) and switching
--device-flags to be an alias of that.
BUG=700366
Patch Set 1 #
Total comments: 5
Messages
Total messages: 14 (3 generated)
jbudorick@chromium.org changed reviewers: + mikecase@chromium.org, shenghuazhang@chromium.org
https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.p... build/android/test_runner.py:370: '--test-arguments', Why are we adding another alias for this? also, --command-line-flags seems like a better name for this arg over --device-flags and --test-arguments.
https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.p... build/android/test_runner.py:370: '--test-arguments', On 2017/03/10 18:08:03, mikecase wrote: > Why are we adding another alias for this? > > also, --command-line-flags seems like a better name for this arg over > --device-flags and --test-arguments. --test-arguments is what it is for gtests, hence the alias. I suppose --command-line-flags/--command-line-flags-file would be a more sensible end state, though.
https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.p... build/android/test_runner.py:373: help='Command-line flags to set on the device') Since this is matching the argument in gtest, and the help comment for gtest is: help='Additional arguments to pass to the test.' which is different with it. Wonder if it can be unified s.t. knowing the support has the same meaning as gtest explicitly.
jam@chromium.org changed reviewers: + jam@chromium.org
https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.p... build/android/test_runner.py:370: '--test-arguments', On 2017/03/10 18:11:17, jbudorick wrote: > On 2017/03/10 18:08:03, mikecase wrote: > > Why are we adding another alias for this? > > > > also, --command-line-flags seems like a better name for this arg over > > --device-flags and --test-arguments. > > --test-arguments is what it is for gtests, hence the alias. I suppose > --command-line-flags/--command-line-flags-file would be a more sensible end > state, though. As a non-Android developer, having to specify command lines differently is confusing.
https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.p... build/android/test_runner.py:370: '--test-arguments', On 2017/03/10 19:20:58, jam wrote: > On 2017/03/10 18:11:17, jbudorick wrote: > > On 2017/03/10 18:08:03, mikecase wrote: > > > Why are we adding another alias for this? > > > > > > also, --command-line-flags seems like a better name for this arg over > > > --device-flags and --test-arguments. > > > > --test-arguments is what it is for gtests, hence the alias. I suppose > > --command-line-flags/--command-line-flags-file would be a more sensible end > > state, though. > > As a non-Android developer, having to specify command lines differently is > confusing. hmm, we might be able to pass arguments that the test runner doesn't recognize on to the device, though I'd consider that change to be a bit more risky. It'd conceivably let you do "args": [ "--enable-browser-side-navigation", ], in the JSON.
On 2017/03/10 19:38:44, jbudorick wrote: > https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.py > File build/android/test_runner.py (right): > > https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.p... > build/android/test_runner.py:370: '--test-arguments', > On 2017/03/10 19:20:58, jam wrote: > > On 2017/03/10 18:11:17, jbudorick wrote: > > > On 2017/03/10 18:08:03, mikecase wrote: > > > > Why are we adding another alias for this? > > > > > > > > also, --command-line-flags seems like a better name for this arg over > > > > --device-flags and --test-arguments. > > > > > > --test-arguments is what it is for gtests, hence the alias. I suppose > > > --command-line-flags/--command-line-flags-file would be a more sensible end > > > state, though. > > > > As a non-Android developer, having to specify command lines differently is > > confusing. > > hmm, we might be able to pass arguments that the test runner doesn't recognize > on to the device, though I'd consider that change to be a bit more risky. It'd > conceivably let you do > > "args": [ > "--enable-browser-side-navigation", > ], > > in the JSON. How is that different from today, where one can pass unknown arguments to gtests on android or on other platforms?
On 2017/03/10 21:56:50, jam wrote: > On 2017/03/10 19:38:44, jbudorick wrote: > > https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.py > > File build/android/test_runner.py (right): > > > > > https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.p... > > build/android/test_runner.py:370: '--test-arguments', > > On 2017/03/10 19:20:58, jam wrote: > > > On 2017/03/10 18:11:17, jbudorick wrote: > > > > On 2017/03/10 18:08:03, mikecase wrote: > > > > > Why are we adding another alias for this? > > > > > > > > > > also, --command-line-flags seems like a better name for this arg over > > > > > --device-flags and --test-arguments. > > > > > > > > --test-arguments is what it is for gtests, hence the alias. I suppose > > > > --command-line-flags/--command-line-flags-file would be a more sensible > end > > > > state, though. > > > > > > As a non-Android developer, having to specify command lines differently is > > > confusing. > > > > hmm, we might be able to pass arguments that the test runner doesn't recognize > > on to the device, though I'd consider that change to be a bit more risky. It'd > > conceivably let you do > > > > "args": [ > > "--enable-browser-side-navigation", > > ], > > > > in the JSON. > > How is that different from today, where one can pass unknown arguments to gtests > on android or on other platforms? My concerns would be: 1) devs trying to pass something to test_runner.py, making a mistake, and having it silently pass that down to the binary rather than the hard failure that it'd be currently. 2) flag overlap between the test runner and the binary, though I admittedly don't know of any specific instances of this and we can work around such cases relatively easily as they arise.
On 2017/03/10 22:14:46, jbudorick wrote: > On 2017/03/10 21:56:50, jam wrote: > > On 2017/03/10 19:38:44, jbudorick wrote: > > > > https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.py > > > File build/android/test_runner.py (right): > > > > > > > > > https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.p... > > > build/android/test_runner.py:370: '--test-arguments', > > > On 2017/03/10 19:20:58, jam wrote: > > > > On 2017/03/10 18:11:17, jbudorick wrote: > > > > > On 2017/03/10 18:08:03, mikecase wrote: > > > > > > Why are we adding another alias for this? > > > > > > > > > > > > also, --command-line-flags seems like a better name for this arg over > > > > > > --device-flags and --test-arguments. > > > > > > > > > > --test-arguments is what it is for gtests, hence the alias. I suppose > > > > > --command-line-flags/--command-line-flags-file would be a more sensible > > end > > > > > state, though. > > > > > > > > As a non-Android developer, having to specify command lines differently is > > > > confusing. > > > > > > hmm, we might be able to pass arguments that the test runner doesn't > recognize > > > on to the device, though I'd consider that change to be a bit more risky. > It'd > > > conceivably let you do > > > > > > "args": [ > > > "--enable-browser-side-navigation", > > > ], > > > > > > in the JSON. > > > > How is that different from today, where one can pass unknown arguments to > gtests > > on android or on other platforms? > > My concerns would be: > 1) devs trying to pass something to test_runner.py, making a mistake, and > having it silently pass that down to the binary rather than the hard failure > that it'd be currently. > 2) flag overlap between the test runner and the binary, though I admittedly > don't know of any specific instances of this and we can work around such cases > relatively easily as they arise. There's a small number of reviewers for these files right? We can use that to enforce that only known flags are passed?
On 2017/03/10 22:26:37, jam wrote: > On 2017/03/10 22:14:46, jbudorick wrote: > > On 2017/03/10 21:56:50, jam wrote: > > > On 2017/03/10 19:38:44, jbudorick wrote: > > > > > > https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.py > > > > File build/android/test_runner.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2743033002/diff/1/build/android/test_runner.p... > > > > build/android/test_runner.py:370: '--test-arguments', > > > > On 2017/03/10 19:20:58, jam wrote: > > > > > On 2017/03/10 18:11:17, jbudorick wrote: > > > > > > On 2017/03/10 18:08:03, mikecase wrote: > > > > > > > Why are we adding another alias for this? > > > > > > > > > > > > > > also, --command-line-flags seems like a better name for this arg > over > > > > > > > --device-flags and --test-arguments. > > > > > > > > > > > > --test-arguments is what it is for gtests, hence the alias. I suppose > > > > > > --command-line-flags/--command-line-flags-file would be a more > sensible > > > end > > > > > > state, though. > > > > > > > > > > As a non-Android developer, having to specify command lines differently > is > > > > > confusing. > > > > > > > > hmm, we might be able to pass arguments that the test runner doesn't > > recognize > > > > on to the device, though I'd consider that change to be a bit more risky. > > It'd > > > > conceivably let you do > > > > > > > > "args": [ > > > > "--enable-browser-side-navigation", > > > > ], > > > > > > > > in the JSON. > > > > > > How is that different from today, where one can pass unknown arguments to > > gtests > > > on android or on other platforms? > > > > My concerns would be: > > 1) devs trying to pass something to test_runner.py, making a mistake, and > > having it silently pass that down to the binary rather than the hard failure > > that it'd be currently. > > 2) flag overlap between the test runner and the binary, though I admittedly > > don't know of any specific instances of this and we can work around such cases > > relatively easily as they arise. > > There's a small number of reviewers for these files right? We can use that to > enforce that only known flags are passed? Yeah, probably <5 people, and I'm on the watchlist in the unusual event that I'm not on the reviewer list. I'll experiment with this and post a new patchset either later today or Monday.
Basically everything changed about this CL (including the upstream branch), so I'm moving it to another CL: https://codereview.chromium.org/2752493002/
Description was changed from ========== [android] Add support for providing flags to instrumentation tests directly. --device-flags/--device-flags-file both previously supported passing flags for instrumentation tests in a file. There was no instrumentation tests support for providing such flags directly. This CL adds that support by adding --test-arguments (matching gtests) and switching --device-flags to be an alias of that. BUG=700366 ========== to ========== [android] Add support for providing flags to instrumentation tests directly. --device-flags/--device-flags-file both previously supported passing flags for instrumentation tests in a file. There was no instrumentation tests support for providing such flags directly. This CL adds that support by adding --test-arguments (matching gtests) and switching --device-flags to be an alias of that. BUG=700366 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
