Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23295006:
[Android] Changes _RunJavaTests to accept filters (Closed)
|
Created:
7 years, 4 months ago by gkanwar1 Modified:
7 years, 4 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionChanges _RunJavaTests to accept filters
This eliminates the need to specify package name and bring it
more in line with normal instrumentation tests.
NOTRY=True
BUG=268683
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218530
Patch Set 1 #
Total comments: 6
Patch Set 2 : Leaves old _RunJavaTests signature for backwards compatibility #
Messages
Total messages: 13 (0 generated)
ptal
lgtm. Make sure all downstream tests pass. https://codereview.chromium.org/23295006/diff/1/chrome/android/host_driven_te... File chrome/android/host_driven_tests/DummyTest.py (right): https://codereview.chromium.org/23295006/diff/1/chrome/android/host_driven_te... chrome/android/host_driven_tests/DummyTest.py:16: return self._RunJavaTests(['DummyTest.testPass']) You can specify using '.' and '#'?
Marcus can you do a quick review here. The upside here is simplicity but it assumes "TestClass.testMethod" are unique across packages.
I'll run the downstream tests when I get the change tomorrow. https://codereview.chromium.org/23295006/diff/1/chrome/android/host_driven_te... File chrome/android/host_driven_tests/DummyTest.py (right): https://codereview.chromium.org/23295006/diff/1/chrome/android/host_driven_te... chrome/android/host_driven_tests/DummyTest.py:16: return self._RunJavaTests(['DummyTest.testPass']) On 2013/08/19 20:46:59, frankf wrote: > You can specify using '.' and '#'? Yes. The current filtering system is happy either way, and this is backwards compatible.
lgtm, just one suggestion to avoid breakage and sheriffing headaches :) https://codereview.chromium.org/23295006/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_case.py (right): https://codereview.chromium.org/23295006/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_case.py:99: def _RunJavaTests(self, test_filters): we should probably do this two-sided? keep the other method until it rolls, then remove it later? it should be easy to just delegate from the old interface to the new one anyways..
Just waiting to hear back from Marcus re: two-sided. https://codereview.chromium.org/23295006/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_case.py (right): https://codereview.chromium.org/23295006/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_case.py:99: def _RunJavaTests(self, test_filters): On 2013/08/20 11:08:07, bulach wrote: > we should probably do this two-sided? keep the other method until it rolls, then > remove it later? it should be easy to just delegate from the old interface to > the new one anyways.. By two-sided do you mean: * Upstream: Create a new method, keep the old * Downstream: Update tests to point at the new method * Upstream: Remove the old method or * Upstream: Change this method * Downstream: Wait for roll and land a patch updating all downstream tests I was imagining the latter, but we could do the former as well, since it would be a more straightforward process (if a little longer).
still lgtm, just one clarification inline: https://codereview.chromium.org/23295006/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_case.py (right): https://codereview.chromium.org/23295006/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_case.py:99: def _RunJavaTests(self, test_filters): On 2013/08/20 15:45:33, gkanwar1 wrote: > On 2013/08/20 11:08:07, bulach wrote: > > we should probably do this two-sided? keep the other method until it rolls, > then > > remove it later? it should be easy to just delegate from the old interface to > > the new one anyways.. > > By two-sided do you mean: > * Upstream: Create a new method, keep the old > * Downstream: Update tests to point at the new method > * Upstream: Remove the old method > > or > > * Upstream: Change this method > * Downstream: Wait for roll and land a patch updating all downstream tests > > I was imagining the latter, but we could do the former as well, since it would > be a more straightforward process (if a little longer). the former :) requires less coordination between you and sheriffs / gardeners / CQ... I agree it takes a bit longer, but you can fire and forget all of them independently... if you prefer the latter, that's fine by me as well, just make sure you have the downstream patch ready and sent to the appropriate sheriffs / gardeners (there's a mailing list for that) so they can land that once it rolls.. assuming of course this won't break the autoroller, not sure if we run these tests there: if we do, then there's no way around, we'd have to do in 3 steps version.
https://codereview.chromium.org/23295006/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_case.py (right): https://codereview.chromium.org/23295006/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_case.py:99: def _RunJavaTests(self, test_filters): On 2013/08/20 15:53:10, bulach wrote: > On 2013/08/20 15:45:33, gkanwar1 wrote: > > On 2013/08/20 11:08:07, bulach wrote: > > > we should probably do this two-sided? keep the other method until it rolls, > > then > > > remove it later? it should be easy to just delegate from the old interface > to > > > the new one anyways.. > > > > By two-sided do you mean: > > * Upstream: Create a new method, keep the old > > * Downstream: Update tests to point at the new method > > * Upstream: Remove the old method > > > > or > > > > * Upstream: Change this method > > * Downstream: Wait for roll and land a patch updating all downstream tests > > > > I was imagining the latter, but we could do the former as well, since it would > > be a more straightforward process (if a little longer). > > the former :) > requires less coordination between you and sheriffs / gardeners / CQ... I agree > it takes a bit longer, but you can fire and forget all of them independently... > > if you prefer the latter, that's fine by me as well, just make sure you have the > downstream patch ready and sent to the appropriate sheriffs / gardeners (there's > a mailing list for that) so they can land that once it rolls.. assuming of > course this won't break the autoroller, not sure if we run these tests there: if > we do, then there's no way around, we'd have to do in 3 steps version. Yeah, the only other problem I had with the former was that we are necessarily required to change the method name. Since we're changing the function to use filters anyway, that actually makes sense, so I'm happy to do it that way. I'll upload a change for that shortly.
Updated to leave the old function in place. ptal, thanks!
lgtm thanks
Thanks. Waiting on the last trybot, then I'll land this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gkanwar@chromium.org/23295006/2003
Message was sent while issue was closed.
Change committed as 218530 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
