Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815453003:
Create ChromeActivityTestRule for converting JUnit4 tests (Closed)
|
Created:
3 years, 8 months ago by the real yoland Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, dominickn+watch_chromium.org, lizeb+watch-custom-tabs_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate ChromeActivityTestRule for converting JUnit4 tests
The follows the same pattern created for content test rules and test base, where
test rules and test bases share as much same implementation as possible in the
test common classes during the migraion. For more on JUnit4 migration, please
check src/testing/android/docs/junit4.md
BUG=640116
Review-Url: https://codereview.chromium.org/2815453003
Cr-Commit-Position: refs/heads/master@{#465858}
Committed: https://chromium.googlesource.com/chromium/src/+/8dc40e2e6496012195f8b28db158e787486fbc07
Patch Set 1 #
Total comments: 7
Patch Set 2 : address john's comments #
Total comments: 9
Patch Set 3 : address ted's comments #Patch Set 4 : rebase #
Total comments: 6
Patch Set 5 : Added TODOs #
Total comments: 2
Patch Set 6 : nits and rebase #Messages
Total messages: 26 (12 generated)
yolandyan@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org, tedchoc@chromium.org
Description was changed from ========== Create ChromeActivityTestRule for converting JUnit4 tests BUG=640116 ========== to ========== Create ChromeActivityTestRule for converting JUnit4 tests The follows the same pattern created for content test rules and test base, where test rules and test bases share as much same implementation as possible in the test common classes. For more on JUnit4 migration, please check src/testing/android/docs/junit4.md BUG=640116 ==========
Description was changed from ========== Create ChromeActivityTestRule for converting JUnit4 tests The follows the same pattern created for content test rules and test base, where test rules and test bases share as much same implementation as possible in the test common classes. For more on JUnit4 migration, please check src/testing/android/docs/junit4.md BUG=640116 ========== to ========== Create ChromeActivityTestRule for converting JUnit4 tests The follows the same pattern created for content test rules and test base, where test rules and test bases share as much same implementation as possible in the test common classes during the migraion. For more on JUnit4 migration, please check src/testing/android/docs/junit4.md BUG=640116 ==========
https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java (right): https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:39: @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE, How will this get handled in the JUnit4 version? On a related note, why is https://codereview.chromium.org/2523983002 closed? https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:42: public abstract class ChromeActivityTestCaseBase<T extends ChromeActivity> I'm very excited about the coming end of this class. https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:51: @Deprecated Will this result in a bunch of new build-time warnings? If so, I'm not sure we necessarily want that as we go through the conversion. https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java (right): https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java:57: final Statement superBase = new Statement() { Is this necessary? Can you just call mTestCommon.setUp() and base.evaluate() inline below?
https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java (right): https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:39: @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE, On 2017/04/11 at 14:31:38, jbudorick wrote: > How will this get handled in the JUnit4 version? > > On a related note, why is https://codereview.chromium.org/2523983002 closed? Over here in ChromeActivityTestCaseBase, this is still handled by the JUnit3 side, In junit4 test, the same annotation supported by changes in BaseJUnit4ClassRunner in CL https://codereview.chromium.org/2766393004 https://codereview.chromium.org/2523983002 is temporarily closed and I will try to come up with a better solution for CommandLineFlag rule after the conversion. https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:51: @Deprecated On 2017/04/11 at 14:31:38, jbudorick wrote: > Will this result in a bunch of new build-time warnings? If so, I'm not sure we necessarily want that as we go through the conversion. hmm, ya, I guess it's not necessary since it's going away in the end anyway https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java (right): https://codereview.chromium.org/2815453003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java:57: final Statement superBase = new Statement() { On 2017/04/11 at 14:31:39, jbudorick wrote: > Is this necessary? Can you just call mTestCommon.setUp() and base.evaluate() inline below? My bad, this should have been ``` final Statement superBase = super.apply(new Statement() {...}, desc); This is because perf test setup should happen before everything, even before the action given in super.apply() However, in reality, knowing the implementation of ActivityTestRule, nothing is done in super.apply(), so it would have been the same just by putting everything in the same Statement and pass it to super.apply()
https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java (right): https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:865: public static interface ChromeTestCommonCallback { I don't think static is required. https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:866: String getTestNameForTestCommon(); In general, I think the names should just match the existing names in ChromeActivityTestCaseBase. Adding ForTestCommon is likely just requiring us to add more methods. If they were previously protected, then I think we should just add public method vars instead of adding these level of specificity. https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:867: <S extends ChromeActivity> void setActivityForTestCommon(S chromeActivity); Casting to any other type of ChromeActivity vs the type specified in the original test is very dangerous. If you want to templatize this, it should be done at the interface level and not at the method one. https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java (right): https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java:141: public void dragStart(float x, float y, long downTime) { FWIW, many of these methods are really only useful if they are in the class inheritance chain. If you have to start doing mRule.dragStart<...>, then it's not a huge benefit of redirection to touchcommon. For cases like this, why wouldn't we want to have a junit4 class hierarchy that provides these in the base to avoid this boilerplate redirect.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java (right): https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:865: public static interface ChromeTestCommonCallback { On 2017/04/12 at 00:02:11, Ted C wrote: > I don't think static is required. Done https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:866: String getTestNameForTestCommon(); On 2017/04/12 at 00:02:11, Ted C wrote: > In general, I think the names should just match the existing names in ChromeActivityTestCaseBase. Adding ForTestCommon is likely just requiring us to add more methods. If they were previously protected, then I think we should just add public method vars instead of adding these level of specificity. got it, these forTesTCommon methods are only existing until the end of the migration. Changed https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:867: <S extends ChromeActivity> void setActivityForTestCommon(S chromeActivity); On 2017/04/12 at 00:02:11, Ted C wrote: > Casting to any other type of ChromeActivity vs the type specified in the original test is very dangerous. If you want to templatize this, it should be done at the interface level and not at the method one. Done https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java (right): https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java:141: public void dragStart(float x, float y, long downTime) { On 2017/04/12 at 00:02:11, Ted C wrote: > FWIW, many of these methods are really only useful if they are in the class inheritance chain. If you have to start doing mRule.dragStart<...>, then it's not a huge benefit of redirection to touchcommon. > > For cases like this, why wouldn't we want to have a junit4 class hierarchy that provides these in the base to avoid this boilerplate redirect. First, it's easier to manage tests without a bunch of base test classes. The danger of having both base classes and rules is that people will end up putting some methods in rules, others in base class, and there is no way to generically draw a line in between in terms of who handles what. Even worse, we might end up in a situation where base class has its declared test rules has well. Having one base class here, people will just end up adding more layers of base classes. An helper method might be provided or changed by any of these base classes. An example would be ChromeActivityTestBase's abstract method `startMainActivity`, so some descendants will have to declare this method just to tell test base not to do anything, others might already have parents that defined the class for them. It seems to me that this can be confusing for devs who want to write or change tests. Second reason is performance of inheritance in JUnit4, bacause junit4's runner relies on reflection (`getDelaredXXX`) a lot to look for method and field, if there is any super class for a test, JUnit4 would make extra calls for find these elements in all super classes as well. I haven't done a profiling for this, but performance with base classes surely would not help. Considering the potential damage of having base classes and rule together, I think calling adding `TouchCommon.` to these calls doesn't seem that bad. There are only 4 tests that uses `dragStart()`, and the most used call `singleClickView()`, there are 15 tests that uses it. They can be easily converted to TouchCommon.xxx during this migration.
https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java (right): https://codereview.chromium.org/2815453003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java:141: public void dragStart(float x, float y, long downTime) { On 2017/04/12 18:22:54, the real yoland wrote: > On 2017/04/12 at 00:02:11, Ted C wrote: > > FWIW, many of these methods are really only useful if they are in the class > inheritance chain. If you have to start doing mRule.dragStart<...>, then it's > not a huge benefit of redirection to touchcommon. > > > > For cases like this, why wouldn't we want to have a junit4 class hierarchy > that provides these in the base to avoid this boilerplate redirect. > > First, it's easier to manage tests without a bunch of base test classes. The problem here is that I fundamentally disagree with your claim here. The main issue with the class inheritance we've created in Chrome is that we did it with little rhyme or reason. The base class was introduced only when we had tabbed mode. We didn't do the necessarily cleanups when we introduced document, CCTs, Webapps, etc... I will be equally disappointed with the outcome if all our tests have to include the exact same boilerplate. Having composable rules are great, and definitely where we should get to. But at the same time for truly common code, we should introduce classes that make writing tests that much easier. The rules should make that better for if you don't need full capabilities of one of these, you can easily compose the rules into something that works for you. But here, this is something neither you or I should ultimately decide, we should figure out as a team what is best, so I would recommend having this debate somewhere outside of this change. > The > danger of having both base classes and rules is that people will end up putting > some methods in rules, others in base class, and there is no way to generically > draw a line in between in terms of who handles what. Even worse, we might end up > in a situation where base class has its declared test rules has well. > > Having one base class here, people will just end up adding more layers of base > classes. An helper method might be provided or changed by any of these base > classes. An example would be ChromeActivityTestBase's abstract method > `startMainActivity`, so some descendants will have to declare this method just > to tell test base not to do anything, others might already have parents that > defined the class for them. It seems to me that this can be confusing for devs > who want to write or change tests. > > Second reason is performance of inheritance in JUnit4, bacause junit4's runner > relies on reflection (`getDelaredXXX`) a lot to look for method and field, if > there is any super class for a test, JUnit4 would make extra calls for find > these elements in all super classes as well. I haven't done a profiling for > this, but performance with base classes surely would not help. Based on how long our tests take to run, I would bet hugely that any reflection overhead is minimal. > > Considering the potential damage of having base classes and rule together, I > think calling adding `TouchCommon.` to these calls doesn't seem that bad. There > are only 4 tests that uses `dragStart()`, and the most used call > `singleClickView()`, there are 15 tests that uses it. They can be easily > converted to TouchCommon.xxx during this migration. https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java (right): https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:439: void newIncognitoTabFromMenu() throws InterruptedException { Do we want to take this opportunity to split out things that don't make sense in the base rule? For example, this only applies for Tabbed mode. So it should be in a rule specific to it. https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:514: void typeInOmnibox(final String text, final boolean oneCharAtATime) same, this only applies to tabbed mode. https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeJUnit4ClassRunner.java (right): https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeJUnit4ClassRunner.java:39: return Arrays.asList(new PreTestHook[] { I think CollectionUtil.newArrayList is better here.
<snip> > > First, it's easier to manage tests without a bunch of base test classes. > > The problem here is that I fundamentally disagree with your claim here. > > The main issue with the class inheritance we've created in Chrome is that > we did it with little rhyme or reason. The base class was introduced only > when we had tabbed mode. We didn't do the necessarily cleanups when we > introduced document, CCTs, Webapps, etc... I suppose it's better to say it's easier to manage tests without *our* bunch of base test classes. > > I will be equally disappointed with the outcome if all our tests have to > include the exact same boilerplate. > > Having composable rules are great, and definitely where we should get to. > > But at the same time for truly common code, we should introduce classes > that make writing tests that much easier. The rules should make that better > for if you don't need full capabilities of one of these, you can easily > compose the rules into something that works for you. > > But here, this is something neither you or I should ultimately decide, we > should figure out as a team what is best, so I would recommend having this > debate somewhere outside of this change. > I think we're in general agreement about the ideal end state. That may not be the case w/ how we're planning to get there. We're planning on getting entirely off of JUnit4 through these mostly rote conversions *and then* breaking our big monolithic rules into concise, single-purpose rules and, where reasonable, compositions thereof. Switching frameworks *while* refactoring from monolithic base classes to more focused classes used in composition seems more risky, more tedious, and more confusing for developers than doing the two separately. We may also be in disagreement about the amount of tolerable boilerplate. In the case of TouchCommon, something like mTouchRule.dragStart(...) does not seem substantially more annoying than dragStart(...) provided the touch rule knows about and handles the Activity, which seems like the main value of the existing redirection. <snip>
On 2017/04/13 19:45:06, jbudorick wrote: > <snip> > > > > First, it's easier to manage tests without a bunch of base test classes. > > > > The problem here is that I fundamentally disagree with your claim here. > > > > The main issue with the class inheritance we've created in Chrome is that > > we did it with little rhyme or reason. The base class was introduced only > > when we had tabbed mode. We didn't do the necessarily cleanups when we > > introduced document, CCTs, Webapps, etc... > > I suppose it's better to say it's easier to manage tests without *our* bunch of > base test classes. > > > > > I will be equally disappointed with the outcome if all our tests have to > > include the exact same boilerplate. > > > > Having composable rules are great, and definitely where we should get to. > > > > But at the same time for truly common code, we should introduce classes > > that make writing tests that much easier. The rules should make that better > > for if you don't need full capabilities of one of these, you can easily > > compose the rules into something that works for you. > > > > But here, this is something neither you or I should ultimately decide, we > > should figure out as a team what is best, so I would recommend having this > > debate somewhere outside of this change. > > > > I think we're in general agreement about the ideal end state. That may not be > the case w/ how we're planning to get there. We're planning on getting entirely > off of JUnit4 through these mostly rote conversions *and then* breaking our big > monolithic rules into concise, single-purpose rules and, where reasonable, > compositions thereof. Switching frameworks *while* refactoring from monolithic > base classes to more focused classes used in composition seems more risky, more > tedious, and more confusing for developers than doing the two separately. This seems fine, but I have my back of mind worry that this will be one of things where we talk about cleanups but never prioritize them high enough and we've moved from a broken system to another broken system with a shinier name. But...let's stay optimistic here. > > We may also be in disagreement about the amount of tolerable boilerplate. In the > case of TouchCommon, something like > > mTouchRule.dragStart(...) > > does not seem substantially more annoying than > > dragStart(...) > For TouchCommon, we just shouldn't have a rule IMO. I think the inheritance was the only thing that made it nice. dragStart(getActivity(), otherParams) vs mTouchRule.dragStart(otherParams); Granted if getActivity() actually needs to be mOtherRule.getActivity() then that is slightly annoying, but I'm still not convinced the rule is worth it. My main thing is that I think we should have base classes that compose common used rules and patterns to make the similar tests easier to write. > > provided the touch rule knows about and handles the Activity, which seems like > the main value of the existing redirection. > > <snip> FWIW, this lgtm given the plan.
On 2017/04/13 20:07:09, Ted C wrote: > On 2017/04/13 19:45:06, jbudorick wrote: > > <snip> > > > > > > First, it's easier to manage tests without a bunch of base test classes. > > > > > > The problem here is that I fundamentally disagree with your claim here. > > > > > > The main issue with the class inheritance we've created in Chrome is that > > > we did it with little rhyme or reason. The base class was introduced only > > > when we had tabbed mode. We didn't do the necessarily cleanups when we > > > introduced document, CCTs, Webapps, etc... > > > > I suppose it's better to say it's easier to manage tests without *our* bunch > of > > base test classes. > > > > > > > > I will be equally disappointed with the outcome if all our tests have to > > > include the exact same boilerplate. > > > > > > Having composable rules are great, and definitely where we should get to. > > > > > > But at the same time for truly common code, we should introduce classes > > > that make writing tests that much easier. The rules should make that better > > > for if you don't need full capabilities of one of these, you can easily > > > compose the rules into something that works for you. > > > > > > But here, this is something neither you or I should ultimately decide, we > > > should figure out as a team what is best, so I would recommend having this > > > debate somewhere outside of this change. > > > > > > > I think we're in general agreement about the ideal end state. That may not be > > the case w/ how we're planning to get there. We're planning on getting > entirely > > off of JUnit4 through these mostly rote conversions *and then* breaking our > big > > monolithic rules into concise, single-purpose rules and, where reasonable, > > compositions thereof. Switching frameworks *while* refactoring from monolithic > > base classes to more focused classes used in composition seems more risky, > more > > tedious, and more confusing for developers than doing the two separately. > > This seems fine, but I have my back of mind worry that this will be one of > things > where we talk about cleanups but never prioritize them high enough and we've > moved from a broken system to another broken system with a shinier name. It's at worst a broken system that isn't deprecated, not just one with a shinier name :) > > But...let's stay optimistic here. > > > > > We may also be in disagreement about the amount of tolerable boilerplate. In > the > > case of TouchCommon, something like > > > > mTouchRule.dragStart(...) > > > > does not seem substantially more annoying than > > > > dragStart(...) > > > > For TouchCommon, we just shouldn't have a rule IMO. I think the inheritance was > the only thing that made it nice. > > dragStart(getActivity(), otherParams) > > vs > > mTouchRule.dragStart(otherParams); > > Granted if getActivity() actually needs to be mOtherRule.getActivity() then > that is slightly annoying, but I'm still not convinced the rule is worth it. > > My main thing is that I think we should have base classes that compose common > used rules and patterns to make the similar tests easier to write. We can (and certainly will) have more discussions about this beyond this CL as the mass conversions come to a close. Like Yoland, I'd been leaning more toward having Rules bind together other Rules and then having tests use those compound Rules directly rather than having base classes serve a similar role. > > > > > provided the touch rule knows about and handles the Activity, which seems like > > the main value of the existing redirection. > > > > > > > <snip> > > FWIW, this lgtm given the plan.
On 2017/04/13 at 20:07:09, tedchoc wrote: > On 2017/04/13 19:45:06, jbudorick wrote: > > <snip> > > > > > > First, it's easier to manage tests without a bunch of base test classes. > > > > > > The problem here is that I fundamentally disagree with your claim here. > > > > > > The main issue with the class inheritance we've created in Chrome is that > > > we did it with little rhyme or reason. The base class was introduced only > > > when we had tabbed mode. We didn't do the necessarily cleanups when we > > > introduced document, CCTs, Webapps, etc... > > > > I suppose it's better to say it's easier to manage tests without *our* bunch of > > base test classes. > > > > > > > > I will be equally disappointed with the outcome if all our tests have to > > > include the exact same boilerplate. > > > > > > Having composable rules are great, and definitely where we should get to. > > > > > > But at the same time for truly common code, we should introduce classes > > > that make writing tests that much easier. The rules should make that better > > > for if you don't need full capabilities of one of these, you can easily > > > compose the rules into something that works for you. > > > > > > But here, this is something neither you or I should ultimately decide, we > > > should figure out as a team what is best, so I would recommend having this > > > debate somewhere outside of this change. > > > > > > > I think we're in general agreement about the ideal end state. That may not be > > the case w/ how we're planning to get there. We're planning on getting entirely > > off of JUnit4 through these mostly rote conversions *and then* breaking our big > > monolithic rules into concise, single-purpose rules and, where reasonable, > > compositions thereof. Switching frameworks *while* refactoring from monolithic > > base classes to more focused classes used in composition seems more risky, more > > tedious, and more confusing for developers than doing the two separately. > > This seems fine, but I have my back of mind worry that this will be one of things > where we talk about cleanups but never prioritize them high enough and we've > moved from a broken system to another broken system with a shinier name. > > But...let's stay optimistic here. > One thing to add-on to a big reason behind is the migration method is that since we are using a script parse and refactor the base class methods, since manually converting this can be quite tedious. The script is not smart enough helper methods are split up into different rules, which rule to use, how they interact with each other. However, I add TODOs in the code and created this bug to keep track of rules to be refactored once the migration is over. (crbug.com/711517) > > > > We may also be in disagreement about the amount of tolerable boilerplate. In the > > case of TouchCommon, something like > > > > mTouchRule.dragStart(...) > > > > does not seem substantially more annoying than > > > > dragStart(...) > > > > For TouchCommon, we just shouldn't have a rule IMO. I think the inheritance was > the only thing that made it nice. > > dragStart(getActivity(), otherParams) > > vs > > mTouchRule.dragStart(otherParams); > > Granted if getActivity() actually needs to be mOtherRule.getActivity() then > that is slightly annoying, but I'm still not convinced the rule is worth it. > > My main thing is that I think we should have base classes that compose common > used rules and patterns to make the similar tests easier to write. > > > > > provided the touch rule knows about and handles the Activity, which seems like > > the main value of the existing redirection. > > > > > > > <snip> > > FWIW, this lgtm given the plan.
lgtm w/ nit https://codereview.chromium.org/2815453003/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2815453003/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:11: import android.support.test.runner.AndroidJUnit4; Remove if not used.
The CQ bit was checked by yolandyan@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java (right): https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:439: void newIncognitoTabFromMenu() throws InterruptedException { On 2017/04/13 at 18:50:49, Ted C wrote: > Do we want to take this opportunity to split out things that don't make sense in the base rule? > > For example, this only applies for Tabbed mode. So it should be in a rule specific to it. Added TODO https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCommon.java:514: void typeInOmnibox(final String text, final boolean oneCharAtATime) On 2017/04/13 at 18:50:49, Ted C wrote: > same, this only applies to tabbed mode. Added TODO https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeJUnit4ClassRunner.java (right): https://codereview.chromium.org/2815453003/diff/80001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeJUnit4ClassRunner.java:39: return Arrays.asList(new PreTestHook[] { On 2017/04/13 at 18:50:49, Ted C wrote: > I think CollectionUtil.newArrayList is better here. Done https://codereview.chromium.org/2815453003/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2815453003/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:11: import android.support.test.runner.AndroidJUnit4; On 2017/04/19 at 21:13:13, jbudorick wrote: > Remove if not used. Done
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2815453003/#ps120001 (title: "nits and rebase")
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": 120001, "attempt_start_ts": 1492653425616330,
"parent_rev": "4a08ce5f3acdb7170a36da773a696cdcd3e96788", "commit_rev":
"8dc40e2e6496012195f8b28db158e787486fbc07"}
Message was sent while issue was closed.
Description was changed from ========== Create ChromeActivityTestRule for converting JUnit4 tests The follows the same pattern created for content test rules and test base, where test rules and test bases share as much same implementation as possible in the test common classes during the migraion. For more on JUnit4 migration, please check src/testing/android/docs/junit4.md BUG=640116 ========== to ========== Create ChromeActivityTestRule for converting JUnit4 tests The follows the same pattern created for content test rules and test base, where test rules and test bases share as much same implementation as possible in the test common classes during the migraion. For more on JUnit4 migration, please check src/testing/android/docs/junit4.md BUG=640116 Review-Url: https://codereview.chromium.org/2815453003 Cr-Commit-Position: refs/heads/master@{#465858} Committed: https://chromium.googlesource.com/chromium/src/+/8dc40e2e6496012195f8b28db158... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/8dc40e2e6496012195f8b28db158... |
