Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1219683014:
Added Generic Parameterized Testing (Closed)
|
Created:
5 years, 5 months ago by Sean Kirmani Modified:
5 years, 4 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded Generic Parametrized Testing
Usage and Info in this doc:
https://docs.google.com/a/google.com/document/d/1ItTezv49Idns-r_ZcIJNCLsc3xy0h1uwUMQjCJ9vcCM/edit?usp=sharing
Added a ParameterizedTest annotion with ability to have extra setup and teardown functionality
specific to parameters. Has an array of valid Parameter objects to run. Each Parameter object can
have an array of ParameterArgument objects. ParameterArgument objects can be set to have a default,
otherwise they are required.
CQ-DEPEND=CL:1259253007
Committed: https://crrev.com/01780b3cebecf652aa7d7cf41c781ba77d73aff8
Cr-Commit-Position: refs/heads/master@{#343131}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed imports #Patch Set 3 : #
Total comments: 8
Patch Set 4 : Added ParameterizedTestSet #Patch Set 5 : Fixed Merge Conflict #
Total comments: 1
Patch Set 6 : Removed static variables #
Total comments: 1
Patch Set 7 : #Patch Set 8 : #
Total comments: 1
Patch Set 9 : Added Parameterizable interface #
Total comments: 1
Patch Set 10 : Major Code Cleanup #Patch Set 11 : Bug Fix #
Total comments: 4
Patch Set 12 : #Patch Set 13 : #
Total comments: 13
Patch Set 14 : Major Code Refactor #Patch Set 15 : Merged with RestrictionSkipCheck in TestRunner #Patch Set 16 : #Patch Set 17 : Fixed not-skipping bug on ParameterizedTestSets #Patch Set 18 : #Patch Set 19 : Bug fixes #Patch Set 20 : Bug Fixes #2 #Patch Set 21 : Bug Fixes #3 #Patch Set 22 : rebase #
Total comments: 2
Patch Set 23 : Addressed John's Comments #
Total comments: 11
Patch Set 24 : Addressed John's Comments #2 #Patch Set 25 : CommandLineFlags Merge #
Total comments: 9
Patch Set 26 : Addressed John's Comments #3 #Patch Set 27 : rebase #
Total comments: 5
Patch Set 28 : Parameter.Reader is Smarter #
Total comments: 26
Patch Set 29 : Addressed John's and Randy's Comments #
Total comments: 8
Patch Set 30 : Addressed John's Comments #4 #
Total comments: 4
Patch Set 31 : Formatted ParameterizedTestError print output #Patch Set 32 : John's LGTM Nits #
Total comments: 6
Patch Set 33 : Mike's LGTM Nits #Patch Set 34 : #Patch Set 35 : #Patch Set 36 : #
Total comments: 94
Patch Set 37 : Addressed Nyquist's Comments #Patch Set 38 : Fixed Parameterized Error Log Output #Patch Set 39 : #
Total comments: 26
Patch Set 40 : Added Arguments to Error Log and nits #
Total comments: 12
Patch Set 41 : Code cleanup and more nits #
Total comments: 2
Patch Set 42 : Tommy's LGTM Nit #Messages
Total messages: 59 (11 generated)
skirmani@google.com changed reviewers: + jbudorick@chromium.org
Ready for review! Please check out this doc for all the details about the feature. https://docs.google.com/a/google.com/document/d/1ItTezv49Idns-r_ZcIJNCLsc3xy0...
We should be able to collapse some of these classes (e.g. ParameterTools and ParameterConfig) https://codereview.chromium.org/1219683014/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterConfig.java (right): https://codereview.chromium.org/1219683014/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterConfig.java:15: private BaseActivityInstrumentationTestCase mTestCase; These appear to be unnecessary. https://codereview.chromium.org/1219683014/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterConfig.java:23: public ParameterConfig() { Make this private. https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java (right): https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:29: public String getArgument(String argumentName, String defaultString) { nit: I think this should be getStringArgument to match the one without a default below. https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:30: if (ParameterTools.get(mTestCase).getParameterArgument(getTag(), argumentName) != null) { Just store the result of this call instead of calling it twice. https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameters.java (right): https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameters.java:24: public static final String METHOD_PARAMETERS = "method-parameters"; The tags are essentially string typing, which I'm not a big fan of. Is there any way to support this using the actual type system? https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameters.java:32: private void addParameters() { Why is this a separate function? https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterConfig.java (right): https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterConfig.java:15: private BaseActivityInstrumentationTestCase mTestCase; These both appear to be unused. https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterConfig.java:23: public ParameterConfig() { You can omit the empty constructor. https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterTools.java (right): https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterTools.java:15: private static Method sMethod; These should not be static. In fact, the methods in this class probably don't need to be static, either. https://codereview.chromium.org/1219683014/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterTools.java:32: public static ParameterTools get(BaseActivityInstrumentationTestCase testCase) { Yeah, definitely not.
Ready for review
jbudorick@chromium.org changed reviewers: + mikecase@chromium.org
Eliminating the statics will have enough of an effect on the rest of the code that I'll hold off on a further review. I can't emphasize enough that mutable globals -- in this case, those statics -- are usually a design flaw, and they're definitely a code smell. Adding Case, as it doesn't hurt to have him on the review, but I don't expect him to review it before the statics rework. https://codereview.chromium.org/1219683014/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java (right): https://codereview.chromium.org/1219683014/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:35: public static Queue<ParameterizedTest> sParameterizedTests; O_O These are not OK. Try to eliminate non-constant statics. As we talked about yesterday, they're a recipe for a disaster.
Static variables are gone! Ready for review.
Made test running more modular and readable. Will work on interfacing soon, but I'll be at the SET Intern Summit tomorrow, so I wanted to send you something to look at since I will be OOO. Ready for partial review.
Now ready for a full review!
Ready for review! Major changes: - Added Parameterizable interface. - Moved TestResults to separate directory. - Moved ParameterConfig stuff to ParameterizedTestResult.
https://codereview.chromium.org/1219683014/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java (right): https://codereview.chromium.org/1219683014/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:112: if (BaseActivityInstrumentationTestCase.class.isAssignableFrom(test.getClass())) { I don't think parameterization should be intrinsic to BaseActivityInstrumentationTestCase. Instead, it should be keyed off of an interface that BaseActivityInstrumentationTestCase implements (but that other test cases can implement as well). https://codereview.chromium.org/1219683014/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:81: private void handleException(Throwable exception) throws Throwable { Handling both exceptions raised in runTest and exceptions raised in tearDown in a single flow is liable to be confusing. https://codereview.chromium.org/1219683014/diff/160001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java (right): https://codereview.chromium.org/1219683014/diff/160001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:116: if (BaseActivityInstrumentationTestCase.class.isAssignableFrom(test.getClass())) { The other side of adding the Parameterizable interface is having this interact with the tests through that interface. https://codereview.chromium.org/1219683014/diff/200001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java (right): https://codereview.chromium.org/1219683014/diff/200001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:45: if (test instanceof Parameterizable && test instanceof TestCase) { We already know that test is a TestCase. https://codereview.chromium.org/1219683014/diff/200001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:73: private <T extends TestCase & Parameterizable> void parameterRun(final TestCase test) { Comments from offline: parameterRun and runWithParameters (and maybe run) should be collapsed into one function. https://codereview.chromium.org/1219683014/diff/200001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:115: setUpParameters(test, parameterizedTest); Comments from offline: let's try moving setUpParameters before setUp and tearDownParameters after tearDown. This should let you use TestResult.run rather than having to deal with all of this Protectable stuff. https://codereview.chromium.org/1219683014/diff/200001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterTools.java (right): https://codereview.chromium.org/1219683014/diff/200001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterTools.java:16: public class ParameterTools { I think that most of these should be in the Parameterizable interface.
- Removed setUpParameterized, runTestParameterized, tearDownParameterized - Merged parameterRun and runWithParameters
skirmani@google.com changed reviewers: + rnephew@chromium.org
This is headed in the right direction. https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:30: private ParameterizedTest mParameterizedTest; I think you can rework this s.t. mParameterizedTest is unnecessary. https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java (right): https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:29: public class ParameterizedTestResult extends SkippingTestResult { We should discuss ordering of functions within a class. https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:30: private List<BaseParameter> mParameters = new ArrayList<>(); nit: Sort these alphabetically. https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:51: // ignore We should at least log this. https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:64: mErrors.add(new ParameterFailure(e, mParameterizedTestIndex)); The index doesn't say very much. Can we use the parameters instead? https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:81: @SuppressWarnings("unchecked") What's being suppressed here? The cast to T? https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:91: runProtected(testCase, parameterizedTest); Do we need to use runProtected at all? Could we just do a try/catch block here and convert mErrors to a local ArrayList? https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:107: private <T extends TestCase & Parameterizable, V extends TestCase> void runProtected( I don't think we need two runProtected functions. This one should be merged into parameterRun. https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:113: findMethod(testClass, "setUp").invoke(test); I would prefer we avoid reflection if at all possible. https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java (right): https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:12: public interface Parameterizable { I was thinking that some of the utilities provided by ParameterTools could be part of the Parameterizable interface. https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:16: // TODO: remove ? https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTest.java (right): https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTest.java:14: * Provides annotations related to parametrized test handling. This should be rephrased. It looks like you stole this from CommandLineFlags, which is a class with two annotations inside. This is just an annotation. https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestSet.java (right): https://codereview.chromium.org/1219683014/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestSet.java:14: * Provides annotations related to parametrized test set handling. ditto
- Removed runProtected from ParameterizedTestResult
- Added TestRunnerMethods enum for reflection security
- Removed mParameterizedTest from BaseActivityInstrumentationRunner
- Output the list of failed parameters instead of the index; this should be more
useful
- Consolidated all parameter annotation related files into one file:
- ParameterArgument -> Parameter.Argument
- Parameterizable -> Parameter.Izable
- ParameterizedTest -> Parameter.IzedTest
- ParameterizedTestSet -> Parameter.IzedTest.Set
- ParameterTools -> Parameter.Reader
- Added getStringArrayArgument to BaseParameter so new parameters can use String
arrays
- Removed unnecessary globals from ParameterizedTestResult
- Added Log for if runParameterized fails
- Re-ordered Methods in ParameterizedTestResult to make more sense
Ready for review!
On 2015/07/28 19:53:03, Sean Kirmani wrote: > - Removed runProtected from ParameterizedTestResult > - Added TestRunnerMethods enum for reflection security > - Removed mParameterizedTest from BaseActivityInstrumentationRunner > - Output the list of failed parameters instead of the index; this should be more > useful > - Consolidated all parameter annotation related files into one file: > - ParameterArgument -> Parameter.Argument > - Parameterizable -> Parameter.Izable > - ParameterizedTest -> Parameter.IzedTest > - ParameterizedTestSet -> Parameter.IzedTest.Set > - ParameterTools -> Parameter.Reader > - Added getStringArrayArgument to BaseParameter so new parameters can use String > arrays > - Removed unnecessary globals from ParameterizedTestResult > - Added Log for if runParameterized fails > - Re-ordered Methods in ParameterizedTestResult to make more sense > > Ready for review! PTAL. Depending CL was committed, so ready for a full review now.
https://codereview.chromium.org/1219683014/diff/410001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/410001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:46: public void setTestResult(ParameterizedTestResult testResult) { I think this is my biggest sticking point with this design. Passing the test result to the test case feels backward. What if we just had it build and pass a Parameter.Reader? https://codereview.chromium.org/1219683014/diff/410001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java (right): https://codereview.chromium.org/1219683014/diff/410001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:43: public interface Izable { .Izable and .IzedTest read a little too strangely. Let's move them each to their own files as Parameterizable and ParameterizedTest. ParameterizedTest.Set should be fine, and I like Parameter.Argument and Parameter.Reader.
- Made a ParameterToolkit for the Test Case to interact with. - Moved ParameterizedTest and Parameterizable to different files. PTAL
wip review. Still mulling over the use of getParameterizedTest. https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java (right): https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:30: private static final String TAG = "cr.ParameterizedTest"; I'd prefer "cr.base.test" https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:48: public ParameterizedTest getCurrentParameterizedTest() { This is a getter for a member of ParameterizedTestResult...? https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:66: private <T extends TestCase & Parameterizable> void runParameterized(final TestCase test) Could this take a final T test instead? https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:68: @SuppressWarnings("unchecked") (if it can, you can ditch this, iirc) https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:74: mAvailableParameters = new ArrayList<>(); Where do you set up mAvailableParameters? https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:78: TestRunnerMethod.SETUP.invoke(testCase); o_o I strongly dislike this and would like to avoid doing it if at all possible. https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:79: for (final ParameterizedTest parameterizedTest : parameterizedTests) { I don't think we want to do setUp test#param1 test#param2 ... tearDown instead, I think we want to do setUp test#param1 tearDown setUp test#param2 tearDown ... https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:93: TestRunnerMethod.TEARDOWN.invoke(testCase); o_o https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:122: mAvailableParameters = new ArrayList<BaseParameter>(); What's up with this? https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:143: private static class ParameterFailedError extends Throwable { nit: ParameterizedTestFailedError? https://codereview.chromium.org/1219683014/diff/430001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/result/ParameterizedTestResult.java:185: private enum TestRunnerMethod { I would prefer almost any other implementation to this one if possible. Let's talk about the options you considered.
- Renamed TAG to cr.base.test - Renamed ParameterFailedError to ParameterizedTestFailedError - Re-ordered test execution - Moved mAvailableParameters declaration to constructor Also, a couple other majors changes: - I've changed the design of the app so that it will set up the parameters before setting up the test. The only downside I'm currently seeing is that it will be difficult to sign into the app. I'll work on that when dealing with the Signin Parameters. - I've moved the ParameterizedTestResult stuff into BaseTestResult. It should reduce confusion in the future of having multiple different TestResults.
That change of order of parameter set up execution helps us get rid of the reflection as well. That way we don't have to deal with individually accessing the protected methods. Please take another PTAL.
https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:46: * Creates an instance of SkippingTestResult. s/Skipping/Base/ https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:104: if (test instanceof Parameterizable) { nit: linebreak above this. https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:107: } catch (Throwable t) { Why should we catch this instead of letting it propagate? https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:108: Log.i(TAG, "Parameterized test run failed."); ... and if we do catch something here, why aren't we logging the exception? https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:119: public class ParameterToolkit { I think we can get rid of this class entirely by: - passing a Parameter.Reader to the test case s.t. it can use it during the test (this may involve the reader tracking a bit more state) - changing addParameters to either: - getAvailableParameters(), which would return a list of parameters that would then be added within runParameterized, or - addParameters(List<BaseParameters>), which would add relevant parameters to the available parameters list directly. https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:144: testCase.addParameters(); Let's do one of two things here: testCase.addParameters(this); https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:151: errors.add(new ParameterFailure(e, parameterizedTest)); These should be handled as failures, not as errors. The distinction is important. https://codereview.chromium.org/1219683014/diff/470001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:245: return buffer.toString().substring(buffer.toString().indexOf('\n') + 1); Is this just returning the first line of the stack trace...?
- Fixed copyright year - Renamed Skipping to Base in documentation - Fixed nits - Logs the error of the ThreadDeath in the case the parameter run fails - Removed ParameterToolkit - Handles errors and failures separately PTAL
https://codereview.chromium.org/1219683014/diff/510001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/510001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:45: public void setParameterizedTest(ParameterizedTest parameterizedTest) { I thought we were getting rid of this and replacing it with a Parameter.Reader? https://codereview.chromium.org/1219683014/diff/510001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/510001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:115: private <T extends TestCase & Parameterizable> void runParameterized(final TestCase test) Can this be runParameterized(final T test)? https://codereview.chromium.org/1219683014/diff/510001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:220: error.getThrowable().toString(), parametersString.toString())); nit: indentation off https://codereview.chromium.org/1219683014/diff/510001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:223: error.getThrowable().toString())); same https://codereview.chromium.org/1219683014/diff/510001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:238: return buffer.toString().substring(buffer.toString().indexOf('\n') + 1); Why is this only adding the first line?
- Fixed indentation nits - Made current ParameterizedTest in Parameter.Reader set-able - Pass Parameter.Reader to TestCase now Please take another PTAL
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:27: protected Parameter.Reader mParameterReader; Is there any reason for this to be protected instead of making it private & having derived classes access it via getParameterReader? https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:116: private <T extends TestCase & Parameterizable> void runParameterized(final TestCase test) nit: Vertical whitespace can make your code more readable. https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:198: private static class ParameterizedTestError extends Throwable { extends Exception https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java (right): https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:93: public void setParameterizedTest(ParameterizedTest parameterizedTest) { nit: maybe "setCurrentParameterizedTest" Otherwise, it's easy to confuse w/ getParameterizedTest & isParameterizedTest https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:100: * @return the {@link Parameter} for a given {@link ParameterizedTest} with the nit: add @param https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:118: * @return the {@link Parameter.Argument} for a given {@link ParameterizedTest} for the nit: add @param https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:122: public Parameter.Argument getParameterArgument(String targetParameter, Should we have an overload of this that takes a Parameter, too? https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:188: assertTrue("string1 should have vowel(s).", hasVowel(stringVar)); Why are you checking hasVowel instead of the value? https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:192: assertTrue("int1 should be prime.", isPrime(intVar)); Again, check the exact value. https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:416: private boolean isPrime(int num) { Get rid of this. https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:436: private boolean hasVowel(String str) { Get rid of this. https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:441: private void emptyTests() { What's with this function?
mikecase & rnephew: ptal
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java (right): https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:53: public int[] getIntArrayArgument(String argumentName, int[] defaultIntArray) { I think it would help with readability if you had the ordering consistent. The ordering flips partway through. public String getStringArgument(String argumentName, String defaultString) public String getStringArgument(String argumentName) ... public int[] getIntArrayArgument(String argumentName) public int[] getIntArrayArgument(String argumentName, int[] defaultIntArray)
Patch Set #29: Addressed John's and Randy's Comments
- Made mParameterReader private in BaseActivityInstrumentationTestCase
- Nit fix: Added vertical whitespace to runParamterized in BaseTestResult
- Made ParameterizedTestError extend Exception instead of Throwable
- Renamed setParameterizedTest to setCurrentParameterizedTest in
Parameter.Reader
- Added @param tag to getParameter in Parameter.Reader
- Added @param tag to getParameterArgument in Parameter.Reader
- Added an overloaded function that takes in a Parameter for
getParameterArgument in Parameter.Reader
- Checked exact value in ParameterizedTestAnnotationTest instead of isPrime and
hasVowel
- removed isPrime and hasVowel from ParameterizedTestAnnotationTest
- removed emptyTests
- made ordering consistent for methods in BaseParameters
- added new tests where emptyTests was previously called in
ParameterizedTestAnnotationTest
- Removed try/catch statements from ParameterizedTestAnnotationTest
- Made an assertArrayEquals method
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
File
base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java
(right):
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:27:
protected Parameter.Reader mParameterReader;
On 2015/08/05 13:34:01, jbudorick wrote:
> Is there any reason for this to be protected instead of making it private &
> having derived classes access it via getParameterReader?
Done.
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java
(right):
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:116:
private <T extends TestCase & Parameterizable> void runParameterized(final
TestCase test)
On 2015/08/05 13:34:01, jbudorick wrote:
> nit: Vertical whitespace can make your code more readable.
Done.
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:198:
private static class ParameterizedTestError extends Throwable {
On 2015/08/05 13:34:01, jbudorick wrote:
> extends Exception
Done.
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
File
base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java
(right):
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:53:
public int[] getIntArrayArgument(String argumentName, int[] defaultIntArray) {
On 2015/08/05 15:35:31, rnephew1 wrote:
> I think it would help with readability if you had the ordering consistent. The
> ordering flips partway through.
>
> public String getStringArgument(String argumentName, String defaultString)
> public String getStringArgument(String argumentName)
> ...
> public int[] getIntArrayArgument(String argumentName)
> public int[] getIntArrayArgument(String argumentName, int[] defaultIntArray)
Done.
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
File
base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java
(right):
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:93:
public void setParameterizedTest(ParameterizedTest parameterizedTest) {
On 2015/08/05 13:34:02, jbudorick wrote:
> nit: maybe "setCurrentParameterizedTest"
>
> Otherwise, it's easy to confuse w/ getParameterizedTest & isParameterizedTest
Done.
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:100:
* @return the {@link Parameter} for a given {@link ParameterizedTest} with the
On 2015/08/05 13:34:01, jbudorick wrote:
> nit: add @param
Done.
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:118:
* @return the {@link Parameter.Argument} for a given {@link ParameterizedTest}
for the
On 2015/08/05 13:34:01, jbudorick wrote:
> nit: add @param
Done.
https://codereview.chromium.org/1219683014/diff/530001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:122:
public Parameter.Argument getParameterArgument(String targetParameter,
On 2015/08/05 13:34:01, jbudorick wrote:
> Should we have an overload of this that takes a Parameter, too?
Done.
https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java
(right):
https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:188:
assertTrue("string1 should have vowel(s).", hasVowel(stringVar));
Checked exact value.
https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:192:
assertTrue("int1 should be prime.", isPrime(intVar));
On 2015/08/05 13:34:02, jbudorick wrote:
> Again, check the exact value.
Done.
https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:416:
private boolean isPrime(int num) {
On 2015/08/05 13:34:02, jbudorick wrote:
> Get rid of this.
Done.
https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:436:
private boolean hasVowel(String str) {
On 2015/08/05 13:34:02, jbudorick wrote:
> Get rid of this.
Done.
https://codereview.chromium.org/1219683014/diff/530001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:441:
private void emptyTests() {
Removed.
PTAL!
https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:39: public List<BaseParameter> getAvailableParameters() { javadocs for public functions https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:129: mAvailableParameters.addAll(testCase.getAvailableParameters()); Make mAvailableParameters local to this function & pass it to setUpParameters and tearDownParameters. https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:217: buffer.append('\n'); nit: collapse this line into the ctor above https://codereview.chromium.org/1219683014/diff/550001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:376: private void assertArrayEquals(String message, Object[] expected, Object[] actual) { Is it possible to collapse these two as a generic?
Patch Set #30: Addressed John's Comments #4
- Added JavaDoc to public methods in BaseActivityInstrumentationTestCase
- Made available parameters a local to runParameterizedMethod and passed it to
setUpParameters and tearDownParameters
- Nit fix: Added '\n' to the StringBuffer constructor in BaseTestResult
- Used android.test.MoreAsserts to check array equality instead of creating my
own methods
https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java...
File
base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java
(right):
https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:39:
public List<BaseParameter> getAvailableParameters() {
On 2015/08/06 16:48:17, jbudorick wrote:
> javadocs for public functions
Done.
https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java...
File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java
(right):
https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:129:
mAvailableParameters.addAll(testCase.getAvailableParameters());
On 2015/08/06 16:48:17, jbudorick wrote:
> Make mAvailableParameters local to this function & pass it to setUpParameters
> and tearDownParameters.
Done.
https://codereview.chromium.org/1219683014/diff/550001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:217:
buffer.append('\n');
On 2015/08/06 16:48:17, jbudorick wrote:
> nit: collapse this line into the ctor above
Done.
https://codereview.chromium.org/1219683014/diff/550001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java
(right):
https://codereview.chromium.org/1219683014/diff/550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:376:
private void assertArrayEquals(String message, Object[] expected, Object[]
actual) {
On 2015/08/06 16:48:17, jbudorick wrote:
> Is it possible to collapse these two as a generic?
Used MoreAsserts.assertEquals instead.
jbudorick@chromium.org changed reviewers: + yfriedman@chromium.org
lgtm w/ nit +yfriedman for owners https://codereview.chromium.org/1219683014/diff/570001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/570001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:51: * Setter method for {@link Parameter.Reader} called from the {@link BaseTestResult}. nit: no need to reference what the calling site is, just say that this sets the parameter reader. https://codereview.chromium.org/1219683014/diff/570001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/570001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:8: import android.test.MoreAsserts; huh, I'd never heard of this. Cool.
lgtm. Looks really cool, only have 2 nits. https://codereview.chromium.org/1219683014/diff/610001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/610001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:219: buffer.append(errorBuffer(mErrors.get(mErrors.size() - 1))); nit: Not a huge deal, but if mErrors is set to be an empty list, this will be an array out of bounds exception. https://codereview.chromium.org/1219683014/diff/610001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java (right): https://codereview.chromium.org/1219683014/diff/610001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java:13: /** Used to pass parameters to be used in a method */ I've never seen this. Can you have a javadoc comment to describe a constant? https://codereview.chromium.org/1219683014/diff/610001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/610001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:226: @Parameter( nit: indentation looks off @ParameterizedTest.Set(tests = { @ParameterizedTest(parameters = { @Parameter( second line in indented 8 spaces, and the third line is only indented by 4. I think everything should be 8? This happens in all of the test methods in this class.
https://codereview.chromium.org/1219683014/diff/570001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/570001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:51: * Setter method for {@link Parameter.Reader} called from the {@link BaseTestResult}. On 2015/08/06 19:16:04, jbudorick wrote: > nit: no need to reference what the calling site is, just say that this sets the > parameter reader. Done. https://codereview.chromium.org/1219683014/diff/570001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/570001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:8: import android.test.MoreAsserts; On 2015/08/06 19:16:04, jbudorick wrote: > huh, I'd never heard of this. Cool. Yeah, I thought so too! https://codereview.chromium.org/1219683014/diff/610001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/610001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:219: buffer.append(errorBuffer(mErrors.get(mErrors.size() - 1))); On 2015/08/07 17:06:06, mikecase wrote: > nit: Not a huge deal, but if mErrors is set to be an empty list, this will be an > array out of bounds exception. Done. https://codereview.chromium.org/1219683014/diff/610001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java (right): https://codereview.chromium.org/1219683014/diff/610001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java:13: /** Used to pass parameters to be used in a method */ On 2015/08/07 17:06:06, mikecase wrote: > I've never seen this. Can you have a javadoc comment to describe a constant? Removed. https://codereview.chromium.org/1219683014/diff/610001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/610001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:226: @Parameter( On 2015/08/07 17:06:06, mikecase wrote: > nit: indentation looks off > > @ParameterizedTest.Set(tests = { > @ParameterizedTest(parameters = { > @Parameter( > > second line in indented 8 spaces, and the third line is only indented by 4. I > think everything should be 8? This happens in all of the test methods in this > class. Done.
yfriedman@chromium.org changed reviewers: + nyquist@chromium.org
yfriedman->nyquist (this looks up your alley ;) My 2c though is that I worry about maintainability. Y'all are the experts here but in some cases, when a data-driven test fails, it's hard to get a sense for which test actually failed. Assuming the logs make it easy to understand which test case is failing I'm ok with this
On 2015/08/07 20:02:42, Yaron wrote: > yfriedman->nyquist (this looks up your alley ;) > > My 2c though is that I worry about maintainability. Y'all are the experts here > but in some cases, when a data-driven test fails, it's hard to get a sense for > which test actually failed. Assuming the logs make it easy to understand which > test case is failing I'm ok with this I've actually addressed this! In the BaseTestResult, I added a ParameterizedTestError which produces an output for all failed parameters. The log does output the error/failure for each parameter where there are multiple parameters. It tells you which ParameterizedTest failed so it'll be easy to catch which one broke the test.
The CQ bit was checked by skirmani@google.com to run a CQ dry run
The CQ bit was unchecked by skirmani@google.com
lgtm
I guess I have a similar concern as yfriedman@ about how easy it'll be to identify a failing test. Could you maybe add some examples as comments? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:37: private static final int SLEEP_INTERVAL = 50; // milliseconds Could you instead rename the constant to just _MS instead of adding a comment? (also below) https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:40: private Instrumentation mInstrumentation; Could this be final? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:47: mSkipChecks = new ArrayList<SkipCheck>(); Is it possible to just use new ArrayList<>() here? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:62: public boolean shouldSkip(TestCase testCase); You don't need |public| here. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:74: protected boolean shouldSkip(final TestCase test) { |final| is unnecessary here. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:82: protected void run(final TestCase test) { |final| is unnecessary. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:115: private <T extends TestCase & Parameterizable> void runParameterized(final TestCase test) Unnecessary |final| for param. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:117: final T testCase = (T) test; Unnecessary |final| for variable. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:127: if (!parameterizedTests.isEmpty()) { How about flipping this to the non-negative block first (calling super.run)? It's easier to read. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:130: for (final ParameterizedTest parameterizedTest : parameterizedTests) { Unnecessary final. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:140: } catch (Throwable e) { It's very seldom we catch throwable in our codebase. Could you add a comment saying that it's intentional here? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:160: private <T extends TestCase & Parameterizable> void setUpParameters(final T test, Static and unnecessary final. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:170: private <T extends TestCase & Parameterizable> void tearDownParameters(final T test, Static and unnecessary final. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:181: private Throwable mThrowable; final https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:182: private ParameterizedTest mParameterizedTest; final https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:212: public String toString() { Could you in fact just show an example here in the a comment about how an error would look like? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:213: StringBuffer buffer = new StringBuffer('\n'); I don't think you need the thread safety of StringBuffer here, so how about just using StringBuilder? If so, I think you want to pass in "\\n" instead of '\n' so it's not interpreted as an integer (size of buffer). https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:214: if (!mErrors.isEmpty()) { Could you flip this if? And if so, you could hold off declaring the StringBuilder. Something like this maybe: ### if (mErrors.isEmpty()) return "\n"; StringBuilder builder = new StringBuilder("\\n"); for (...) ... return builder.toString(); ### https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:225: private StringBuffer errorBuffer(ParameterError error) { Could this just return a StringBuilder? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:229: if (!parameters.isEmpty()) { Could you flip this if-statement so the non-negative case comes first? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:230: StringBuffer parametersString = new StringBuffer(); Maybe you could use StringBuilder here as well? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:231: for (int i = 0; i < parameters.size() - 1; i++) { Could you just use for (Parameter parameter : parameters) here? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:249: private String trace(ParameterError error) { static https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:254: return buffer.substring(buffer.indexOf("\n") + 1).toString().trim(); Do you need .toString() here? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:5: package org.chromium.base.test.util.parameters; This package name doesn't match the directory. Same for all the other files here. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:11: private String mTag; Nit: final https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:12: private Parameter.Reader mParameterReader; Nit: final https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:14: public BaseParameter(String tag, Parameter.Reader parameterReader) { What is a tag in this context? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:71: private void checkArgumentExists(Parameter.Argument parameterArgument) { static https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:25: public @interface Argument { Do you need to repeat |public| here? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:37: public class Reader { Remove |public| here as well? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:47: mParameterizedTest = null; Why is this line here? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:58: for (ParameterizedTest parameterizedTest : getParameterizedTestSet().tests()) { Could you just do Collections.addAll(...)? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:62: if (mAnnotatedElement.isAnnotationPresent(ParameterizedTest.class)) { could this just be an |else if|? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:110: if (parameter.tag().equals(targetParameter)) { Can .tag() ever return null? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:134: public Parameter.Argument getParameterArgument(Parameter parameter, When should this method be called compared to the method above? Since they are both public it'd be nice with some explanation. Or is this meant to be private? Also, this can be static. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:137: if (targetArgument.equals(argument.name())) { can |targetArgument| ever be null? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:13: public Map<String, BaseParameter> getAvailableParameters(); |public| is unnecessary here and below. It's implicit from the visibility of the interface. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:14: public void setParameterReader(Parameter.Reader parameterReader); Why does this have to be part of the interface? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTest.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTest.java:29: public @interface Set { Do you need |public| here? https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java:5: package org.chromium.base.test.util.parameters; This is missing |.parameter| in the package name. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java:11: public static final String PARAMETER_TAG = "method-parameter"; Who needs to read this? Could this be private or package protected? https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:5: package org.chromium.chrome.test.util; This doesn't match the file path. Also, the filepath here says "parameter", but the base package is just parameter. Is that intentional? Also, the rest of the package name is wrong. Did you mean org.chromium.chrome.browser.parameter ? https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:30: assertFalse("This is not a parameterized test.", getParameterReader() Should the explanation message be flipped here? It's only shown if the test fails. https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:64: .getParameter(MethodParameter.PARAMETER_TAG); Could this be moved to the previous line? https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:349: private String mismatchMessage(String name) { static? https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:353: private int fib(int input) { Could this be static?
Thanks for reviewing. PTAL! https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:37: private static final int SLEEP_INTERVAL = 50; // milliseconds On 2015/08/11 09:29:57, nyquist wrote: > Could you instead rename the constant to just _MS instead of adding a comment? > (also below) Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:40: private Instrumentation mInstrumentation; On 2015/08/11 09:29:56, nyquist wrote: > Could this be final? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:47: mSkipChecks = new ArrayList<SkipCheck>(); On 2015/08/11 09:29:57, nyquist wrote: > Is it possible to just use new ArrayList<>() here? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:62: public boolean shouldSkip(TestCase testCase); On 2015/08/11 09:29:56, nyquist wrote: > You don't need |public| here. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:74: protected boolean shouldSkip(final TestCase test) { On 2015/08/11 09:29:57, nyquist wrote: > |final| is unnecessary here. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:82: protected void run(final TestCase test) { On 2015/08/11 09:29:56, nyquist wrote: > |final| is unnecessary. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:115: private <T extends TestCase & Parameterizable> void runParameterized(final TestCase test) On 2015/08/11 09:29:56, nyquist wrote: > Unnecessary |final| for param. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:117: final T testCase = (T) test; On 2015/08/11 09:29:57, nyquist wrote: > Unnecessary |final| for variable. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:127: if (!parameterizedTests.isEmpty()) { On 2015/08/11 09:29:57, nyquist wrote: > How about flipping this to the non-negative block first (calling super.run)? > It's easier to read. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:130: for (final ParameterizedTest parameterizedTest : parameterizedTests) { On 2015/08/11 09:29:57, nyquist wrote: > Unnecessary final. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:140: } catch (Throwable e) { On 2015/08/11 09:29:56, nyquist wrote: > It's very seldom we catch throwable in our codebase. Could you add a comment > saying that it's intentional here? This is intentional. This logs the error output for all the failed parameterized tests. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:160: private <T extends TestCase & Parameterizable> void setUpParameters(final T test, On 2015/08/11 09:29:57, nyquist wrote: > Static and unnecessary final. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:170: private <T extends TestCase & Parameterizable> void tearDownParameters(final T test, On 2015/08/11 09:29:56, nyquist wrote: > Static and unnecessary final. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:181: private Throwable mThrowable; On 2015/08/11 09:29:57, nyquist wrote: > final Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:182: private ParameterizedTest mParameterizedTest; On 2015/08/11 09:29:56, nyquist wrote: > final Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:212: public String toString() { On 2015/08/11 09:29:56, nyquist wrote: > Could you in fact just show an example here in the a comment about how an error > would look like? Here is the error output. This shows three cases: the standard non-parameterized error output (testNoParameterizedTestAnnotation), the error output of a single failed parameterized test (testSingleTestParameterArgumentsWithParameterSet), the error of multiple failed parameterized tests (testParameterArgumentsWithParameterSetOfMoreThanOneTest). This might be a little more readable: https://docs.google.com/a/google.com/document/d/1T-FFTY1W4quFDFyZxWYcxlX6wPeY... C 93.414s Main ******************************************************************************** C 93.414s Main Detailed Logs C 93.414s Main ******************************************************************************** C 93.414s Main [FAIL] org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest#testNoParameterizedTestAnnotation: C 93.414s Main junit.framework.AssertionFailedError C 93.414s Main at org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest.testNoParameterizedTestAnnotation(ParameterizedTestAnnotationTest.java:34) C 93.414s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 93.414s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 93.414s Main at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) C 93.414s Main at org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:128) C 93.414s Main at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:104) C 93.415s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 93.415s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 93.415s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) C 93.415s Main at org.chromium.chrome.test.ChromeInstrumentationTestRunner.onStart(ChromeInstrumentationTestRunner.java:93) C 93.415s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1853) C 93.415s Main C 93.415s Main [FAIL] org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest#testParameterArgumentsWithParameterSetOfMoreThanOneTest: C 93.415s Main org.chromium.base.test.BaseTestResult$ParameterizedTestFailure: \n C 93.415s Main junit.framework.AssertionFailedError: Output should be the fibonacci number at index input. expected:<-1> but was:<1> (with parameters method-parameter) C 93.415s Main junit.framework.AssertionFailedError: Output should be the fibonacci number at index input. expected:<-1> but was:<1> C 93.415s Main at org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest.testParameterArgumentsWithParameterSetOfMoreThanOneTest(ParameterizedTestAnnotationTest.java:288) C 93.415s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 93.415s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 93.415s Main at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) C 93.415s Main at org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:136) C 93.415s Main at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:104) C 93.415s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 93.415s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 93.415s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) C 93.415s Main at org.chromium.chrome.test.ChromeInstrumentationTestRunner.onStart(ChromeInstrumentationTestRunner.java:93) C 93.415s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1853) C 93.415s Main C 93.415s Main junit.framework.AssertionFailedError: Output should be the fibonacci number at index input. expected:<-1> but was:<13> (with parameters method-parameter) C 93.415s Main junit.framework.AssertionFailedError: Output should be the fibonacci number at index input. expected:<-1> but was:<13> C 93.415s Main at org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest.testParameterArgumentsWithParameterSetOfMoreThanOneTest(ParameterizedTestAnnotationTest.java:288) C 93.415s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 93.416s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 93.416s Main at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) C 93.416s Main at org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:136) C 93.416s Main at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:104) C 93.416s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 93.416s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 93.416s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) C 93.416s Main at org.chromium.chrome.test.ChromeInstrumentationTestRunner.onStart(ChromeInstrumentationTestRunner.java:93) C 93.416s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1853) C 93.416s Main at org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:149) C 93.416s Main at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:104) C 93.416s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 93.416s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 93.416s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) C 93.416s Main at org.chromium.chrome.test.ChromeInstrumentationTestRunner.onStart(ChromeInstrumentationTestRunner.java:93) C 93.416s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1853) C 93.416s Main C 93.416s Main [FAIL] org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest#testSingleTestParameterArgumentsWithParameterSet: C 93.416s Main org.chromium.base.test.BaseTestResult$ParameterizedTestFailure: \n C 93.416s Main junit.framework.AssertionFailedError: Output should be the fibonacci number at each index input. expected array element[1]:<-1> but was:<1> (with parameters method-parameter) C 93.416s Main junit.framework.AssertionFailedError: Output should be the fibonacci number at each index input. expected array element[1]:<-1> but was:<1> C 93.416s Main at android.test.MoreAsserts.failWithMessage(MoreAsserts.java:573) C 93.416s Main at android.test.MoreAsserts.failWrongElement(MoreAsserts.java:532) C 93.416s Main at android.test.MoreAsserts.assertEquals(MoreAsserts.java:116) C 93.416s Main at org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest.testSingleTestParameterArgumentsWithParameterSet(ParameterizedTestAnnotationTest.java:317) C 93.416s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 93.417s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 93.417s Main at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) C 93.417s Main at org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:136) C 93.417s Main at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:104) C 93.417s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 93.417s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 93.417s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) C 93.417s Main at org.chromium.chrome.test.ChromeInstrumentationTestRunner.onStart(ChromeInstrumentationTestRunner.java:93) C 93.417s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1853) C 93.417s Main at org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:149) C 93.417s Main at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:104) C 93.417s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 93.417s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 93.417s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) C 93.417s Main at org.chromium.chrome.test.ChromeInstrumentationTestRunner.onStart(ChromeInstrumentationTestRunner.java:93) C 93.417s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1853) C 93.417s Main ******************************************************************************** C 93.417s Main Summary C 93.417s Main ******************************************************************************** C 93.417s Main [==========] 16 tests ran. C 93.417s Main [ PASSED ] 13 tests. C 93.417s Main [ FAILED ] 3 tests, listed below: C 93.417s Main [ FAILED ] org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest#testNoParameterizedTestAnnotation C 93.418s Main [ FAILED ] org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest#testParameterArgumentsWithParameterSetOfMoreThanOneTest C 93.418s Main [ FAILED ] org.chromium.base.test.util.parameter.ParameterizedTestAnnotationTest#testSingleTestParameterArgumentsWithParameterSet C 93.418s Main C 93.418s Main 3 FAILED TESTS C 93.418s Main ******************************************************************************** https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:213: StringBuffer buffer = new StringBuffer('\n'); On 2015/08/11 09:29:56, nyquist wrote: > I don't think you need the thread safety of StringBuffer here, so how about just > using StringBuilder? If so, I think you want to pass in "\\n" instead of '\n' so > it's not interpreted as an integer (size of buffer). Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:214: if (!mErrors.isEmpty()) { On 2015/08/11 09:29:56, nyquist wrote: > Could you flip this if? And if so, you could hold off declaring the > StringBuilder. Something like this maybe: > > ### > if (mErrors.isEmpty()) return "\n"; > > StringBuilder builder = new StringBuilder("\\n"); > for (...) ... > return builder.toString(); > ### Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:225: private StringBuffer errorBuffer(ParameterError error) { On 2015/08/11 09:29:56, nyquist wrote: > Could this just return a StringBuilder? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:229: if (!parameters.isEmpty()) { On 2015/08/11 09:29:57, nyquist wrote: > Could you flip this if-statement so the non-negative case comes first? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:230: StringBuffer parametersString = new StringBuffer(); On 2015/08/11 09:29:57, nyquist wrote: > Maybe you could use StringBuilder here as well? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:231: for (int i = 0; i < parameters.size() - 1; i++) { On 2015/08/11 09:29:57, nyquist wrote: > Could you just use for (Parameter parameter : parameters) here? I don't want to add the comma to the last element. Wouldn't it be more efficient to do all but the last one and then append the last one item without the comma? Otherwise I'd have to check if it was the last parameter every iteration of the loop. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:249: private String trace(ParameterError error) { On 2015/08/11 09:29:57, nyquist wrote: > static Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:254: return buffer.substring(buffer.indexOf("\n") + 1).toString().trim(); On 2015/08/11 09:29:56, nyquist wrote: > Do you need .toString() here? I unfortunately did need toString because StringBuilder does not support .trim(). I fixed this by writing my own trim method. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:5: package org.chromium.base.test.util.parameters; On 2015/08/11 09:29:57, nyquist wrote: > This package name doesn't match the directory. Same for all the other files > here. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:11: private String mTag; On 2015/08/11 09:29:57, nyquist wrote: > Nit: final Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:12: private Parameter.Reader mParameterReader; On 2015/08/11 09:29:57, nyquist wrote: > Nit: final Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:14: public BaseParameter(String tag, Parameter.Reader parameterReader) { On 2015/08/11 09:29:57, nyquist wrote: > What is a tag in this context? This is the name of the identifying tag to look for in annotations. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/BaseParameter.java:71: private void checkArgumentExists(Parameter.Argument parameterArgument) { On 2015/08/11 09:29:57, nyquist wrote: > static Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:25: public @interface Argument { On 2015/08/11 09:29:57, nyquist wrote: > Do you need to repeat |public| here? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:37: public class Reader { On 2015/08/11 09:29:58, nyquist wrote: > Remove |public| here as well? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:47: mParameterizedTest = null; On 2015/08/11 09:29:57, nyquist wrote: > Why is this line here? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:58: for (ParameterizedTest parameterizedTest : getParameterizedTestSet().tests()) { On 2015/08/11 09:29:57, nyquist wrote: > Could you just do Collections.addAll(...)? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:62: if (mAnnotatedElement.isAnnotationPresent(ParameterizedTest.class)) { On 2015/08/11 09:29:57, nyquist wrote: > could this just be an |else if|? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:110: if (parameter.tag().equals(targetParameter)) { On 2015/08/11 09:29:57, nyquist wrote: > Can .tag() ever return null? Fixed. But it should never return null. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:134: public Parameter.Argument getParameterArgument(Parameter parameter, On 2015/08/11 09:29:58, nyquist wrote: > When should this method be called compared to the method above? Since they are > both public it'd be nice with some explanation. Or is this meant to be private? > Also, this can be static. This is to be used in creating tests. We can get both the Parameter and the Parameter.Arguments to use in tests. But part of test usage would be being able to read these parameters we pass in sometimes. For example, if I want to test fibonacci, I'd need to read the parameter argument. But if I wanted to sign into the OS, I may not need to read it. John suggested that we could use the pass in a Parameter object or a string with the name of the targetParameter and let the user decide. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:137: if (targetArgument.equals(argument.name())) { On 2015/08/11 09:29:57, nyquist wrote: > can |targetArgument| ever be null? Fixed. But this should never be null. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:13: public Map<String, BaseParameter> getAvailableParameters(); On 2015/08/11 09:29:58, nyquist wrote: > |public| is unnecessary here and below. It's implicit from the visibility of the > interface. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:14: public void setParameterReader(Parameter.Reader parameterReader); On 2015/08/11 09:29:58, nyquist wrote: > Why does this have to be part of the interface? This is so that the BaseTestResult can call this method and set the Parameter.Reader for the TestCase. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTest.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTest.java:29: public @interface Set { On 2015/08/11 09:29:58, nyquist wrote: > Do you need |public| here? Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java (right): https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java:5: package org.chromium.base.test.util.parameters; On 2015/08/11 09:29:58, nyquist wrote: > This is missing |.parameter| in the package name. Done. https://codereview.chromium.org/1219683014/diff/690001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/MethodParameter.java:11: public static final String PARAMETER_TAG = "method-parameter"; On 2015/08/11 09:29:58, nyquist wrote: > Who needs to read this? Could this be private or package protected? This is read by the test itself. That way it knows which parameter to read. This is actually just the first parameter, so there needs to be identification between different kind of parameters. I have three more signin based parameters on another CL dependent on this one! https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:5: package org.chromium.chrome.test.util; On 2015/08/11 09:29:58, nyquist wrote: > This doesn't match the file path. Also, the filepath here says "parameter", but > the base package is just parameter. Is that intentional? > Also, the rest of the package name is wrong. Did you mean > org.chromium.chrome.browser.parameter ? Done. https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:30: assertFalse("This is not a parameterized test.", getParameterReader() On 2015/08/11 09:29:58, nyquist wrote: > Should the explanation message be flipped here? It's only shown if the test > fails. Done. https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:64: .getParameter(MethodParameter.PARAMETER_TAG); On 2015/08/11 09:29:58, nyquist wrote: > Could this be moved to the previous line? Done. https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:349: private String mismatchMessage(String name) { On 2015/08/11 09:29:58, nyquist wrote: > static? Done. https://codereview.chromium.org/1219683014/diff/690001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/parameters/ParameterizedTestAnnotationTest.java:353: private int fib(int input) { On 2015/08/11 09:29:58, nyquist wrote: > Could this be static? Done.
https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:62: * @return the {@link Parameter.Reader} for the current {@link ParameterizedTest} being run. Nit: This isn't currently imported, so either import it, or use the full package name here. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java (right): https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:110: * If {@link org.chromium.base.test.util.MinAndroidSdkLevel} is present, checks its value Optional nit: While you're at it, this fully qualified package name is unnecessary, since it's already imported. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:160: private static <T extends TestCase & Parameterizable> void setUpParameters(T test, The test parameter is unused here and below. Is that intentional? https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:170: private static <T extends TestCase & Parameterizable> void tearDownParameters(T test, The test parameter is unused here and below. Is that intentional? https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:212: public String toString() { Could you put parts of the output example into the code as the comment so other can read it while they look at the code? https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:225: private StringBuilder errorBuilder(ParameterError error) { Nit: static? https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:233: StringBuilder parametersString = new StringBuilder(); You don't technically know the underlying implementation of the List you are iterating over, right? So worst case your code is doing an O(N) lookup for each call to get(i). I guess you could do something like: private static String createTagList(List<Parameter> parameters) { Iterator<Parameter> it = parameters.iterator(); if (!it.hasNext()) return ""; StringBuilder tags = new StringBuilder(); tags.append(it.next().tag()); while (it.hasNext()) { tags.append(",").append(it.next().tag()); } return tags.toString(); } https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:238: builder.append(String.format("%n%s (with parameters %s)%n", I think we really want to print the key=value parts of the failed test here. Otherwise you lose valuable information, and in the case where you use this for a matrix of data input, like in the last fibonacci-test you wrote, you would want to know which fibonacci input and output failed, not just that "it failed". https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:277: * @return The target {@link android.content.Context} if available; null otherwise. Nit: Just use {@link Context}, since it's already imported. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java (right): https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:117: * a {@link Paramter.Argument}. Nit: {@link Parameter.Argument} https://codereview.chromium.org/1219683014/diff/750001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/750001/chrome/android/javates... chrome/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestAnnotationTest.java:15: * Tester class for the {@link ParameterizedTest} annotation and the {@link ParameterizedTestSet} Nit: {@link ParameterizedTest.Set} https://codereview.chromium.org/1219683014/diff/750001/chrome/android/javates... chrome/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestAnnotationTest.java:293: * {@link ParameterTest} annotations by using an intArray instead of an intVar. Computationally, Nit: {@link ParameterizedTest} ? https://codereview.chromium.org/1219683014/diff/750001/chrome/android/javates... chrome/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestAnnotationTest.java:335: public void testSingleTestParameterArgumentsWithoutParameterSet() { This is a typical place where displaying the input and expected values in the output would be immensely helpful.
skirmani@google.com changed reviewers: - mikecase@chromium.org, rnephew@chromium.org
PTAL! https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:62: * @return the {@link Parameter.Reader} for the current {@link ParameterizedTest} being run. On 2015/08/12 16:40:23, nyquist wrote: > Nit: This isn't currently imported, so either import it, or use the full package > name here. Done. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java (right): https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:110: * If {@link org.chromium.base.test.util.MinAndroidSdkLevel} is present, checks its value On 2015/08/12 16:40:23, nyquist wrote: > Optional nit: While you're at it, this fully qualified package name is > unnecessary, since it's already imported. Done. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:160: private static <T extends TestCase & Parameterizable> void setUpParameters(T test, On 2015/08/12 16:40:23, nyquist wrote: > The test parameter is unused here and below. Is that intentional? Done. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:170: private static <T extends TestCase & Parameterizable> void tearDownParameters(T test, On 2015/08/12 16:40:23, nyquist wrote: > The test parameter is unused here and below. Is that intentional? Done. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:212: public String toString() { On 2015/08/12 16:40:23, nyquist wrote: > Could you put parts of the output example into the code as the comment so other > can read it while they look at the code? Done. There's also an example of a few test errors here: https://docs.google.com/a/google.com/document/d/1T-FFTY1W4quFDFyZxWYcxlX6wPeY... https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:225: private StringBuilder errorBuilder(ParameterError error) { On 2015/08/12 16:40:23, nyquist wrote: > Nit: static? Done. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:233: StringBuilder parametersString = new StringBuilder(); On 2015/08/12 16:40:23, nyquist wrote: > You don't technically know the underlying implementation of the List you are > iterating over, right? So worst case your code is doing an O(N) lookup for each > call to get(i). > > I guess you could do something like: > > private static String createTagList(List<Parameter> parameters) { > Iterator<Parameter> it = parameters.iterator(); > if (!it.hasNext()) return ""; > StringBuilder tags = new StringBuilder(); > tags.append(it.next().tag()); > while (it.hasNext()) { > tags.append(",").append(it.next().tag()); > } > return tags.toString(); > } Done. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:238: builder.append(String.format("%n%s (with parameters %s)%n", On 2015/08/12 16:40:23, nyquist wrote: > I think we really want to print the key=value parts of the failed test here. > Otherwise you lose valuable information, and in the case where you use this for > a matrix of data input, like in the last fibonacci-test you wrote, you would > want to know which fibonacci input and output failed, not just that "it failed". Done. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:277: * @return The target {@link android.content.Context} if available; null otherwise. On 2015/08/12 16:40:23, nyquist wrote: > Nit: Just use {@link Context}, since it's already imported. Done. https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java (right): https://codereview.chromium.org/1219683014/diff/750001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:117: * a {@link Paramter.Argument}. On 2015/08/12 16:40:24, nyquist wrote: > Nit: {@link Parameter.Argument} Done. https://codereview.chromium.org/1219683014/diff/750001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestAnnotationTest.java (right): https://codereview.chromium.org/1219683014/diff/750001/chrome/android/javates... chrome/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestAnnotationTest.java:15: * Tester class for the {@link ParameterizedTest} annotation and the {@link ParameterizedTestSet} On 2015/08/12 16:40:24, nyquist wrote: > Nit: {@link ParameterizedTest.Set} Done. https://codereview.chromium.org/1219683014/diff/750001/chrome/android/javates... chrome/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestAnnotationTest.java:293: * {@link ParameterTest} annotations by using an intArray instead of an intVar. Computationally, On 2015/08/12 16:40:24, nyquist wrote: > Nit: {@link ParameterizedTest} ? Done. https://codereview.chromium.org/1219683014/diff/750001/chrome/android/javates... chrome/android/javatests/src/org/chromium/base/test/util/parameter/ParameterizedTestAnnotationTest.java:335: public void testSingleTestParameterArgumentsWithoutParameterSet() { On 2015/08/12 16:40:24, nyquist wrote: > This is a typical place where displaying the input and expected values in the > output would be immensely helpful. Done.
https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:216: * {{ERROR}} is the standard error output from {@link Throwable.getThrowable().toString()}. Nit: {@link ParameterError#getTh...} https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:217: * {{PARAMETER_TAG}} is the {@link Parameter.tag()} value associated with the parameter. {@link Parameter#tag()} here and #name() below https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:253: builder.append(createErrorBuilder(mErrors.get(i))).append("\n"); I this also has the same issue with iterating over lists and calling get(i). https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:312: return trim(builder.delete(0, builder.indexOf("\n") + 1)); Maybe just me, but maybe you could extract a method like: private static StringBuilder deleteFirstLine(StringBuilder builder) { return builder.delete(0, builder.indexOf("\n") + 1); } That makes it a little bit clearer what those magic indexes do. https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:322: nonWhitespaceCharFound = true; Couldn't you just |return sb;| here, and remove the boolean alltogether? https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java (right): https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:40: static final class ArgumentDefault { Nit: |static| modifier is redundant for inner classes of interfaces.
PTAL! https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:216: * {{ERROR}} is the standard error output from {@link Throwable.getThrowable().toString()}. On 2015/08/12 22:12:56, nyquist wrote: > Nit: {@link ParameterError#getTh...} Done. https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:217: * {{PARAMETER_TAG}} is the {@link Parameter.tag()} value associated with the parameter. On 2015/08/12 22:12:56, nyquist wrote: > {@link Parameter#tag()} here and #name() below Done. https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:253: builder.append(createErrorBuilder(mErrors.get(i))).append("\n"); On 2015/08/12 22:12:56, nyquist wrote: > I this also has the same issue with iterating over lists and calling get(i). Whoops! I missed that. Fixed! https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:312: return trim(builder.delete(0, builder.indexOf("\n") + 1)); On 2015/08/12 22:12:56, nyquist wrote: > Maybe just me, but maybe you could extract a method like: > > private static StringBuilder deleteFirstLine(StringBuilder builder) { > return builder.delete(0, builder.indexOf("\n") + 1); > } > > That makes it a little bit clearer what those magic indexes do. Done. https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:322: nonWhitespaceCharFound = true; On 2015/08/12 22:12:57, nyquist wrote: > Couldn't you just |return sb;| here, and remove the boolean alltogether? I didn't think of that. I like it! Fixed. https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java (right): https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:40: static final class ArgumentDefault { On 2015/08/12 22:12:57, nyquist wrote: > Nit: |static| modifier is redundant for inner classes of interfaces. Done.
PTAL! https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:216: * {{ERROR}} is the standard error output from {@link Throwable.getThrowable().toString()}. On 2015/08/12 22:12:56, nyquist wrote: > Nit: {@link ParameterError#getTh...} Done. https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:217: * {{PARAMETER_TAG}} is the {@link Parameter.tag()} value associated with the parameter. On 2015/08/12 22:12:56, nyquist wrote: > {@link Parameter#tag()} here and #name() below Done. https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:253: builder.append(createErrorBuilder(mErrors.get(i))).append("\n"); On 2015/08/12 22:12:56, nyquist wrote: > I this also has the same issue with iterating over lists and calling get(i). Whoops! I missed that. Fixed! https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:312: return trim(builder.delete(0, builder.indexOf("\n") + 1)); On 2015/08/12 22:12:56, nyquist wrote: > Maybe just me, but maybe you could extract a method like: > > private static StringBuilder deleteFirstLine(StringBuilder builder) { > return builder.delete(0, builder.indexOf("\n") + 1); > } > > That makes it a little bit clearer what those magic indexes do. Done. https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:322: nonWhitespaceCharFound = true; On 2015/08/12 22:12:57, nyquist wrote: > Couldn't you just |return sb;| here, and remove the boolean alltogether? I didn't think of that. I like it! Fixed. https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java (right): https://codereview.chromium.org/1219683014/diff/770001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameter.java:40: static final class ArgumentDefault { On 2015/08/12 22:12:57, nyquist wrote: > Nit: |static| modifier is redundant for inner classes of interfaces. Done.
just one last comment, otherwise lgtm https://codereview.chromium.org/1219683014/diff/790001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/790001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:256: if (iter.hasNext()) { You could do this check and append this before the while-loop to not do the if-check within the while loop. Like you've done on the other ones? Something along the lines of: if (iter.hasNext()) { builder.append(createErrorBuilder(iter.next())); } while (iter.hasNext()) { builder.append("\n"); builder.append(createErrorBuilder(iter.next())); }
skirmani@google.com changed reviewers: - jbudorick@chromium.org
https://codereview.chromium.org/1219683014/diff/790001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java (right): https://codereview.chromium.org/1219683014/diff/790001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:256: if (iter.hasNext()) { On 2015/08/12 22:53:42, nyquist wrote: > You could do this check and append this before the while-loop to not do the > if-check within the while loop. Like you've done on the other ones? Something > along the lines of: > > if (iter.hasNext()) { > builder.append(createErrorBuilder(iter.next())); > } > while (iter.hasNext()) { > builder.append("\n"); > builder.append(createErrorBuilder(iter.next())); > } Done.
lgtm. shipit!
The CQ bit was checked by skirmani@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, mikecase@chromium.org, rnephew@chromium.org Link to the patchset: https://codereview.chromium.org/1219683014/#ps810001 (title: "Tommy's LGTM Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1219683014/810001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1219683014/810001
Message was sent while issue was closed.
Committed patchset #42 (id:810001)
Message was sent while issue was closed.
Patchset 42 (id:??) landed as https://crrev.com/01780b3cebecf652aa7d7cf41c781ba77d73aff8 Cr-Commit-Position: refs/heads/master@{#343131} |
