|
|
Created:
4 years ago by the real yoland Modified:
3 years, 4 months ago CC:
bsheedy, chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate Next Gen Parameter Test Framework for JUnit4
This test framework enables developers to write parameterized tests with simple
static field. The framework allows both test class parameterization (through
constructor arguments injection) and test method parameterization (through
test method arguments injection).
For more detail on how to write parameterized tests using this framework, please
check the crbug or ExampleParameterizedTest.java file.
BUG=673092
Review-Url: https://codereview.chromium.org/2568633002
Cr-Commit-Position: refs/heads/master@{#493605}
Committed: https://chromium.googlesource.com/chromium/src/+/fafc82e1442b3a6d2c2a7b9e364e30b76647322a
Patch Set 1 #
Total comments: 42
Patch Set 2 : Add Unit test and changes after +jbudorick comment #Patch Set 3 : Add Unit test and changes after +jbudorick comment #
Total comments: 24
Patch Set 4 : Add test name and description #Patch Set 5 : Address +mikecase comments and enforce acceptible types #
Total comments: 44
Patch Set 6 : name change/comments after +jbudorick #
Total comments: 42
Patch Set 7 : Remove InnerParameterizedRunner and refactor after +mikecase and +jbudorick's comments #
Total comments: 82
Patch Set 8 : Remove all builders, change naming style #
Total comments: 83
Patch Set 9 : Address +jbudorick's comments #Patch Set 10 : Clear syntax error #Patch Set 11 : Fix findbug errors #
Total comments: 39
Patch Set 12 : address comments #Patch Set 13 : find bug issue #
Total comments: 2
Patch Set 14 : Limit to only one class parameter set #
Total comments: 24
Patch Set 15 : Address comments #
Total comments: 32
Patch Set 16 : address issues #
Total comments: 3
Patch Set 17 : add callable and assertion message #Patch Set 18 : rebase #Messages
Total messages: 89 (42 generated)
yolandyan@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
Sorry for the Friday evening CL! I had a lot of fun creating this one, really eager to share. I will write documentation on Monday and unit tests. And next step would be better name support, enable parameterized rules, and then really start to convert tests to JUnit4! yeahhh
This CL appears only partially complete given the number of incomplete comments, TODOs, minor syntax issues, etc, so this is mostly a design (and naming) review. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java:1: package org.chromium.base.test; Copyrights plz Also, why is this not in org.chromium.base.test.params? https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4ForwardingClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4ForwardingClassRunner.java:34: @Override Why is this overriding BlockJUnit4ClassRunner's implementation if it's just calling it? https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamGroup.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamGroup.java:9: public class ImmutableParamGroup { ImmutableParamGroup isn't a ParamGroup? https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java:10: public class ImmutableParamSet { This isn't a Set and thus shouldn't be named like one. Also, this isn't a ParamSet? https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:24: * InnerParamClassRunner contains a forwarding class runner that can be defined by user in a test The forwarding class runner is better described as a runner delegate. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:30: public class InnerParamClassRunner<T extends ParamTestForwardingRunner> I'm not sure I agree with the relationship between this and mParamTestForwardingRunner. As it is, this class handles child creation and description, while the contained runner handles execution. I'm wondering if it might be cleaner if we instead had BlockJUnit4ForwardingClassRunner delegate child creation and description to this and handle execution on its own. I don't think you'd need to have BJU4FCR set instance variables in this class in that circumstance. Users who wanted to customize would still be able to subclass BJU4FCR. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:34: private final T mParamTestForwardingRunner; mRunnerDelegate https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:62: } else { nit: Since you're returning in the if block, drop the else block to reduce nesting, i.e. if (map.isEmpty()) { return ... } List<ParameterizedFrameworkMethod> returnList = new ArrayList<>(); ... https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:86: if (map.get(group) == null) { Can this be true? https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java:12: public class ParamClassRunnerFactory { Does this need to exist outside of ParamRunner? https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamFrameworkMethod.java:33: static List<ParamFrameworkMethod> convertFrameworkMethod( This should be named s.t. it conveys that it's operating on and returning a list of FrameworkMethods and ParameterizedFrameworkMethods, respectively. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:22: A, B, C, D, E, F, G, H, I, J, K, L, M, N, O, P, Q, R, S, T; I don't think arbitrarily lettered parameter groups are going to make for readable code. Is there a more descriptive way we can label groups? Can we let test authors name their owner MethodParameter groups w/ strings? https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:32: public @interface Param {} "Param" and "SubParam" aren't very intuitive. What about "ClassParameter" and "MethodParameter"?
https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java:1: package org.chromium.base.test; On 2016/12/12 at 19:04:48, jbudorick wrote: > Copyrights plz > > > Also, why is this not in org.chromium.base.test.params? Hmm, ya, changed https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4ForwardingClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4ForwardingClassRunner.java:34: @Override On 2016/12/12 at 19:04:48, jbudorick wrote: > Why is this overriding BlockJUnit4ClassRunner's implementation if it's just calling it? Because the interface defined it. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamGroup.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamGroup.java:9: public class ImmutableParamGroup { On 2016/12/12 at 19:04:48, jbudorick wrote: > ImmutableParamGroup isn't a ParamGroup? Similar to ParamSet, This is NOT a ParamGroup because ParamGroup is actually a builder for ImmutableParamGroup. This is really optimized to abstract the implementation detail away from those who write tests and make it easier for them. What I think might work better is name ImmutableParamGroup/Set something completely different, like ArgumentGroup/Set? https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java:10: public class ImmutableParamSet { On 2016/12/12 at 19:04:48, jbudorick wrote: > This isn't a Set and thus shouldn't be named like one. > > Also, this isn't a ParamSet? This is NOT a ParamSet because ParamSet is actually a builder for ImmutableParamSet (a bit confusing for sure) but an immutable list of immutable object is required for parameterized tests (or else, test 1's interaction with object may change test 2), so ParamSet/ParamGroup are just used as builder to create ImmutableParamSet/ImmutableParamGroup. They are not named as ParamSetBuilder because I thought it might confuse people who write tests. In terms of naming, maybe just Parameter? ParameterGroup is a group of parameter, and an actual individual argument in parameter can be referred as Parameter element in docs? https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:24: * InnerParamClassRunner contains a forwarding class runner that can be defined by user in a test On 2016/12/12 at 19:04:48, jbudorick wrote: > The forwarding class runner is better described as a runner delegate. Done https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:30: public class InnerParamClassRunner<T extends ParamTestForwardingRunner> On 2016/12/12 at 19:04:49, jbudorick wrote: > I'm not sure I agree with the relationship between this and mParamTestForwardingRunner. As it is, this class handles child creation and description, while the contained runner handles execution. I'm wondering if it might be cleaner if we instead had BlockJUnit4ForwardingClassRunner delegate child creation and description to this and handle execution on its own. I don't think you'd need to have BJU4FCR set instance variables in this class in that circumstance. Users who wanted to customize would still be able to subclass BJU4FCR. I agree, the relationship between this and mRunnerDelegate isn't very clean, this mainly due to the fact we want to use delegate runner at the first place (official junit parameterized version of this class simply inherit from BlockJUnit4). My idea of how customization is done is 1. if your customization has nothing to do with parameterizing process, do it in the runner delegate or its parent. (e.g. Straight up make changes in BaseJUnit4ClassRunner if that's your runner delegate's parent) 2. if user want to modify how the parameter are processed (e.g change test creation), they can actually subclass this class and make class runner factory create their version of this class I added more graph in the design doc on this topic https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:34: private final T mParamTestForwardingRunner; On 2016/12/12 at 19:04:49, jbudorick wrote: > mRunnerDelegate Done https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:62: } else { On 2016/12/12 at 19:04:48, jbudorick wrote: > nit: Since you're returning in the if block, drop the else block to reduce nesting, i.e. > > if (map.isEmpty()) { > return ... > } > > List<ParameterizedFrameworkMethod> returnList = new ArrayList<>(); > ... Done https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:86: if (map.get(group) == null) { On 2016/12/12 at 19:04:48, jbudorick wrote: > Can this be true? It happens, if someone's test mistakenly has @UseMethodParameter("A") without providing method parameter group that is annotated with "A" value at all, e.g. public TestClass { @MethodParameter("B") static ParamGroup GROUP_A = ...; @Test @UseMethodParameter("A") public void simpleTest(...) {...}; } https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java:12: public class ParamClassRunnerFactory { On 2016/12/12 at 19:04:49, jbudorick wrote: > Does this need to exist outside of ParamRunner? I was actually hoping to merge this with InnerParamClassRunner.java so and make InnerParamClassRunner private so people don't use it. If we don't worry so much that people would use InnerParamClassRunner by accident, and we don't need another inner class runner in near future, we don't really need to the factory at all, we instantiate the inner class runner in ParamRunner.java https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamFrameworkMethod.java:33: static List<ParamFrameworkMethod> convertFrameworkMethod( On 2016/12/12 at 19:04:49, jbudorick wrote: > This should be named s.t. it conveys that it's operating on and returning a list of FrameworkMethods and ParameterizedFrameworkMethods, respectively. changed to wrapAllFrameworkMethods https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:22: A, B, C, D, E, F, G, H, I, J, K, L, M, N, O, P, Q, R, S, T; On 2016/12/12 at 19:04:49, jbudorick wrote: > I don't think arbitrarily lettered parameter groups are going to make for readable code. Is there a more descriptive way we can label groups? Can we let test authors name their owner MethodParameter groups w/ strings? Ya, we can use either integers or strings Change to String for now https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:32: public @interface Param {} On 2016/12/12 at 19:04:49, jbudorick wrote: > "Param" and "SubParam" aren't very intuitive. What about "ClassParameter" and "MethodParameter"? Done
Have a bunch of old comments. Taking a break. Will finish reviewing this afternoon. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java:82: * } nit: } https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamGroup.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamGroup.java:7: * Created by yolandyan on 12/5/16. nit: Probably should change this docstring https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java:8: * Created by yolandyan on 12/5/16. nit: Probably should change this. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java:24: return new ArrayList<>(); Does it matter that this is not an unmodifiable list? My guess is it doesnt matter. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:98: , group.toString())); nit: this indenting looks off. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamTestForwardingRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamTestForwardingRunner.java:7: * Created by yolandyan on 12/7/16. nit: probably want to update htis. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:17: * Created by yolandyan on 12/5/16. nit: update https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:59: mWidth, Arrays.toString(objects), objects.length)); nit: this indenting looks wrong. I think mWidth should just be lined up with line above. https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:27: * ParamGroup ParamRunner look for {@code @ClassParameter} annotation in test I think you are missing a period after ParamGroup? https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:30: * annotation, and create a map that points GROUP to its corresponding What is GROUP? https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java (right): https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:31: @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.FIELD) ? https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:65: objects = new Object[1]; I dont think I understand this. Should this be "new Object[0]"? https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:68: ParamSet set = new StandardParamSetBuilder(objects); ParamSet vs ParamGroup naming I think is confusing. https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:124: // string, etc) and callable If someone passes a Junit Rule object here, this should give a warning as we discussed.
How does the RunnerDelegate get specified? Can you add a second Param to @RunWith(ParamRunner.class, MyDelegate.class)? Also, it might be overkill to allow multiple @ClassParameter param groups. It seems to make things quite a bit more complex for not a ton of benefit imo. Still, could be sweet for some use cases like using different combinations of Rules (and running w/ & w/o WebView multiprocess for example) https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ExampleBlockJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ExampleBlockJUnit4RunnerDelegate.java:27: * Override this to not validate in runner delegate, validation is already done by ParamRunner Comment is confusing to read at first. Would change s/Override/"Am overriding" https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:26: * runner and should NEVER be used directly InnerParamClassRunner contains a period after "directly". https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:82: + "parameters but there isn't any test that uses there parameters", indentations looks off. https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:151: + " the required parameters are %s", This indentation looks off. https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:116: Field[] allFields = testClass.getDeclaredFields(); The doc for getDeclaredFields() says.. " The elements in the array returned are not sorted and are not in any particular order. " Im assuming you are depending on their order since if you do like.... @ClassParameter some int param @ClassParameter some string param public TestClass(int blah, String blaaah) And need to feed the params to the constructor in the correct order. Just FYI. It seems to work according to you so maybe it is no big deal. https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:251: reminder = reminder % mOffsets[i - 1]; should this be called remainder?
https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java:82: * } On 2016/12/14 at 19:25:31, mikecase wrote: > nit: } Done https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamGroup.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamGroup.java:7: * Created by yolandyan on 12/5/16. On 2016/12/14 at 19:25:31, mikecase wrote: > nit: Probably should change this docstring Done https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java:8: * Created by yolandyan on 12/5/16. On 2016/12/14 at 19:25:31, mikecase wrote: > nit: Probably should change this. Done https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ImmutableParamSet.java:24: return new ArrayList<>(); On 2016/12/14 at 19:25:31, mikecase wrote: > Does it matter that this is not an unmodifiable list? My guess is it doesnt matter. Changed to collections.emptyList() https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:98: , group.toString())); On 2016/12/14 at 19:25:31, mikecase wrote: > nit: this indenting looks off. Changed https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamTestForwardingRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamTestForwardingRunner.java:7: * Created by yolandyan on 12/7/16. On 2016/12/14 at 19:25:31, mikecase wrote: > nit: probably want to update htis. Changed https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:17: * Created by yolandyan on 12/5/16. On 2016/12/14 at 19:25:31, mikecase wrote: > nit: update Done https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:59: mWidth, Arrays.toString(objects), objects.length)); On 2016/12/14 at 19:25:31, mikecase wrote: > nit: this indenting looks wrong. I think mWidth should just be lined up with line above. Done https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ExampleBlockJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ExampleBlockJUnit4RunnerDelegate.java:27: * Override this to not validate in runner delegate, validation is already done by ParamRunner On 2016/12/15 at 01:47:49, mikecase wrote: > Comment is confusing to read at first. > > Would change s/Override/"Am overriding" Changed https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:26: * runner and should NEVER be used directly InnerParamClassRunner contains a On 2016/12/15 at 01:47:49, mikecase wrote: > period after "directly". Done https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:82: + "parameters but there isn't any test that uses there parameters", On 2016/12/15 at 01:47:49, mikecase wrote: > indentations looks off. Done https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:151: + " the required parameters are %s", On 2016/12/15 at 01:47:49, mikecase wrote: > This indentation looks off. Done https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:27: * ParamGroup ParamRunner look for {@code @ClassParameter} annotation in test On 2016/12/14 at 19:25:31, mikecase wrote: > I think you are missing a period after ParamGroup? Done https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:30: * annotation, and create a map that points GROUP to its corresponding On 2016/12/14 at 19:25:31, mikecase wrote: > What is GROUP? Changed https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:116: Field[] allFields = testClass.getDeclaredFields(); On 2016/12/15 at 01:47:49, mikecase wrote: > The doc for getDeclaredFields() says.. > > " The elements in the array returned are not sorted and are not in any particular order. " > > Im assuming you are depending on their order since if you do like.... > > @ClassParameter > some int param > > @ClassParameter > some string param > > public TestClass(int blah, String blaaah) > > And need to feed the params to the constructor in the correct order. Just FYI. It seems to work according to you so maybe it is no big deal. Added todo https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:251: reminder = reminder % mOffsets[i - 1]; On 2016/12/15 at 01:47:49, mikecase wrote: > should this be called remainder? shhh, bby, is k https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java (right): https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:31: @Retention(RetentionPolicy.RUNTIME) On 2016/12/14 at 19:25:31, mikecase wrote: > @Target(ElementType.FIELD) ? Done https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:65: objects = new Object[1]; On 2016/12/14 at 19:25:31, mikecase wrote: > I dont think I understand this. > > Should this be "new Object[0]"? This is because ParamSet are a list of the parameters, and if null is the single argument, it's not the paramset being null, it's the only element in that list being null, the length of that list is 1. paramset = [null] Changed to not allow null here, and an additional method: addSingleNull() https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:68: ParamSet set = new StandardParamSetBuilder(objects); On 2016/12/14 at 19:25:32, mikecase wrote: > ParamSet vs ParamGroup naming I think is confusing. waht, no way, everyone understood this perfectly so far https://codereview.chromium.org/2568633002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:124: // string, etc) and callable On 2016/12/14 at 19:25:31, mikecase wrote: > If someone passes a Junit Rule object here, this should give a warning as we discussed. Added, also add check to make sure this is only primitive types and its wrappers
This is going to sound stupid, but we need to fix the names in here before reviewing this any more. The naming is confusing to the point that it makes it *more* difficult to review than it otherwise would be. Names of classes, functions, and variables should reflect what they do. Things that do different things should be named differently; things that do similar things should be named similarly. This is still a partial review. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:29: * InnerParamClassRunner contains a class runner delegate that can be defined by user in a test This should be broken into sentences. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:43: private final List<ImmutableParamSet> mClassParameterGroup; ParamSet ... ParameterGroup? https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:73: private List<ParamFrameworkMethod> initializeFrameworkMethod( nit: this should be renamed. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:74: Map<String, ImmutableParamGroup> map, String postFix) { "map" is insufficiently descriptive for a function as complex as this one. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:95: for (String groupString : map.keySet()) { Can you just iterator over the entrySet and then check if entry.getValue() is null? https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:102: if (map.get(groupString) == null) { You should call map.get(groupString) once per iteration of the outer loop, not twice in the middle loop. This entire error detection block should be moved outside the middle loop as well. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:117: returnList.add(new ParamFrameworkMethod(method.getMethod(), set, postFix)); This is also doing a Cartesian product, this time of methods annotated with a given MethodParameter and ParameterSets within the ParameterSetList for that MethodParameter. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:149: mRunnerDelegate.setTest(testInstance); I still dislike this and think the delegate relationship should be inverted. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java:19: public <T extends ParamRunnerDelegate> Runner createRunner(Class<?> testClass, nit: drop the first param onto its own line https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java:23: throws ParamRunnerDelegateInstantiationException, InitializationError { nit: I'm not sure about having this line at the same indentation depth as the parameters. It's hard to distinguish between the two. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:97: * Generate a map of String to a list of method ParamGroup annotated with This is hard to understand. It should be something like: Returns a map between MethodParameter names and corresponding ParameterSetLists. (where ParameterSetList is a possible name for ImmutableParamGroup) https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:113: * Generate a list of all the ParamGroup annotated with @ClassParameter Similarly, this should be something like: Returns a list containing a ParameterSetList for each ClassParameter. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:130: protected static Map<Class<? extends Annotation>, List<Field>> parseTestClassFields( nit: this should precede the two generate* methods. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:192: private static Iterator<List<ImmutableParamSet>> combineParameterSets( I'm not a math major (ahem), but I think this is calculating the Cartesian product of the ClassParameter ParameterSetLists. It should be named slightly differently -- "combine" is a bit too generic. Case, any ideas? https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:223: static class ParamSetCombinationIterator implements Iterator<List<ImmutableParamSet>> { It would seem that this could be implemented more cleanly with multiple iterators, one for each parameter, rather than integer offsets. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:47: public @interface RuleParam {} nit: RuleParameter https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:67: "All ParamSets in a ParamGroup must have equal length, there " nit: All <name> in a <name> must have equal length. The current <name> (%s) contains %d parameters, while previous <name>s contain %d parameters. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:78: throw new IllegalArgumentException("null can not be passed in as a single argument" This seems unwieldy. Why can't we make a single argument null add a single null parameter set? https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:107: List<ImmutableParamSet> copy = new ArrayList<>(); nit: This isn't a copy of mParamList and thus shouldn't be named as though it is one. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:166: private static final Set<Class<?>> ACCEPTIBLE_TYPES = getAcceptibleTypes(); nit: acceptable
Best renamings I could think of are something along the lines of .... ParameterSet -> ParameterSet ImmutableParamGroup -> ParameterSetList ParamGroup -> ParameterSetListBuilder ParamRunner -> ParameterizedRunner ParamFrameworkMethod -> ParameterizedFrameworkMethod or with s/ParameterSet/Parameters/ Naming is hard... :/ https://codereview.chromium.org/2568633002/diff/80001/base/test/android/junit... File base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/junit... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java:188: + Arrays.toString(expectedTestNames.toArray()), nit Looks like wrong indentation. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/junit... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java:215: + Arrays.toString(expectedTestNames.toArray()), nit: looks like wrong indentation.
Best renamings I could think of are something along the lines of .... ParameterSet -> ParameterSet ImmutableParamGroup -> ParameterSetList ParamGroup -> ParameterSetListBuilder ParamRunner -> ParameterizedRunner ParamFrameworkMethod -> ParameterizedFrameworkMethod or with s/ParameterSet/Parameters/ Naming is hard... :/
https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:29: * InnerParamClassRunner contains a class runner delegate that can be defined by user in a test On 2017/01/03 at 23:03:00, jbudorick wrote: > This should be broken into sentences. Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:43: private final List<ImmutableParamSet> mClassParameterGroup; On 2017/01/03 at 23:03:00, jbudorick wrote: > ParamSet ... ParameterGroup? Changed https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:73: private List<ParamFrameworkMethod> initializeFrameworkMethod( On 2017/01/03 at 23:03:00, jbudorick wrote: > nit: this should be renamed. Changed to generateParameterizedFrameworkMethod https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:74: Map<String, ImmutableParamGroup> map, String postFix) { On 2017/01/03 at 23:03:00, jbudorick wrote: > "map" is insufficiently descriptive for a function as complex as this one. Change to tagToListOfParameterSet https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:95: for (String groupString : map.keySet()) { On 2017/01/03 at 23:03:00, jbudorick wrote: > Can you just iterator over the entrySet and then check if entry.getValue() is null? Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:102: if (map.get(groupString) == null) { On 2017/01/03 at 23:03:00, jbudorick wrote: > You should call map.get(groupString) once per iteration of the outer loop, not twice in the middle loop. This entire error detection block should be moved outside the middle loop as well. Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:117: returnList.add(new ParamFrameworkMethod(method.getMethod(), set, postFix)); On 2017/01/03 at 23:03:00, jbudorick wrote: > This is also doing a Cartesian product, this time of methods annotated with a given MethodParameter and ParameterSets within the ParameterSetList for that MethodParameter. I added this as a comment for better explanation https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:149: mRunnerDelegate.setTest(testInstance); On 2017/01/03 at 23:03:00, jbudorick wrote: > I still dislike this and think the delegate relationship should be inverted. Hmm I m not following on this idea, will check with you in person https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java:19: public <T extends ParamRunnerDelegate> Runner createRunner(Class<?> testClass, On 2017/01/03 at 23:03:00, jbudorick wrote: > nit: drop the first param onto its own line Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamClassRunnerFactory.java:23: throws ParamRunnerDelegateInstantiationException, InitializationError { On 2017/01/03 at 23:03:00, jbudorick wrote: > nit: I'm not sure about having this line at the same indentation depth as the parameters. It's hard to distinguish between the two. Changed https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:97: * Generate a map of String to a list of method ParamGroup annotated with On 2017/01/03 at 23:03:01, jbudorick wrote: > This is hard to understand. It should be something like: > > Returns a map between MethodParameter names and corresponding ParameterSetLists. > > (where ParameterSetList is a possible name for ImmutableParamGroup) Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:113: * Generate a list of all the ParamGroup annotated with @ClassParameter On 2017/01/03 at 23:03:01, jbudorick wrote: > Similarly, this should be something like: > > Returns a list containing a ParameterSetList for each ClassParameter. Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:130: protected static Map<Class<? extends Annotation>, List<Field>> parseTestClassFields( On 2017/01/03 at 23:03:01, jbudorick wrote: > nit: this should precede the two generate* methods. Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:192: private static Iterator<List<ImmutableParamSet>> combineParameterSets( On 2017/01/03 at 23:03:01, jbudorick wrote: > I'm not a math major (ahem), but I think this is calculating the Cartesian product of the ClassParameter ParameterSetLists. It should be named slightly differently -- "combine" is a bit too generic. Case, any ideas? Change names to calculateClassParameterSetsCartesianProduct https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamRunner.java:223: static class ParamSetCombinationIterator implements Iterator<List<ImmutableParamSet>> { On 2017/01/03 at 23:03:01, jbudorick wrote: > It would seem that this could be implemented more cleanly with multiple iterators, one for each parameter, rather than integer offsets. Hmm, have to check with you again on this. https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:47: public @interface RuleParam {} On 2017/01/03 at 23:03:01, jbudorick wrote: > nit: RuleParameter Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:67: "All ParamSets in a ParamGroup must have equal length, there " On 2017/01/03 at 23:03:01, jbudorick wrote: > nit: > > All <name> in a <name> must have equal length. The current <name> (%s) contains %d parameters, while previous <name>s contain %d parameters. Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:78: throw new IllegalArgumentException("null can not be passed in as a single argument" On 2017/01/03 at 23:03:01, jbudorick wrote: > This seems unwieldy. Why can't we make a single argument null add a single null parameter set? True...must be brain dead that day changed https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:107: List<ImmutableParamSet> copy = new ArrayList<>(); On 2017/01/03 at 23:03:01, jbudorick wrote: > nit: This isn't a copy of mParamList and thus shouldn't be named as though it is one. Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/params/ParamUtils.java:166: private static final Set<Class<?>> ACCEPTIBLE_TYPES = getAcceptibleTypes(); On 2017/01/03 at 23:03:01, jbudorick wrote: > nit: acceptable Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/junit... File base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/junit... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java:188: + Arrays.toString(expectedTestNames.toArray()), On 2017/01/05 at 16:50:02, mikecase wrote: > nit Looks like wrong indentation. Done https://codereview.chromium.org/2568633002/diff/80001/base/test/android/junit... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java:215: + Arrays.toString(expectedTestNames.toArray()), On 2017/01/05 at 16:50:02, mikecase wrote: > nit: looks like wrong indentation. Done
Looks much better and is way more readable. Partial review. Mostly spelling nit. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:28: * Validation is already done by ParameterizeRunner nit: ParameterizedRunner (missing d) https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetBuilder.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetBuilder.java:8: * ParamterSetBuilder is a builder for ImmutableParamterSetBuilder the nit: ImmutableParamterSetBuilder spelled wrong https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetBuilder.java:10: * ParamterSetBuilder is element for {@code ParameterSetListBuilder}, each ParamterSetBuilder spelled wrong. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:31: * ParameterizedRunner generate a list to runners for testClass with nit: list *of* runners ? https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:32: * ParameterSetList. ParameterizedRunner look for {@code @ClassParameter} nit: ParameterizedRunner look*s* https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:33: * annotation in test class and generate a list of InnerParameterizedRunner nit: generate*s* https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:34: * runners for each ParamterSetBuilder in the test class The class runner also ParamterSetBuilder spelled wrong. Also, should it be for each ParameterSet? Seems to be weird to generate a list of runners for each ParameterSetBuilder? Or at least that sounds strange. nit: class*.* https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:35: * looks for @MethodParameter annotation, and create a map that points String nit: create*s* https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:36: * value (group) to its corresponding ParameterSetListBuilder. nit: confusing wording. Maybe... creates a map that maps Strings to ParameterSetListBuilders. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:66: errors.add(new Exception(gripe)); nit: maybe just combine the above two lines? https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:83: errors.add(new Exception(gripe)); nit: consider combining above two lines? https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:89: * ParameterSetListBuilder annotated by @ClassParameter I really think having Builder in the documentation makes everything sounds more confusing. IDK, but maybe lie and just write like Return a list of runners for each ParamaterSet in the ParameterSetLists annotated by @ClassParameter. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:101: Map<String, ParameterSetList> testMethodParameterSetListBuilderMap = testMethodParameterSetListBuilderMap. wooooooo, thats a doozy of a name. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:121: * {@code @UseRunnerDelegate} is used The default runner delegate is nit: used*.* https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:156: "This test class: %s, does not have @ClassParameter or " Is there magic that replaces this %s with the method parameter? https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:203: if (!Modifier.isStatic(field.getModifiers())) throw new IllegalParameterArgumentException(); nit: consider putting message in Exception saying what went wrong.
I mentioned this to case offline, but we're going to have to do some more cleanup on the comments. What you've got so far in that regard is a good start, but with how complex this is, we need the comments to be very clear. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit: 2017 now https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/InnerParameterizedRunner.java:38: public class InnerParameterizedRunner<T extends ParameterizedRunnerDelegate> Spoke about delegate inversion offline. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/InnerParameterizedRunner.java:59: Collections.unmodifiableMap(subParameterMap), testMethodPostfix); Should the caller be passing in an unmodifiable map rather than this creating one here? https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/InnerParameterizedRunner.java:142: new ParameterizedFrameworkMethod(method.getMethod(), set, postFix)); ParameterizedFrameworkMethod knows about the method parameters here. Can we also pass the class parameters to it & eliminate some of the createTest/setTest logic below? https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:16: import org.chromium.base.test.params.InnerParameterizedRunner.ParameterizedRunnerDelegateInstantiationException; o_O You probably should be importing the outer class, not the inner class. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:38: public final class ParameterizedRunner extends Suite { I'm worried about how much this is duplicating from BlockJUnit4ClassRunner. How much validation does this need to do? Can it instead be done by the contained runners?
https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/11 at 19:57:32, jbudorick wrote: > nit: 2017 now lol, done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:28: * Validation is already done by ParameterizeRunner On 2017/01/11 at 19:07:49, mikecase wrote: > nit: ParameterizedRunner (missing d) Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/InnerParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/InnerParameterizedRunner.java:38: public class InnerParameterizedRunner<T extends ParameterizedRunnerDelegate> On 2017/01/11 at 19:57:32, jbudorick wrote: > Spoke about delegate inversion offline. Removed InnerParameterizedRunner entirely https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/InnerParameterizedRunner.java:59: Collections.unmodifiableMap(subParameterMap), testMethodPostfix); On 2017/01/11 at 19:57:32, jbudorick wrote: > Should the caller be passing in an unmodifiable map rather than this creating one here Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/InnerParameterizedRunner.java:142: new ParameterizedFrameworkMethod(method.getMethod(), set, postFix)); On 2017/01/11 at 19:57:32, jbudorick wrote: > ParameterizedFrameworkMethod knows about the method parameters here. Can we also pass the class parameters to it & eliminate some of the createTest/setTest logic below? great idea! createTest/setTest logic is eliminated https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:16: import org.chromium.base.test.params.InnerParameterizedRunner.ParameterizedRunnerDelegateInstantiationException; On 2017/01/11 at 19:57:32, jbudorick wrote: > o_O > > You probably should be importing the outer class, not the inner class. Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:31: * ParameterizedRunner generate a list to runners for testClass with On 2017/01/11 at 19:07:49, mikecase wrote: > nit: list *of* runners ? Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:32: * ParameterSetList. ParameterizedRunner look for {@code @ClassParameter} On 2017/01/11 at 19:07:50, mikecase wrote: > nit: ParameterizedRunner look*s* Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:33: * annotation in test class and generate a list of InnerParameterizedRunner On 2017/01/11 at 19:07:50, mikecase wrote: > nit: generate*s* Am I getting a D for this class? Mr.Case :( https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:34: * runners for each ParamterSetBuilder in the test class The class runner also On 2017/01/11 at 19:07:50, mikecase wrote: > ParamterSetBuilder spelled wrong. Also, should it be for each ParameterSet? Seems to be weird to generate a list of runners for each ParameterSetBuilder? Or at least that sounds strange. > > nit: class*.* Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:35: * looks for @MethodParameter annotation, and create a map that points String On 2017/01/11 at 19:07:50, mikecase wrote: > nit: create*s* Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:36: * value (group) to its corresponding ParameterSetListBuilder. On 2017/01/11 at 19:07:50, mikecase wrote: > nit: confusing wording. > > Maybe... > > creates a map that maps Strings to ParameterSetListBuilders. Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:38: public final class ParameterizedRunner extends Suite { On 2017/01/11 at 19:57:32, jbudorick wrote: > I'm worried about how much this is duplicating from BlockJUnit4ClassRunner. How much validation does this need to do? Can it instead be done by the contained runners? Actually, the runner delegate completely override the validation process and does not do anything. This way it's more efficient because each runner delegate are actually validating the same test class. More importantly, current implementation requires runner delegate to override their validation method because things allowed in parameterized tests (e.g. test method have arguments, constructor having arguments) are not allowed in BlockJUnit4ClassRunner. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:66: errors.add(new Exception(gripe)); On 2017/01/11 at 19:07:50, mikecase wrote: > nit: maybe just combine the above two lines? Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:83: errors.add(new Exception(gripe)); On 2017/01/11 at 19:07:50, mikecase wrote: > nit: consider combining above two lines? Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:89: * ParameterSetListBuilder annotated by @ClassParameter On 2017/01/11 at 19:07:50, mikecase wrote: > I really think having Builder in the documentation makes everything sounds more confusing. IDK, but maybe lie and just write like > > Return a list of runners for each ParamaterSet in the ParameterSetLists annotated by @ClassParameter. Changed https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:101: Map<String, ParameterSetList> testMethodParameterSetListBuilderMap = On 2017/01/11 at 19:07:50, mikecase wrote: > testMethodParameterSetListBuilderMap. wooooooo, thats a doozy of a name. lol, it's not even correct changed https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:121: * {@code @UseRunnerDelegate} is used The default runner delegate is On 2017/01/11 at 19:07:50, mikecase wrote: > nit: used*.* I blame stupid eclipse auto formating, never again. Done https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:156: "This test class: %s, does not have @ClassParameter or " On 2017/01/11 at 19:07:49, mikecase wrote: > Is there magic that replaces this %s with the method parameter? lool, nope, changed https://codereview.chromium.org/2568633002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:203: if (!Modifier.isStatic(field.getModifiers())) throw new IllegalParameterArgumentException(); On 2017/01/11 at 19:07:49, mikecase wrote: > nit: consider putting message in Exception saying what went wrong. Done
Now ParameterizedFactory just instantiate a new custom ParameterizedRunnerDelegate.
This is the diff between patch 5 and 6: https://codereview.chromium.org/2568633002/diff2/100001:120001/base/test/andr... Biggest change for patch 6: moved things in InnerParameterizedRunner to ParameterizedFactory: https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java...
On 2017/01/12 at 06:23:32, the real yoland wrote: > This is the diff between patch 5 and 6: https://codereview.chromium.org/2568633002/diff2/100001:120001/base/test/andr... It's actually 6 and 7 > > Biggest change for patch 6: moved things in InnerParameterizedRunner to ParameterizedFactory: 7* > https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java...
All nits. Sending out what I have now https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:30: * Override this to NOT CALL SUPER's method, running the super's collectInitializationErrors SUPER's method. Running the.... https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:31: * would cause super's collectInitializationErrors is called in constructor and the overriden the grammar seems to be off in this sentence. Something like this maybe? (not sure I understood sentence correctly) would cause super's collectInitializationErrors to be called in the constrictor and then fail due to the overridden computeTestMethod relying on a local member variable ???? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java:30: * would cause super's collectInitializationErrors is called in constructor and the overriden do same types of fixes I suggested above. Sentence is a bit confusing. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetBuilder.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetBuilder.java:8: * ParamterSetBuilder is a builder for ImmutableParamterSetBuilder the ImmutableParamterSetBuilder, dont think this is a thing anymore https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetListBuilder.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetListBuilder.java:8: * ParameterSetListBuilder is a builder for ImmutableParameterSetListBuilder The builder for ParameterSetList? IIRC you removed Immutable from class names. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetListBuilder.java:10: * {@code ImmutableParamterSetBuilder} that are used to store arguments for ImmutableParamterSetBuilder https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:58: // TODO(yolandyan) add annotation from the ImmutableParamterSet, enable ImmutableParamterSet is not a thing anymore I dont think https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:95: * @throws ParameterizedRunnerFactory.ParameterizedTestInstanciationException if the test class Instanciation is mispelled. Should be Instantiation. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:202: * only ParameterSetListBuilder I dont understand this sentence. Is it... Validate that all fields annotated by parameter related annotation are ParameterSetListBuilders https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:218: * Return an Iterator of lists of ImmutableParamterSetBuilder Each list of ImmutableParamterSetBuilder, I dont think this is a thing anymore https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:246: // empty list result Maybe combine above two lines? Also "a" should be "an" https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:267: * Create a iterator when there are multiple ParameterSetListBuilder in a an interator https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:268: * test class and Cartesian product of all ParamterSetBuilder ParamterSetBuilder spelled wrong https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:298: "Element in ParamterSetBuilderCombinationIterator should " ParamterSetBuilderCombinationIterator parameter spelled wrong https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:20: * computeTestMethod rely on a local member variable Same comment as other uses of this comment. Might want to reword it. I have suggestion somewhere above. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:65: * Match test methods that is annotated with @UseMethodParameter(X) with "is" should be "are" https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:67: * builder field Sentence is kinda confusing. Might reword to.... Math test methods annotated by @UseMethodParameter(X) with ParameterSetList annotated by @MethodParameter(X) in their builder field. Not much different but maybe slightly clearer? idk https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:70: * @param tagToParameterSetList a map that maps String tag X to its No need for X in comment if you dont use X at least twice. Would reword to A map of String tags to ParameterSetList
partial. Need to go back over ParameterizedRunner and ParameterizedRunnerFactory. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:30: * Override this to NOT CALL SUPER's method, running the super's collectInitializationErrors On 2017/01/13 16:50:43, mikecase wrote: > SUPER's method. Running the.... WHY ARE WE YELLING https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java:33: * Validation needed for parameterized test is already done by ParameterizedRunner, The validation needed for parameterized tests is already done by ParameterizedRunner. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:14: public class ParameterSet { In light of the fate of ParameterSetList, I'm also wondering if this needs to exist independently. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:31: // TODO(yolandyan): add @NonNull When do you plan to do this? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetBuilder.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetBuilder.java:8: * ParamterSetBuilder is a builder for ImmutableParamterSetBuilder the The comment is far too complicated. It should just be something like "Builder for ParameterSets." Parameter ^ you have forgotten the e in a few of these lines, too. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetList.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetList.java:14: public class ParameterSetList { This class doesn't need to exist. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetListBuilder.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetListBuilder.java:8: * ParameterSetListBuilder is a builder for ImmutableParameterSetListBuilder The Again, this comment is way too complicated. Just "Builder for ParameterSet." is fine. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterUtil.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterUtil.java:28: public class ParameterUtil { Why does this class exist? Why aren't its contents in their own files? Is there a good reason to hide the builder implementations? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:16: * Custom FrameworkMethod that includes an {@code ParameterSet} that nit: a ParameterSet https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:17: * represent the parameters for this test method nits: - represent -> represents - end with . https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:19: class ParameterizedFrameworkMethod extends FrameworkMethod { is package scope correct here? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:23: ParameterizedFrameworkMethod(Method method, ParameterSet parameterSet, String postFix) { same question https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:27: mName = method.getName() + postFix + parameterSet.getName(); Does parameterSet need to have its own name? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:14: * InnerParameterizedRunner to execute tests outdated https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:18: * Override this to NOT CALL SUPER's method, running the super's collectInitializationErrors Why is this method in this interface...? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:25: List<FrameworkMethod> computeTestMethods(); These two methods should have javadoc comments. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:26: public class ParameterizedRunnerFactory { "ParameterizedRunnerFactory" implies that this is creating ParameterizedRunners, which is not the case. Maybe "ParameterizedRunnerDelegateFactory"? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:75: private List<ParameterizedFrameworkMethod> generateParameterizedFrameworkMethod( This should be renamed to reflect the fact that it's not generating a single method, it's generating a list of them. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:84: List<ParameterizedFrameworkMethod> returnList = new ArrayList<>(); nit: +1 blank line after this one https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:119: // TODO(yolandyan): confirm this is correct syntax at the end eh? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:151: // TODO(yolandyan): add exception message eh? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:161: // TODO(yolandyan): add link to page for how to extends create custom parameterized eh? https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:166: super(String.format("Current class runner delegate: %s, can not be instantiated.", too much punctuation here. get rid of the : and the , delegate % can not be instantiated https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:172: public static class ParameterizedTestInstanciationException extends Exception { Instanciation -> Instantiation
I changed the naming style as John brought up that having the parameterized test look just like normal test can cause problems, I changed it from `testHello` to `test__Hello` https://codereview.chromium.org/2568633002/diff2/120001:140001/base/test/andr... https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:30: * Override this to NOT CALL SUPER's method, running the super's collectInitializationErrors On 2017/01/13 at 21:55:59, jbudorick wrote: > On 2017/01/13 16:50:43, mikecase wrote: > > SUPER's method. Running the.... > > WHY ARE WE YELLING I DON'T KNOW :( https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:31: * would cause super's collectInitializationErrors is called in constructor and the overriden On 2017/01/13 at 16:50:43, mikecase wrote: > the grammar seems to be off in this sentence. Something like this maybe? (not sure I understood sentence correctly) > > > would cause super's collectInitializationErrors to be called in the constrictor and then fail due to > the overridden computeTestMethod relying on a local member variable > > ???? Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java:30: * would cause super's collectInitializationErrors is called in constructor and the overriden On 2017/01/13 at 16:50:43, mikecase wrote: > do same types of fixes I suggested above. Sentence is a bit confusing. Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java:33: * Validation needed for parameterized test is already done by ParameterizedRunner, On 2017/01/13 at 21:55:59, jbudorick wrote: > The validation needed for parameterized tests is already done by ParameterizedRunner. Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:14: public class ParameterSet { On 2017/01/13 at 21:55:59, jbudorick wrote: > In light of the fate of ParameterSetList, I'm also wondering if this needs to exist independently. Well this actually has more jobs, it stores its own name. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:31: // TODO(yolandyan): add @NonNull On 2017/01/13 at 21:55:59, jbudorick wrote: > When do you plan to do this? 10...9..8..7...6.5. wait, deleted https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetBuilder.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetBuilder.java:8: * ParamterSetBuilder is a builder for ImmutableParamterSetBuilder the On 2017/01/13 at 21:55:59, jbudorick wrote: > The comment is far too complicated. It should just be something like > > "Builder for ParameterSets." > > Parameter > ^ > you have forgotten the e in a few of these lines, too. Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetList.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetList.java:14: public class ParameterSetList { On 2017/01/13 at 21:55:59, jbudorick wrote: > This class doesn't need to exist. Changed https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetListBuilder.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetListBuilder.java:8: * ParameterSetListBuilder is a builder for ImmutableParameterSetListBuilder The On 2017/01/13 at 16:50:43, mikecase wrote: > builder for ParameterSetList? > > IIRC you removed Immutable from class names. Deleted this file No more builders https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSetListBuilder.java:10: * {@code ImmutableParamterSetBuilder} that are used to store arguments for On 2017/01/13 at 16:50:43, mikecase wrote: > ImmutableParamterSetBuilder Deleted https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterUtil.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterUtil.java:28: public class ParameterUtil { On 2017/01/13 at 21:55:59, jbudorick wrote: > Why does this class exist? Why aren't its contents in their own files? Is there a good reason to hide the builder implementations? Completely changed this https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:16: * Custom FrameworkMethod that includes an {@code ParameterSet} that On 2017/01/13 at 21:56:00, jbudorick wrote: > nit: a ParameterSet Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:17: * represent the parameters for this test method On 2017/01/13 at 21:55:59, jbudorick wrote: > nits: > - represent -> represents > - end with . Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:19: class ParameterizedFrameworkMethod extends FrameworkMethod { On 2017/01/13 at 21:56:00, jbudorick wrote: > is package scope correct here? Changed https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:23: ParameterizedFrameworkMethod(Method method, ParameterSet parameterSet, String postFix) { On 2017/01/13 at 21:56:00, jbudorick wrote: > same question Changed https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:27: mName = method.getName() + postFix + parameterSet.getName(); On 2017/01/13 at 21:56:00, jbudorick wrote: > Does parameterSet need to have its own name? Yes, it's useful to distinguish different tests. e.g. @MethodParameter static ParamterSetListBuilder builder = ... static { builder.addParameterSet(...).setName("Abc") builder.addParameterSet(...).setName("Xyz") } @Test public void testThreeLetterWord() {...} this would generate testThreadLetterWordAbc, testThreadLetterWordXyz https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:58: // TODO(yolandyan) add annotation from the ImmutableParamterSet, enable On 2017/01/13 at 16:50:43, mikecase wrote: > ImmutableParamterSet is not a thing anymore I dont think Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:95: * @throws ParameterizedRunnerFactory.ParameterizedTestInstanciationException if the test class On 2017/01/13 at 16:50:43, mikecase wrote: > Instanciation is mispelled. Should be Instantiation. Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:202: * only ParameterSetListBuilder On 2017/01/13 at 16:50:43, mikecase wrote: > I dont understand this sentence. > > Is it... > > Validate that all fields annotated by parameter related annotation are ParameterSetListBuilders Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:218: * Return an Iterator of lists of ImmutableParamterSetBuilder Each list of On 2017/01/13 at 16:50:43, mikecase wrote: > ImmutableParamterSetBuilder, I dont think this is a thing anymore Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:246: // empty list result On 2017/01/13 at 16:50:43, mikecase wrote: > Maybe combine above two lines? > > Also "a" should be "an" Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:267: * Create a iterator when there are multiple ParameterSetListBuilder in a On 2017/01/13 at 16:50:43, mikecase wrote: > an interator pls, its iterator. loool Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:268: * test class and Cartesian product of all ParamterSetBuilder On 2017/01/13 at 16:50:43, mikecase wrote: > ParamterSetBuilder spelled wrong Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:298: "Element in ParamterSetBuilderCombinationIterator should " On 2017/01/13 at 16:50:43, mikecase wrote: > ParamterSetBuilderCombinationIterator > > parameter spelled wrong Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:14: * InnerParameterizedRunner to execute tests On 2017/01/13 at 21:56:00, jbudorick wrote: > outdated Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:18: * Override this to NOT CALL SUPER's method, running the super's collectInitializationErrors On 2017/01/13 at 21:56:00, jbudorick wrote: > Why is this method in this interface...? Force people to override it, I guess when the people is mainly us... lol https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:20: * computeTestMethod rely on a local member variable On 2017/01/13 at 16:50:43, mikecase wrote: > Same comment as other uses of this comment. Might want to reword it. I have suggestion somewhere above. Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:25: List<FrameworkMethod> computeTestMethods(); On 2017/01/13 at 21:56:00, jbudorick wrote: > These two methods should have javadoc comments. Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:26: public class ParameterizedRunnerFactory { On 2017/01/13 at 21:56:00, jbudorick wrote: > "ParameterizedRunnerFactory" implies that this is creating ParameterizedRunners, which is not the case. Maybe "ParameterizedRunnerDelegateFactory"? Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:65: * Match test methods that is annotated with @UseMethodParameter(X) with On 2017/01/13 at 16:50:43, mikecase wrote: > "is" should be "are" Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:67: * builder field On 2017/01/13 at 16:50:43, mikecase wrote: > Sentence is kinda confusing. Might reword to.... > > > Math test methods annotated by @UseMethodParameter(X) with > ParameterSetList annotated by @MethodParameter(X) in their > builder field. > > Not much different but maybe slightly clearer? idk Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:70: * @param tagToParameterSetList a map that maps String tag X to its On 2017/01/13 at 16:50:43, mikecase wrote: > No need for X in comment if you dont use X at least twice. > > Would reword to > > A map of String tags to ParameterSetList Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:75: private List<ParameterizedFrameworkMethod> generateParameterizedFrameworkMethod( On 2017/01/13 at 21:56:00, jbudorick wrote: > This should be renamed to reflect the fact that it's not generating a single method, it's generating a list of them. Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:84: List<ParameterizedFrameworkMethod> returnList = new ArrayList<>(); On 2017/01/13 at 21:56:00, jbudorick wrote: > nit: +1 blank line after this one Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:119: // TODO(yolandyan): confirm this is correct syntax at the end On 2017/01/13 at 21:56:00, jbudorick wrote: > eh? oh, because the syntas of these things changes throughout the refactoring process, this is to remind myself to correct the exception message's code syntax. Maybe I m using TODO wrong, my TODOs are for both current CL and future changes https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:151: // TODO(yolandyan): add exception message On 2017/01/13 at 21:56:00, jbudorick wrote: > eh? lol, okay, maybe it's just my english https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:161: // TODO(yolandyan): add link to page for how to extends create custom parameterized On 2017/01/13 at 21:56:00, jbudorick wrote: > eh? ...hmm, maybe it's my brain deleted https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:166: super(String.format("Current class runner delegate: %s, can not be instantiated.", On 2017/01/13 at 21:56:00, jbudorick wrote: > too much punctuation here. get rid of the : and the , > > delegate % can not be instantiated Done https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:172: public static class ParameterizedTestInstanciationException extends Exception { On 2017/01/13 at 21:56:00, jbudorick wrote: > Instanciation -> Instantiation Done
I did not review the tests. There are two things that really stand out to me in this review: - there are a lot of TODOs that appear to be for you to address in this CL. - there are multiple implementations that appear to be either unnecessarily complex or insufficiently documented. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerFactory.java:119: // TODO(yolandyan): confirm this is correct syntax at the end On 2017/01/26 00:49:12, the real yoland wrote: > On 2017/01/13 at 21:56:00, jbudorick wrote: > > eh? > > oh, because the syntas of these things changes throughout the refactoring > process, this is to remind myself > to correct the exception message's code syntax. > > Maybe I m using TODO wrong, my TODOs are for both current CL and future changes TODOs are fine in both cases, but you should expect comments if any of the "current CL" TODOs are present in a patchset. My "eh"s here were just because your TODOs looked like "current CL" TODOs and I wanted to make sure that you addressed them one way or another. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java:16: public class BlockJUnit4RunnerDelegate This is BaseJUnit4RunnerDelegate with a different base class. Can we combine the two? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:27: public ParameterSet(Object... values) { I'm not sure about a variadic constructor here vs something like public ParameterSet() {} public ParameterSet values(Object... values) { mRawValues = Arrays.asList(values); } https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:31: public ParameterSet appendName(String name) { I think this should just be name, not appendName https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:39: public static ParameterSet create(Object... objects) { w/ the above changes, I think this would be unnecessary. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:47: // TODO(yolandyan): make sure this is a defensive copy Is this a current CL TODO or a future CL TODO? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:83: List<Object> returnActualParameterList() { So this essentially freezes mRawValues as mList? I'm curious, what's the use case here? It looks like most of the uses could be replaced by a toArray method... Also, this name is overkill. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:58: // TODO(yolandyan) add annotation from the ParameterSet, enable Again -- current CL TODO, or future? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:41: private static final List<Runner> NO_RUNNERS = Collections.<Runner>emptyList(); Is this necessary when you only use it in the constructor...? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:52: // TODO(yolandyan): refactor to utilize TestClass and FrameworkField Current / future? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:63: // TODO(yolandyan): more validations, e.g. validate tags in annotations are matching Current / future? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:69: private List<FrameworkMethod> computeTestMethods() { Why is this so far away from the only place where it's used? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:81: * Return a list of runners for each class parameter set list This comment should more extensively explain what's going on here. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:92: ParameterizedRunnerDelegateFactory.ParameterizedTestInstantiationException, Do these two need to be inside ParameterizedRunnerDelegateFactory? These types are really unwieldy... https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:96: Map<String, List<ParameterSet>> tagToMethodParameterSetList = You should move this down closer to where it's used. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:116: * Parse all fields in test class and find the ones annotated by parameter rather than saying "parameter related annotations," list the annotations -- @ClassParameter, @MethodParameter, etc https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:121: Field[] allFields = testClass.getDeclaredFields(); nit: why is this in between declaring result and adding ClassParameter and MethodParameter to result? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:138: "This test class: %s, does not have @ClassParameter or " "%s has no fields annotated with @ClassParameter or @MethodParameter. It should not use ParameterizedRunner." https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:151: throws IllegalAccessException { I don't think this should be at the same indentation level as the arguments above. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:171: result.add(getParameterSetList(field, testClass)); isn't this result.add(parameterSetList)? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:179: @SuppressWarnings("unchecked") ? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:183: throw new IllegalParameterArgumentException("ParameterSetList fields must be static"); This exception text should mention the fields that aren't static. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:186: throw new IllegalArgumentException("Fields with @ClassParameter or @MethodParameter " This exception text should mention the fields that aren't lists. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:229: return DEFAULT_RUNNER_DELEGATE; I'm not sure that having this be a constant is worthwhile. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:233: * Return an Iterator of lists of ParamterSet. ParamterSet -> ParameterSet https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:261: * [1, "b"], [2, "a"], [2, "b"], [3, "a"], [3, "b"] and this iterator would Delete the second part of this sentence starting with "and this iterator" https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:294: static class ParameterSetListCartesianProductIterator implements Iterator<List<ParameterSet>> { Can this be private? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:307: pointers[i] = parameterSetLists.get(parameterAmount - i - 1).size(); I don't understand what you're doing here. Why does pointers correspond to parameterSetLists reversed? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:310: int[] offsets = new int[parameterAmount - 1]; Why are there offsets for all but one parameterSetList? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:320: throw new IllegalStateException( This should be an UnsupportedOperationException: https://docs.oracle.com/javase/7/docs/api/java/util/Iterator.html#remove() https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:331: public List<ParameterSet> next() { Let's talk about this. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:354: public IllegalParameterArgumentException(int size, ParameterSet set) { Don't do this. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:20: public interface ParameterizedRunnerDelegate { I'm still not sure why this exists as an interface. ParameterizedRunnerDelegateFactory makes objects of type org.junit.runner.Runner, not ParameterizedRunnerDelegates (though they're ParameterizedRunnerDelegates in practice). https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:22: * Override this to an empty method and NOT call super's method. This seems like a kludge. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:27: public <T extends ParameterizedRunnerDelegate> Runner createRunner(TestClass testClass, javadoc https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:31: throws ParameterizedTestInstantiationException, indentation https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:34: List<ParameterizedFrameworkMethod> parameterizedFrameworkMethodList = Why are this and castedFrameworkMethodList separate? Why isn't this just List<FrameworkMethod> methodList = generateListOfParameterizedFrameworkMethod(...); ? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:46: Assert.assertTrue(String.format("Runner delegate's class: %s must extends from Runner", nit: Just "%s must extend from org.junit.runner.Runner" should be fine. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:49: return (Runner) runnerDelegate; It's a little concerning that you need to cast this. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:74: private List<ParameterizedFrameworkMethod> generateListOfParameterizedFrameworkMethod( This could use a better name. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:108: // methods by using a Cartesian product of testMethod with tag X and This is less of a cartesian product scenario than the other one. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:119: // TODO(yolandyan): confirm this is correct syntax at the end current / future https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:142: public Object createTest(TestClass testClass, List<ParameterSet> listOfClassParameters) Was there a presubmit warning for this not having a javadoc? (also, this should have a javadoc comment) https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:169: public static class ParameterizedTestInstantiationException extends Exception { This needs a javadoc comment. Also, as mentioned in another file, can these exceptions be moved out of ParameterizedRunnerDelegateFactory?
https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java:16: public class BlockJUnit4RunnerDelegate On 2017/02/03 at 16:48:11, jbudorick wrote: > This is BaseJUnit4RunnerDelegate with a different base class. Can we combine the two? BlockJUnit4RunnerDelegate exists so that normal junit test can use it too (e.g. ExampleParameterizedTest.java), while BaseJUnit4RunnerDelegate extends from BaseJUnit4ClassRunner, and would have operations involving Android code (e.g. InstrumentationRegistry), so normal junit 4 tests can't really use it. I merged the implementation of method to a ParameterizedRunnerDelegateCommon class https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:27: public ParameterSet(Object... values) { On 2017/02/03 at 16:48:11, jbudorick wrote: > I'm not sure about a variadic constructor here vs something like > > public ParameterSet() {} > public ParameterSet values(Object... values) { > mRawValues = Arrays.asList(values); > } Make sense, no need for create either. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:31: public ParameterSet appendName(String name) { On 2017/02/03 at 16:48:11, jbudorick wrote: > I think this should just be name, not appendName Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:39: public static ParameterSet create(Object... objects) { On 2017/02/03 at 16:48:11, jbudorick wrote: > w/ the above changes, I think this would be unnecessary. Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:47: // TODO(yolandyan): make sure this is a defensive copy On 2017/02/03 at 16:48:11, jbudorick wrote: > Is this a current CL TODO or a future CL TODO? Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:83: List<Object> returnActualParameterList() { On 2017/02/03 at 16:48:11, jbudorick wrote: > So this essentially freezes mRawValues as mList? I'm curious, what's the use case here? It looks like most of the uses could be replaced by a toArray method... > > Also, this name is overkill. I think I was afraid that if this just returns an the original value list, someone would be able to do something like (ActivityTestRule)returnActualParameterList()[0].getActivity().mess something up. That might be a bit far fetched though Also this whole process is wrong, it should be validating and making copy when setting the values Changed https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:58: // TODO(yolandyan) add annotation from the ParameterSet, enable On 2017/02/03 at 16:48:12, jbudorick wrote: > Again -- current CL TODO, or future? future, remove all finished TODOs https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:41: private static final List<Runner> NO_RUNNERS = Collections.<Runner>emptyList(); On 2017/02/03 at 16:48:12, jbudorick wrote: > Is this necessary when you only use it in the constructor...? Changed https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:52: // TODO(yolandyan): refactor to utilize TestClass and FrameworkField On 2017/02/03 at 16:48:12, jbudorick wrote: > Current / future? Future https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:63: // TODO(yolandyan): more validations, e.g. validate tags in annotations are matching On 2017/02/03 at 16:48:13, jbudorick wrote: > Current / future? Removed https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:69: private List<FrameworkMethod> computeTestMethods() { On 2017/02/03 at 16:48:12, jbudorick wrote: > Why is this so far away from the only place where it's used? Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:81: * Return a list of runners for each class parameter set list On 2017/02/03 at 16:48:12, jbudorick wrote: > This comment should more extensively explain what's going on here. Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:92: ParameterizedRunnerDelegateFactory.ParameterizedTestInstantiationException, On 2017/02/03 at 16:48:12, jbudorick wrote: > Do these two need to be inside ParameterizedRunnerDelegateFactory? These types are really unwieldy... I can separate them into individual files? but they are only thrown in RunnerDelegateFactory. Or I can declare here that this method throws Exception https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:96: Map<String, List<ParameterSet>> tagToMethodParameterSetList = On 2017/02/03 at 16:48:12, jbudorick wrote: > You should move this down closer to where it's used. Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:116: * Parse all fields in test class and find the ones annotated by parameter On 2017/02/03 at 16:48:12, jbudorick wrote: > rather than saying "parameter related annotations," list the annotations -- @ClassParameter, @MethodParameter, etc Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:121: Field[] allFields = testClass.getDeclaredFields(); On 2017/02/03 at 16:48:12, jbudorick wrote: > nit: why is this in between declaring result and adding ClassParameter and MethodParameter to result? Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:138: "This test class: %s, does not have @ClassParameter or " On 2017/02/03 at 16:48:12, jbudorick wrote: > "%s has no fields annotated with @ClassParameter or @MethodParameter. It should not use ParameterizedRunner." Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:151: throws IllegalAccessException { On 2017/02/03 at 16:48:12, jbudorick wrote: > I don't think this should be at the same indentation level as the arguments above. Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:171: result.add(getParameterSetList(field, testClass)); On 2017/02/03 at 16:48:12, jbudorick wrote: > isn't this result.add(parameterSetList)? Yes, done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:179: @SuppressWarnings("unchecked") On 2017/02/03 at 16:48:13, jbudorick wrote: > ? Moved to below https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:183: throw new IllegalParameterArgumentException("ParameterSetList fields must be static"); On 2017/02/03 at 16:48:12, jbudorick wrote: > This exception text should mention the fields that aren't static. Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:186: throw new IllegalArgumentException("Fields with @ClassParameter or @MethodParameter " On 2017/02/03 at 16:48:12, jbudorick wrote: > This exception text should mention the fields that aren't lists. Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:229: return DEFAULT_RUNNER_DELEGATE; On 2017/02/03 at 16:48:12, jbudorick wrote: > I'm not sure that having this be a constant is worthwhile. I guess being more specific is better. Removed constant and add throws exception here if @UseRunnerDelegate is not specified https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:233: * Return an Iterator of lists of ParamterSet. On 2017/02/03 at 16:48:12, jbudorick wrote: > ParamterSet -> ParameterSet Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:261: * [1, "b"], [2, "a"], [2, "b"], [3, "a"], [3, "b"] and this iterator would On 2017/02/03 at 16:48:12, jbudorick wrote: > Delete the second part of this sentence starting with "and this iterator" Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:294: static class ParameterSetListCartesianProductIterator implements Iterator<List<ParameterSet>> { On 2017/02/03 at 16:48:12, jbudorick wrote: > Can this be private? but making it private would make this line more than 100 char lol, jk, changed https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:320: throw new IllegalStateException( On 2017/02/03 at 16:48:12, jbudorick wrote: > This should be an UnsupportedOperationException: https://docs.oracle.com/javase/7/docs/api/java/util/Iterator.html#remove() Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:354: public IllegalParameterArgumentException(int size, ParameterSet set) { On 2017/02/03 at 16:48:12, jbudorick wrote: > Don't do this. curious why this is a bad thing to do? https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:20: public interface ParameterizedRunnerDelegate { On 2017/02/03 at 16:48:13, jbudorick wrote: > I'm still not sure why this exists as an interface. ParameterizedRunnerDelegateFactory makes objects of type org.junit.runner.Runner, not ParameterizedRunnerDelegates (though they're ParameterizedRunnerDelegates in practice). The purpose of this is basically to require all runner delegate to override these methods (like a contract). https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:22: * Override this to an empty method and NOT call super's method. On 2017/02/03 at 16:48:13, jbudorick wrote: > This seems like a kludge. ya, unfortunately, I can't think of a way around it. BlockJUnitClassRunner does all kinds of validations so a parameterized test isn't allowed. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:27: public <T extends ParameterizedRunnerDelegate> Runner createRunner(TestClass testClass, On 2017/02/03 at 16:48:13, jbudorick wrote: > javadoc Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:31: throws ParameterizedTestInstantiationException, On 2017/02/03 at 16:48:13, jbudorick wrote: > indentation Done, stupid eclipse https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:34: List<ParameterizedFrameworkMethod> parameterizedFrameworkMethodList = On 2017/02/03 at 16:48:13, jbudorick wrote: > Why are this and castedFrameworkMethodList separate? Why isn't this just > > List<FrameworkMethod> methodList = generateListOfParameterizedFrameworkMethod(...); > > ? Changed https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:46: Assert.assertTrue(String.format("Runner delegate's class: %s must extends from Runner", On 2017/02/03 at 16:48:13, jbudorick wrote: > nit: Just "%s must extend from org.junit.runner.Runner" should be fine. Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:49: return (Runner) runnerDelegate; On 2017/02/03 at 16:48:13, jbudorick wrote: > It's a little concerning that you need to cast this. Changed to better generics https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:74: private List<ParameterizedFrameworkMethod> generateListOfParameterizedFrameworkMethod( On 2017/02/03 at 16:48:13, jbudorick wrote: > This could use a better name. changed to generateUnmodifiableFrameworkMethodList https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:108: // methods by using a Cartesian product of testMethod with tag X and On 2017/02/03 at 16:48:13, jbudorick wrote: > This is less of a cartesian product scenario than the other one. Changed. https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:119: // TODO(yolandyan): confirm this is correct syntax at the end On 2017/02/03 at 16:48:13, jbudorick wrote: > current / future Removed https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:142: public Object createTest(TestClass testClass, List<ParameterSet> listOfClassParameters) On 2017/02/03 at 16:48:13, jbudorick wrote: > Was there a presubmit warning for this not having a javadoc? > > (also, this should have a javadoc comment) Done https://codereview.chromium.org/2568633002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:169: public static class ParameterizedTestInstantiationException extends Exception { On 2017/02/03 at 16:48:13, jbudorick wrote: > This needs a javadoc comment. > > Also, as mentioned in another file, can these exceptions be moved out of ParameterizedRunnerDelegateFactory? Move to ParameterizedRunner.java
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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...
Patchset #11 (id:200001) has been deleted
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
friendly ping :)
Once again, I'm sorry about the delay here. I missed that this was ready for re-review. I didn't review the tests in this pass. I'll try to do so tonight or tomorrow. I think this is getting close, though. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:20: * A set of parameters that are used as parameters for a single test class or a We should clarify this to indicate that it's a set of parameters used in a single *invocation* of a test. As-is, that's not clear, and people could easily think that this is instead the set of all values that a single parameter can take on in all invocations of a test. (I had to look at the example to figure out which one was right.) https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:59: + "an immutable class that is accepted, accepted types are" nit: saying that parameterized tests don't support classes that are not of a type that is accepted isn't a very helpful error message. Maybe something like "Type \"%s\" is not supported in parameterized testing at this time. Accepted types include all primitive types along with " + Arrays.toString(...) https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:67: public int hashCode() { You should explain this implementation in a comment here. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:38: * value to ParameterSetList ParameterSetList -> List<ParameterSet> https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:77: * For class parameter set: each class can have one or more lists of class parameter set. nit: list of class parameter *sets* https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:78: * When it has multiple list of parameter set, a Cartesian product of these parameter multiple *lists* of parameter *sets* https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:79: * set is created. One runner delegate is generated for one list of parameter set in *sets* is created https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:85: * Then 6 list of parameter set will be created in the end A list of 6 parameter sets will be created... https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:87: * So 6 runner delegates will be create and 6 runner delegates will be created. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:91: * passed into factory for each runner delegate to create multiple tests Add a sentence to the end of this that explicitly says that that only one Runner will be created for a method that uses @MethodParameter, regardless of the number of ParameterSets in the associated list. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:292: list.add(Collections.<ParameterSet>emptyList()); neat, I didn't know about emptyList. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:325: int[] pointers = new int[parameterAmount]; This could use some implementation comments. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:369: static class IllegalParameterArgumentException extends IllegalArgumentException { Why are these exceptions package scope? https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:13: * This is an interface to define all the Delegate runners that extends I'm not sure what you mean w/ the part that precedes the comma here. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:18: * create constr What happened to the rest of this sentence? https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:99: String currentGroup = method.getAnnotation(UseMethodParameter.class).value(); Can a method use multiple method parameters? https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:110: // Loop through each the tags and create all the parameterized framework nit: each *of* the tags https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:117: throw new IllegalArgumentException(String.format("Test class provide tag %s " nit: "Test class provides parameters with tag %s but no test uses them." Also, if we have the test name, we should include that in the exception message. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:122: throw new IllegalStateException(String.format("Tag %1$s method parameter " similarly, nit: No parameters with tag %s provided [by class %s]. Please create a static variable... That said, this error message is great -- both actionable & specific.
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...
:] https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:20: * A set of parameters that are used as parameters for a single test class or a On 2017/06/06 at 16:22:58, jbudorick wrote: > We should clarify this to indicate that it's a set of parameters used in a single *invocation* of a test. As-is, that's not clear, and people could easily think that this is instead the set of all values that a single parameter can take on in all invocations of a test. (I had to look at the example to figure out which one was right.) Done, added an example. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:59: + "an immutable class that is accepted, accepted types are" On 2017/06/06 at 16:22:58, jbudorick wrote: > nit: saying that parameterized tests don't support classes that are not of a type that is accepted isn't a very helpful error message. Maybe something like > > "Type \"%s\" is not supported in parameterized testing at this time. Accepted types include all primitive types along with " + Arrays.toString(...) Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:67: public int hashCode() { On 2017/06/06 at 16:22:58, jbudorick wrote: > You should explain this implementation in a comment here. Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:38: * value to ParameterSetList On 2017/06/06 at 16:22:58, jbudorick wrote: > ParameterSetList -> List<ParameterSet> Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:77: * For class parameter set: each class can have one or more lists of class parameter set. On 2017/06/06 at 16:22:58, jbudorick wrote: > nit: list of class parameter *sets* Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:78: * When it has multiple list of parameter set, a Cartesian product of these parameter On 2017/06/06 at 16:22:58, jbudorick wrote: > multiple *lists* of parameter *sets* Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:79: * set is created. One runner delegate is generated for one list of parameter set in On 2017/06/06 at 16:22:59, jbudorick wrote: > *sets* is created Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:85: * Then 6 list of parameter set will be created in the end On 2017/06/06 at 16:22:59, jbudorick wrote: > A list of 6 parameter sets will be created... Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:87: * So 6 runner delegates will be create On 2017/06/06 at 16:22:59, jbudorick wrote: > and 6 runner delegates will be created. Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:91: * passed into factory for each runner delegate to create multiple tests On 2017/06/06 at 16:22:58, jbudorick wrote: > Add a sentence to the end of this that explicitly says that that only one Runner will be created for a method that uses @MethodParameter, regardless of the number of ParameterSets in the associated list. Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:292: list.add(Collections.<ParameterSet>emptyList()); On 2017/06/06 at 16:22:59, jbudorick wrote: > neat, I didn't know about emptyList. hehe, neither do I now. glad to learn again, lol https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:325: int[] pointers = new int[parameterAmount]; On 2017/06/06 at 16:22:59, jbudorick wrote: > This could use some implementation comments. Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:369: static class IllegalParameterArgumentException extends IllegalArgumentException { On 2017/06/06 at 16:22:58, jbudorick wrote: > Why are these exceptions package scope? For testing purpose, @Test(expected = IllegalParameterArgumentException.class) https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:13: * This is an interface to define all the Delegate runners that extends On 2017/06/06 at 16:22:59, jbudorick wrote: > I'm not sure what you mean w/ the part that precedes the comma here. Changed https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java:18: * create constr On 2017/06/06 at 16:22:59, jbudorick wrote: > What happened to the rest of this sentence? Changed https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:99: String currentGroup = method.getAnnotation(UseMethodParameter.class).value(); On 2017/06/06 at 16:22:59, jbudorick wrote: > Can a method use multiple method parameters? nope, the element value in @UseMethodParameter is a single String. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:110: // Loop through each the tags and create all the parameterized framework On 2017/06/06 at 16:22:59, jbudorick wrote: > nit: each *of* the tags Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:117: throw new IllegalArgumentException(String.format("Test class provide tag %s " On 2017/06/06 at 16:22:59, jbudorick wrote: > nit: "Test class provides parameters with tag %s but no test uses them." > > Also, if we have the test name, we should include that in the exception message. Done Done https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:122: throw new IllegalStateException(String.format("Tag %1$s method parameter " On 2017/06/06 at 16:22:59, jbudorick wrote: > similarly, nit: No parameters with tag %s provided [by class %s]. Please create a static variable... > > That said, this error message is great -- both actionable & specific. Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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.
Need to look at the test classes. But this looks good as is. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:51: if (o.getClass().isPrimitive() || ACCEPTABLE_TYPES.contains(o.getClass())) { o.getClass().isPrimitive() ^ this check isn't neccessary i dont think. I don't think its possible for primitive types to be in the values list. https://codereview.chromium.org/2568633002/diff/260001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:33: * sAllParameterSets.add(new ParameterSet().value("c", "d"); sAllParameterSets.add(new ParameterSet().value("c", "d")) https://codereview.chromium.org/2568633002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:110: ret.add(Void.class); alphabetiae nit https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterAnnotations.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterAnnotations.java:49: * Annotation for test class, it specifies which ParameteriezRunnerDelegate to use. ParameteriezRunnerDelegate https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:28: * To parameterize testSimple or MyTestClass's tests, creat multiple ParameterSets creat https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:28: * To parameterize testSimple or MyTestClass's tests, creat multiple ParameterSets creat https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:47: return super.invokeExplosively(target, params); I dont understand why this if statement exists. Could you explain. https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:49: super(klass, Collections.<Runner>emptyList()); // pass in emtpy list of runners emtpy https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:82: } Is this just for performance? Because I think you can just remove this. Also, if you keep, should you be passing testClass.getAnnotatedMethods(Test.class) and not testClass.getAnnotatedMethods()? https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:118: + "there parameters", those parameters or their parameters https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:158: classParameterSet.getValues().toArray()); woah. No type info is passed in here? Constructor.newInstance just takes list of objects? TIL https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:186: */ Some stranded docstrings here.
https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:127: + " ParameterUtil.createList<ParameterSet>();%n" This doc also might need to be updated? Does ParameterUtil exist? https://codereview.chromium.org/2568633002/diff/280001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/params/ParameterSetValidationTest.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterSetValidationTest.java:16: * Test for org.chromium.base.test.params.ParameterUtil Test for ParameterSet? ParameterUtil doesnt seem to exist anymore/ https://codereview.chromium.org/2568633002/diff/280001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java:130: "There names were provided: %s, these expected names are not found: %s", Their
lgtm after fixing nits + the 1 actual comment I had. https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:44: if (params.length == 0 && mParameterSet != null) { remove check for params.length == 0
https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterAnnotations.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterAnnotations.java:49: * Annotation for test class, it specifies which ParameteriezRunnerDelegate to use. On 2017/07/21 at 15:56:02, mikecase wrote: > ParameteriezRunnerDelegate Done https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:28: * To parameterize testSimple or MyTestClass's tests, creat multiple ParameterSets On 2017/07/21 at 15:56:02, mikecase wrote: > creat Don https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:44: if (params.length == 0 && mParameterSet != null) { On 2017/07/21 at 17:40:57, mikecase wrote: > remove check for params.length == 0 Done https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:47: return super.invokeExplosively(target, params); On 2017/07/21 at 15:56:02, mikecase wrote: > I dont understand why this if statement exists. Could you explain. explained offline: params exists here to be consist with junit4's support https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:49: super(klass, Collections.<Runner>emptyList()); // pass in emtpy list of runners On 2017/07/21 at 15:56:02, mikecase wrote: > emtpy I guess it's a british english thing then Done https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:82: } On 2017/07/21 at 15:56:03, mikecase wrote: > Is this just for performance? Because I think you can just remove this. > > Also, if you keep, should you be passing testClass.getAnnotatedMethods(Test.class) and not testClass.getAnnotatedMethods()? Done https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:118: + "there parameters", On 2017/07/21 at 15:56:03, mikecase wrote: > those parameters > or > their parameters Done https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:127: + " ParameterUtil.createList<ParameterSet>();%n" On 2017/07/21 at 16:44:57, mikecase wrote: > This doc also might need to be updated? Does ParameterUtil exist? Done https://codereview.chromium.org/2568633002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:186: */ On 2017/07/21 at 15:56:03, mikecase wrote: > Some stranded docstrings here. Done https://codereview.chromium.org/2568633002/diff/280001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/params/ParameterSetValidationTest.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterSetValidationTest.java:16: * Test for org.chromium.base.test.params.ParameterUtil On 2017/07/21 at 16:44:57, mikecase wrote: > Test for ParameterSet? ParameterUtil doesnt seem to exist anymore/ Merged this test with ParameterizedRunnerTest.java https://codereview.chromium.org/2568633002/diff/280001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java:130: "There names were provided: %s, these expected names are not found: %s", On 2017/07/21 at 16:44:57, mikecase wrote: > Their Done
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.
friendly ping, PTAL :)
Looks very good. Will be an LG once you address ParameterizedRunnerDelegateFactoryTest https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:65: @SuppressFBWarnings("NP_LOAD_OF_KNOWN_NULL_VALUE") ? https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:67: List<Object> deepCopy = new ArrayList<>(); nit: name this something other than deepCopy. It's not a deep copy, and its depth isn't really relevant to what it's doing. https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:45: return super.invokeExplosively(target, mParameterSet.getValues().toArray()); What happens to params in this case? Is it ok that we drop them? https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:86: // TODO(yolandyan) add annotation from the ParameterSet, enable super nit: colon after TODO(yolandyan): ^ https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:87: // test writing to add SkipCheck for an individual parameter also, this would be a pretty slick feature, though I agree with your decision to put it off to another CL. https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:37: private static final String TAG = "ParameterizedRunner"; nit: should be prefixed w/ "cr_" https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:60: * overriden and validate is done seperately. nit: overridden, and validation https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:63: protected void collectInitializationErrors(List<Throwable> errors) { Should this collect errors from validation? https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:73: List<FrameworkMethod> returnList = new ArrayList<>(); nit: declare this after the tagToListOfFrameworkMethod creation loop https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:105: + "parameters with tag %s. But there isn't any test that uses " nit: "parameter with tag \"%s\", but no test uses those parameters." https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:110: throw new IllegalStateException(String.format("No parameters with tag %1$s " nit: "No parameters with tag %1$s are provided by class %2$s." I would omit the code sample from the exception text. If you'd like to reference it in code, drop a comment here. https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:163: runnerDelegateClass, e.getMessage()), Do you need to include e's message here if you're also including e as the cause? Seems like it might be redundant. https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:20: * Test for org.chromium.base.test.params.InnerParamClassRunner nit: update class name https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:23: public class ParameterizedRunnerDelegateFactoryTest { Given the complexity of the logic in ParameterizedRunnerDelegateFactory, I think this ought to cover more. Specifically, I think it ought to have more extensive tests for generateUnmodifiableFrameworkMethodList. https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:53: public void testBadExampleRunnerDelegate() throws Throwable { This test name should reflect *why* BadExampleRunnerDelegate should throw an exception, not just that it does.
Patchset #16 (id:320001) has been deleted
Patchset #16 (id:340001) has been deleted
Diff from last patch: https://codereview.chromium.org/2568633002/diff2/300001:360001/base/test/andr... https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:65: @SuppressFBWarnings("NP_LOAD_OF_KNOWN_NULL_VALUE") On 2017/08/08 at 14:44:26, jbudorick wrote: > ? Removed, this is because the line `deepCopy.add(o)`, changed to `deepCopy.add(null)` https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:67: List<Object> deepCopy = new ArrayList<>(); On 2017/08/08 at 14:44:26, jbudorick wrote: > nit: name this something other than deepCopy. It's not a deep copy, and its depth isn't really relevant to what it's doing. Done https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:45: return super.invokeExplosively(target, mParameterSet.getValues().toArray()); On 2017/08/08 at 14:44:26, jbudorick wrote: > What happens to params in this case? Is it ok that we drop them? I looked into this, JUnit4 actually do not pass any params object into invokeExplosively in any of their runner. However, they have some code under experimental that would pass some argument into a method. However, that would not affect this. https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:86: // TODO(yolandyan) add annotation from the ParameterSet, enable On 2017/08/08 at 14:44:26, jbudorick wrote: > super nit: colon after TODO(yolandyan): > ^ Done https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:87: // test writing to add SkipCheck for an individual parameter On 2017/08/08 at 14:44:26, jbudorick wrote: > also, this would be a pretty slick feature, though I agree with your decision to put it off to another CL. Ack https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:37: private static final String TAG = "ParameterizedRunner"; On 2017/08/08 at 14:44:26, jbudorick wrote: > nit: should be prefixed w/ "cr_" Done https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:60: * overriden and validate is done seperately. On 2017/08/08 at 14:44:26, jbudorick wrote: > nit: overridden, and validation Done https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:63: protected void collectInitializationErrors(List<Throwable> errors) { On 2017/08/08 at 14:44:26, jbudorick wrote: > Should this collect errors from validation? collectInitializationErrors wraps all errors into a runtime error. This squashes our vibrant, diversified group of different throwables into a single bland type :( Jokes aside, throwing different throwables here makes writing failure-expected tests for ParameterizedRunner easier. https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:73: List<FrameworkMethod> returnList = new ArrayList<>(); On 2017/08/08 at 14:44:26, jbudorick wrote: > nit: declare this after the tagToListOfFrameworkMethod creation loop Done https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:105: + "parameters with tag %s. But there isn't any test that uses " On 2017/08/08 at 14:44:26, jbudorick wrote: > nit: > > "parameter with tag \"%s\", but no test uses those parameters." Done https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:110: throw new IllegalStateException(String.format("No parameters with tag %1$s " On 2017/08/08 at 14:44:26, jbudorick wrote: > nit: > > "No parameters with tag %1$s are provided by class %2$s." > > I would omit the code sample from the exception text. If you'd like to reference it in code, drop a comment here. Done https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:163: runnerDelegateClass, e.getMessage()), On 2017/08/08 at 14:44:26, jbudorick wrote: > Do you need to include e's message here if you're also including e as the cause? Seems like it might be redundant. Removed https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:20: * Test for org.chromium.base.test.params.InnerParamClassRunner On 2017/08/08 at 14:44:26, jbudorick wrote: > nit: update class name Done https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:23: public class ParameterizedRunnerDelegateFactoryTest { On 2017/08/08 at 14:44:26, jbudorick wrote: > Given the complexity of the logic in ParameterizedRunnerDelegateFactory, I think this ought to cover more. Specifically, I think it ought to have more extensive tests for generateUnmodifiableFrameworkMethodList. Done https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:53: public void testBadExampleRunnerDelegate() throws Throwable { On 2017/08/08 at 14:44:26, jbudorick wrote: > This test name should reflect *why* BadExampleRunnerDelegate should throw an exception, not just that it does. Done
On 2017/08/08 at 14:44:27, jbudorick wrote: > Looks very good. Will be an LG once you address ParameterizedRunnerDelegateFactoryTest Done, thank you for reviewing this dreadfully long CL! :) > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:65: @SuppressFBWarnings("NP_LOAD_OF_KNOWN_NULL_VALUE") > ? > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:67: List<Object> deepCopy = new ArrayList<>(); > nit: name this something other than deepCopy. It's not a deep copy, and its depth isn't really relevant to what it's doing. > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:45: return super.invokeExplosively(target, mParameterSet.getValues().toArray()); > What happens to params in this case? Is it ok that we drop them? > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:86: // TODO(yolandyan) add annotation from the ParameterSet, enable > super nit: colon after TODO(yolandyan): > ^ > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java:87: // test writing to add SkipCheck for an individual parameter > also, this would be a pretty slick feature, though I agree with your decision to put it off to another CL. > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java (right): > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:37: private static final String TAG = "ParameterizedRunner"; > nit: should be prefixed w/ "cr_" > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:60: * overriden and validate is done seperately. > nit: overridden, and validation > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java:63: protected void collectInitializationErrors(List<Throwable> errors) { > Should this collect errors from validation? > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:73: List<FrameworkMethod> returnList = new ArrayList<>(); > nit: declare this after the tagToListOfFrameworkMethod creation loop > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:105: + "parameters with tag %s. But there isn't any test that uses " > nit: > > "parameter with tag \"%s\", but no test uses those parameters." > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:110: throw new IllegalStateException(String.format("No parameters with tag %1$s " > nit: > > "No parameters with tag %1$s are provided by class %2$s." > > I would omit the code sample from the exception text. If you'd like to reference it in code, drop a comment here. > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:163: runnerDelegateClass, e.getMessage()), > Do you need to include e's message here if you're also including e as the cause? Seems like it might be redundant. > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... > File base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java (right): > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... > base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:20: * Test for org.chromium.base.test.params.InnerParamClassRunner > nit: update class name > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... > base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:23: public class ParameterizedRunnerDelegateFactoryTest { > Given the complexity of the logic in ParameterizedRunnerDelegateFactory, I think this ought to cover more. Specifically, I think it ought to have more extensive tests for generateUnmodifiableFrameworkMethodList. > > https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... > base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:53: public void testBadExampleRunnerDelegate() throws Throwable { > This test name should reflect *why* BadExampleRunnerDelegate should throw an exception, not just that it does.
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
at long last: lgtm w/ nit https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:73: List<FrameworkMethod> returnList = new ArrayList<>(); On 2017/08/08 23:19:32, the real yoland wrote: > On 2017/08/08 at 14:44:26, jbudorick wrote: > > nit: declare this after the tagToListOfFrameworkMethod creation loop > > Done oh, oops, I missed that this was used in the loop below :| https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java:23: public class ParameterizedRunnerDelegateFactoryTest { On 2017/08/08 23:19:32, the real yoland wrote: > On 2017/08/08 at 14:44:26, jbudorick wrote: > > Given the complexity of the logic in ParameterizedRunnerDelegateFactory, I > think this ought to cover more. Specifically, I think it ought to have more > extensive tests for generateUnmodifiableFrameworkMethodList. > > Done Thanks! https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:97: Assert.assertArrayEquals(tagToParameterSetList.keySet().toArray(), nit: this should have an error message -- a user seeing this probably won't know what to make of it without one. Maybe something like "All parameters used by must be defined, and all defined parameters must be used." or some such. (it sounds a bit weird since it's checking two separate conditions...)
https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:97: Assert.assertArrayEquals(tagToParameterSetList.keySet().toArray(), On 2017/08/10 at 15:53:13, jbudorick wrote: > nit: this should have an error message -- a user seeing this probably won't know what to make of it without one. Maybe something like > > "All parameters used by must be defined, and all defined parameters must be used." > > or some such. (it sounds a bit weird since it's checking two separate conditions...) Changed to "None shall pass"
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/2568633002/#ps380001 (title: "add callable and assertion message")
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:97: Assert.assertArrayEquals(tagToParameterSetList.keySet().toArray(), On 2017/08/10 21:43:48, the real yoland wrote: > On 2017/08/10 at 15:53:13, jbudorick wrote: > > nit: this should have an error message -- a user seeing this probably won't > know what to make of it without one. Maybe something like > > > > "All parameters used by must be defined, and all defined parameters must be > used." > > > > or some such. (it sounds a bit weird since it's checking two separate > conditions...) > > Changed to "None shall pass" YOU SHALL NOT PASS
On 2017/08/10 at 21:50:21, jbudorick wrote: > https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... > File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): > > https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:97: Assert.assertArrayEquals(tagToParameterSetList.keySet().toArray(), > On 2017/08/10 21:43:48, the real yoland wrote: > > On 2017/08/10 at 15:53:13, jbudorick wrote: > > > nit: this should have an error message -- a user seeing this probably won't > > know what to make of it without one. Maybe something like > > > > > > "All parameters used by must be defined, and all defined parameters must be > > used." > > > > > > or some such. (it sounds a bit weird since it's checking two separate > > conditions...) > > > > Changed to "None shall pass" > > YOU SHALL NOT PASS loooool
On 2017/08/10 at 21:53:52, the real yoland wrote: > On 2017/08/10 at 21:50:21, jbudorick wrote: > > https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... > > File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): > > > > https://codereview.chromium.org/2568633002/diff/360001/base/test/android/java... > > base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:97: Assert.assertArrayEquals(tagToParameterSetList.keySet().toArray(), > > On 2017/08/10 21:43:48, the real yoland wrote: > > > On 2017/08/10 at 15:53:13, jbudorick wrote: > > > > nit: this should have an error message -- a user seeing this probably won't > > > know what to make of it without one. Maybe something like > > > > > > > > "All parameters used by must be defined, and all defined parameters must be > > > used." > > > > > > > > or some such. (it sounds a bit weird since it's checking two separate > > > conditions...) > > > > > > Changed to "None shall pass" > > > > YOU SHALL NOT PASS > > loooool instant karma!
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/2568633002/#ps400001 (title: "rebase")
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 yolandyan@chromium.org
yolandyan@chromium.org changed reviewers: + nyquist@chromium.org
+nyquist for owner stamp
talked to jbudorick@ offline, so rs lgtm, but could you please extend the CL description as to what this really does? Right now it gives me nothing. For 1500+ lines of new code, it feels like you could at least provide some description of what the new hotness is?
Description was changed from ========== Create Next Gen Parameter Test BUG=673092 ========== to ========== Create Next Gen Parameter Test Framework for JUnit4 This test framework enables developers to write parameterized tests with simple static field. The framework allows both test class parameterization (through constructor arguments injection) and test method parameterization (through test method arguments injection). For more detail on how to write parameterized tests using this framework, please check the crbug or ExampleParameterizedTest.java file. BUG=673092 ==========
The CQ bit was checked by yolandyan@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": 400001, "attempt_start_ts": 1502409295778650, "parent_rev": "857bee5e96261ac2da484ff0c54d6838f6c951dd", "commit_rev": "fafc82e1442b3a6d2c2a7b9e364e30b76647322a"}
Message was sent while issue was closed.
Description was changed from ========== Create Next Gen Parameter Test Framework for JUnit4 This test framework enables developers to write parameterized tests with simple static field. The framework allows both test class parameterization (through constructor arguments injection) and test method parameterization (through test method arguments injection). For more detail on how to write parameterized tests using this framework, please check the crbug or ExampleParameterizedTest.java file. BUG=673092 ========== to ========== Create Next Gen Parameter Test Framework for JUnit4 This test framework enables developers to write parameterized tests with simple static field. The framework allows both test class parameterization (through constructor arguments injection) and test method parameterization (through test method arguments injection). For more detail on how to write parameterized tests using this framework, please check the crbug or ExampleParameterizedTest.java file. BUG=673092 Review-Url: https://codereview.chromium.org/2568633002 Cr-Commit-Position: refs/heads/master@{#493605} Committed: https://chromium.googlesource.com/chromium/src/+/fafc82e1442b3a6d2c2a7b9e364e... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:400001) as https://chromium.googlesource.com/chromium/src/+/fafc82e1442b3a6d2c2a7b9e364e... |