Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2523983002:
Create CommandLineTestRule (Closed)
|
Created:
4 years, 1 month ago by the real yoland Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate CommandLineTestRule and its junit tests
CommandLineTestRule achieve the same goal as the PreTestHook (set up commandline
flags). This CL also removes PreTestHook from class runner.
BUG=640116
Patch Set 1 #
Total comments: 11
Patch Set 2 : Remove PreTestHook in BaseJUnit4ClassRunner #
Total comments: 10
Patch Set 3 : change based on comments #
Total comments: 20
Patch Set 4 : Add CommandLineTestRule #Patch Set 5 : change gn file #
Total comments: 2
Patch Set 6 : Added javadoc explanation #Patch Set 7 : Change CommandLineTestRule methods #Patch Set 8 : Add findbugs filter #Patch Set 9 : implements SetUpTestRule #
Depends on Patchset: Messages
Total messages: 38 (20 generated)
yolandyan@chromium.org changed reviewers: + jbudorick@chromium.org
I like this. https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java (right): https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:78: public static void setUp(Context targetContext, Set<String> flags) { Public methods ought to have javadoc comments. https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:91: flags = addAndRemoveFlags(flags, element.getAnnotation(CommandLineFlags.Add.class), nit: The indentation here, while maybe right, looks weird w/ addAndRemoveFlags matching element.getAnnotation https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:181: * @Rule CommandLineTestRule mCommandLineRule = new CommandLineTestRule(); So what happens if someone uses @CommandLineFlags.{Add,Remove} without the CommandLineTestRule? Nothing? If so, could we warn people somehow? https://codereview.chromium.org/2523983002/diff/1/base/test/android/junit/src... File base/test/android/junit/src/org/chromium/base/test/CommandLineFlagsTest.java (right): https://codereview.chromium.org/2523983002/diff/1/base/test/android/junit/src... base/test/android/junit/src/org/chromium/base/test/CommandLineFlagsTest.java:41: One more test case: a second class that extends ParentClassAddFlagA and doesn't remove A.
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java (right): https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:103: public static Set<String> addAndRemoveFlags(Set<String> flags, nit: updateFlags might be a better name https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:181: * @Rule CommandLineTestRule mCommandLineRule = new CommandLineTestRule(); Is there any way to warn people if they added the annotations but forgot to add this rule? Can we add this rule as default for all test?
https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java (right): https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:78: public static void setUp(Context targetContext, Set<String> flags) { On 2016/11/23 at 16:07:36, jbudorick wrote: > Public methods ought to have javadoc comments. Done https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:91: flags = addAndRemoveFlags(flags, element.getAnnotation(CommandLineFlags.Add.class), On 2016/11/23 at 16:07:36, jbudorick wrote: > nit: The indentation here, while maybe right, looks weird w/ addAndRemoveFlags matching element.getAnnotation Hmm, it does looks weird, but it was formatted by `git cl format base` and presubmit would give me warning if I don't have indentation this way. https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:103: public static Set<String> addAndRemoveFlags(Set<String> flags, On 2016/11/23 at 16:14:34, mikecase wrote: > nit: updateFlags might be a better name Done https://codereview.chromium.org/2523983002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:181: * @Rule CommandLineTestRule mCommandLineRule = new CommandLineTestRule(); On 2016/11/23 at 16:14:34, mikecase wrote: > Is there any way to warn people if they added the annotations but forgot to add this rule? > Can we add this rule as default for all test? I can throw error in BaseJUnit4ClassRunner when a test has Add/Remove annotation without a CommandLineTestRule field I would like to think of an expandible way to do this in BaseJunit4ClassRunner in case other Rules are required for other annotations. Added a temporary TODO in the code, will implement in this CL https://codereview.chromium.org/2523983002/diff/1/base/test/android/junit/src... File base/test/android/junit/src/org/chromium/base/test/CommandLineFlagsTest.java (right): https://codereview.chromium.org/2523983002/diff/1/base/test/android/junit/src... base/test/android/junit/src/org/chromium/base/test/CommandLineFlagsTest.java:41: On 2016/11/23 at 16:07:36, jbudorick wrote: > One more test case: a second class that extends ParentClassAddFlagA and doesn't remove A. Done
friendly ping
non-owner lgtm, w/ question https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java (right): https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:222: mBase.evaluate(); what does mBase.evaluate() do?
https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java (right): https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:10: import org.junit.Assert; Does this work with the existing JUnit3-based code? https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:17: import org.chromium.base.VisibleForTesting; This shouldn't be necessary... https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:104: flags = updateFlags(flags, element.getAnnotation(CommandLineFlags.Add.class), nit: return updateFlags(...) https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:115: flags = updateFlags(flags, desc.getAnnotation(CommandLineFlags.Add.class), nit: return updateFlags(...)
https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java (right): https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:10: import org.junit.Assert; yes, it does https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:17: import org.chromium.base.VisibleForTesting; On 2016/12/01 at 00:30:14, jbudorick wrote: > This shouldn't be necessary... Deleted https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:104: flags = updateFlags(flags, element.getAnnotation(CommandLineFlags.Add.class), On 2016/12/01 at 00:30:14, jbudorick wrote: > nit: return updateFlags(...) Done https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:115: flags = updateFlags(flags, desc.getAnnotation(CommandLineFlags.Add.class), On 2016/12/01 at 00:30:14, jbudorick wrote: > nit: return updateFlags(...) Done https://codereview.chromium.org/2523983002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:222: mBase.evaluate(); On 2016/12/01 at 00:27:54, mikecase wrote: > what does mBase.evaluate() do? This is the the onion structure Rule have to wrap one statement around another. mBase.evaluate() call the inner statement's evaluate, which would it's inner statement, etc. It's not really clean design, but that's just junit4 for you :(
https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java (right): https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:67: * Sets up the CommandLine flags using test method or class. nit: "... using the given test method or class." https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:75: setUp(targetContext, flags); nit: just do setUp(targetContext, getFlags(element)); https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:81: * This will add the difference of the sets of flags specified by {@link CommandLineFlags.Add} Update this sentence: "This will add {@code flags} to the {@link org.chromium.base.CommandLine}." updateFlags, which this does not call, is responsible for getting the set of flags specified by various @CommandLineFlags.Add and @CommandLineFlags.Remove annotations. https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:95: * @return a set of strings to represent the commandline flags This should say something like /** Determines the set of command-line flags that should be added for the given element. * * This determines the difference of the sets of flags specified by {@link CommandLineFlags.Add} * and {@link CommandLineFlags.Remove}. */ https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:97: @VisibleForTesting This shouldn't be necessary. https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:108: * @return a set of strings to represent the commandline flags Same as above. https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:110: @VisibleForTesting This shouldn't be necessary. https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:111: public static Set<String> getFlags(Description desc) { Do we need to provide both versions of getFlags? https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:113: return updateFlags(flags, desc.getAnnotation(CommandLineFlags.Add.class), Why does this call updateFlags a second time? https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:205: public static class CommandLineTestRule implements TestRule { A client would use this as @Rule CommandLineFlags.CommandLineTestRule mCommandLineRule = new CommandLineFlags.CommandLineTestRule() Could we either: 1) rename this s.t. CommandLineFlags.<new name> isn't as repetitive (e.g. name it "Rule" s.t. a client would do CommandLineFlags.Rule), or 2) move this to its own file & have it be standalone.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java (right): https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:67: * Sets up the CommandLine flags using test method or class. On 2017/01/11 at 20:21:33, jbudorick wrote: > nit: "... using the given test method or class." No longer relevant https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:75: setUp(targetContext, flags); On 2017/01/11 at 20:21:33, jbudorick wrote: > nit: just do > > setUp(targetContext, getFlags(element)); No longer relevant https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:81: * This will add the difference of the sets of flags specified by {@link CommandLineFlags.Add} On 2017/01/11 at 20:21:33, jbudorick wrote: > Update this sentence: > > "This will add {@code flags} to the {@link org.chromium.base.CommandLine}." > > updateFlags, which this does not call, is responsible for getting the set of flags specified by various @CommandLineFlags.Add and @CommandLineFlags.Remove annotations. Done https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:95: * @return a set of strings to represent the commandline flags On 2017/01/11 at 20:21:33, jbudorick wrote: > This should say something like > > /** Determines the set of command-line flags that should be added for the given element. > * > * This determines the difference of the sets of flags specified by {@link CommandLineFlags.Add} > * and {@link CommandLineFlags.Remove}. > */ No longer relevant https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:97: @VisibleForTesting On 2017/01/11 at 20:21:33, jbudorick wrote: > This shouldn't be necessary. No longer relevant https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:108: * @return a set of strings to represent the commandline flags On 2017/01/11 at 20:21:33, jbudorick wrote: > Same as above. No longer relevant https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:110: @VisibleForTesting On 2017/01/11 at 20:21:33, jbudorick wrote: > This shouldn't be necessary. No longer relevant https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:111: public static Set<String> getFlags(Description desc) { On 2017/01/11 at 20:21:33, jbudorick wrote: > Do we need to provide both versions of getFlags? No longer relevant https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:113: return updateFlags(flags, desc.getAnnotation(CommandLineFlags.Add.class), On 2017/01/11 at 20:21:32, jbudorick wrote: > Why does this call updateFlags a second time? No longer relevant https://codereview.chromium.org/2523983002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:205: public static class CommandLineTestRule implements TestRule { On 2017/01/11 at 20:21:32, jbudorick wrote: > A client would use this as > > @Rule CommandLineFlags.CommandLineTestRule mCommandLineRule = new CommandLineFlags.CommandLineTestRule() > > Could we either: > 1) rename this s.t. CommandLineFlags.<new name> isn't as repetitive (e.g. name it "Rule" s.t. a client would do CommandLineFlags.Rule), or > 2) move this to its own file & have it be standalone. Moved CommandLineTestRule to its own file
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...
Dependent CL: https://codereview.chromium.org/2755863004
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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...
Description was changed from ========== Create CommandLineTestRule and its junit tests CommandLineTestRule achieve the same goal as the PreTestHook (set up commandline flags). This is the first step to enabling parameterized test with commandline flags in JUnit4, this would allow commandline flags to be specified in both annotations like CommandLineFlags.Add/Remove and the rule itself. Which one can eventually create multiple TestRule to specify the flags used each time BUG=640116 ========== to ========== Create CommandLineTestRule and its junit tests CommandLineTestRule achieve the same goal as the PreTestHook (set up commandline flags). This CL also removes PreTestHook from class runner. BUG=640116 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
CL is ready, PTAL
lgtm w/ q https://codereview.chromium.org/2523983002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineTestRule.java (right): https://codereview.chromium.org/2523983002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineTestRule.java:50: CommandLineFlags.setUp(InstrumentationRegistry.getTargetContext(), mFlagSet); Remind me again (and add a comment explaining) why we can't run this in the Statement returned by apply?
https://codereview.chromium.org/2523983002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineTestRule.java (right): https://codereview.chromium.org/2523983002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/CommandLineTestRule.java:50: CommandLineFlags.setUp(InstrumentationRegistry.getTargetContext(), mFlagSet); On 2017/03/21 at 14:56:50, jbudorick wrote: > Remind me again (and add a comment explaining) why we can't run this in the Statement returned by apply? we actually can do have apply() return a statement that just call this But there is no guarantee this rule will run before activity test rule, if the activity test rule were to automatically launches the activity. Maybe we can do some class runner check that makes sure CommandLineTestRule always run before ActivityTestRule?
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2523983002/#ps120001 (title: "Added TODO")
The CQ bit was unchecked by yolandyan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/21 at 15:12:42, the real yoland wrote: > https://codereview.chromium.org/2523983002/diff/100001/base/test/android/java... > File base/test/android/javatests/src/org/chromium/base/test/util/CommandLineTestRule.java (right): > > https://codereview.chromium.org/2523983002/diff/100001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/util/CommandLineTestRule.java:50: CommandLineFlags.setUp(InstrumentationRegistry.getTargetContext(), mFlagSet); > On 2017/03/21 at 14:56:50, jbudorick wrote: > > Remind me again (and add a comment explaining) why we can't run this in the Statement returned by apply? > > we actually can do have apply() return a statement that just call this > > But there is no guarantee this rule will run before activity test rule, if the activity test rule were to automatically launches the activity. > > Maybe we can do some class runner check that makes sure CommandLineTestRule always run before ActivityTestRule? Added javadoc explanation
yolandyan@chromium.org changed reviewers: + agrieve@chromium.org
+agrieve for base/ OWNER stamp
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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
Implementation of this CL changed from when previous CL was stamp, PTAL |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
