Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(138)

Issue 2568633002: Create Next Gen Parameter Test Framework for JUnit4 (Closed)

Created:
4 years ago by the real yoland
Modified:
3 years, 4 months ago
CC:
bsheedy, chromium-reviews, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Create Next Gen Parameter Test Framework for JUnit4 This test framework enables developers to write parameterized tests with simple static field. The framework allows both test class parameterization (through constructor arguments injection) and test method parameterization (through test method arguments injection). For more detail on how to write parameterized tests using this framework, please check the crbug or ExampleParameterizedTest.java file. BUG=673092 Review-Url: https://codereview.chromium.org/2568633002 Cr-Commit-Position: refs/heads/master@{#493605} Committed: https://chromium.googlesource.com/chromium/src/+/fafc82e1442b3a6d2c2a7b9e364e30b76647322a

Patch Set 1 #

Total comments: 42

Patch Set 2 : Add Unit test and changes after +jbudorick comment #

Patch Set 3 : Add Unit test and changes after +jbudorick comment #

Total comments: 24

Patch Set 4 : Add test name and description #

Patch Set 5 : Address +mikecase comments and enforce acceptible types #

Total comments: 44

Patch Set 6 : name change/comments after +jbudorick #

Total comments: 42

Patch Set 7 : Remove InnerParameterizedRunner and refactor after +mikecase and +jbudorick's comments #

Total comments: 82

Patch Set 8 : Remove all builders, change naming style #

Total comments: 83

Patch Set 9 : Address +jbudorick's comments #

Patch Set 10 : Clear syntax error #

Patch Set 11 : Fix findbug errors #

Total comments: 39

Patch Set 12 : address comments #

Patch Set 13 : find bug issue #

Total comments: 2

Patch Set 14 : Limit to only one class parameter set #

Total comments: 24

Patch Set 15 : Address comments #

Total comments: 32

Patch Set 16 : address issues #

Total comments: 3

Patch Set 17 : add callable and assertion message #

Patch Set 18 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1512 lines, -0 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +13 lines, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +41 lines, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +40 lines, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/params/ParameterAnnotations.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +58 lines, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +128 lines, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +94 lines, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +239 lines, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegate.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +34 lines, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateCommon.java View 1 2 3 4 5 6 7 8 9 1 chunk +42 lines, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +156 lines, -0 lines 0 comments Download
A base/test/android/junit/src/org/chromium/base/test/params/ExampleParameterizedTest.java View 1 2 3 4 5 6 7 8 1 chunk +78 lines, -0 lines 0 comments Download
A base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactoryTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +191 lines, -0 lines 0 comments Download
A base/test/android/junit/src/org/chromium/base/test/params/ParameterizedRunnerTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +201 lines, -0 lines 0 comments Download
A base/test/android/junit/src/org/chromium/base/test/params/ParameterizedTestNameTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +197 lines, -0 lines 0 comments Download

Messages

Total messages: 89 (42 generated)
the real yoland
Sorry for the Friday evening CL! I had a lot of fun creating this one, ...
4 years ago (2016-12-10 07:57:38 UTC) #2
jbudorick
This CL appears only partially complete given the number of incomplete comments, TODOs, minor syntax ...
4 years ago (2016-12-12 19:04:49 UTC) #3
the real yoland
https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java File base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java#newcode1 base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java:1: package org.chromium.base.test; On 2016/12/12 at 19:04:48, jbudorick wrote: > ...
4 years ago (2016-12-13 23:31:19 UTC) #4
mikecase (-- gone --)
Have a bunch of old comments. Taking a break. Will finish reviewing this afternoon. https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java ...
4 years ago (2016-12-14 19:25:32 UTC) #5
mikecase (-- gone --)
How does the RunnerDelegate get specified? Can you add a second Param to @RunWith(ParamRunner.class, MyDelegate.class)? ...
4 years ago (2016-12-15 01:47:49 UTC) #6
the real yoland
https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java File base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java (right): https://codereview.chromium.org/2568633002/diff/1/base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java#newcode82 base/test/android/javatests/src/org/chromium/base/test/ParamRunner.java:82: * } On 2016/12/14 at 19:25:31, mikecase wrote: > ...
4 years ago (2016-12-16 20:49:16 UTC) #7
jbudorick
This is going to sound stupid, but we need to fix the names in here ...
3 years, 11 months ago (2017-01-03 23:03:01 UTC) #8
mikecase (-- gone --)
Best renamings I could think of are something along the lines of .... ParameterSet -> ...
3 years, 11 months ago (2017-01-05 16:50:03 UTC) #9
mikecase (-- gone --)
Best renamings I could think of are something along the lines of .... ParameterSet -> ...
3 years, 11 months ago (2017-01-05 16:50:04 UTC) #10
the real yoland
https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java File base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java (right): https://codereview.chromium.org/2568633002/diff/80001/base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java#newcode29 base/test/android/javatests/src/org/chromium/base/test/params/InnerParamClassRunner.java:29: * InnerParamClassRunner contains a class runner delegate that can ...
3 years, 11 months ago (2017-01-11 02:31:08 UTC) #11
mikecase (-- gone --)
Looks much better and is way more readable. Partial review. Mostly spelling nit. https://codereview.chromium.org/2568633002/diff/100001/base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java File ...
3 years, 11 months ago (2017-01-11 19:07:50 UTC) #12
jbudorick
I mentioned this to case offline, but we're going to have to do some more ...
3 years, 11 months ago (2017-01-11 19:57:32 UTC) #13
the real yoland
https://codereview.chromium.org/2568633002/diff/100001/base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java File base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/100001/base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java#newcode1 base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. ...
3 years, 11 months ago (2017-01-12 06:18:18 UTC) #14
the real yoland
Now ParameterizedFactory just instantiate a new custom ParameterizedRunnerDelegate.
3 years, 11 months ago (2017-01-12 06:20:21 UTC) #15
the real yoland
This is the diff between patch 5 and 6: https://codereview.chromium.org/2568633002/diff2/100001:120001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunner.java?context=10&column_width=80&tab_spaces=8 Biggest change for patch 6: ...
3 years, 11 months ago (2017-01-12 06:23:32 UTC) #16
the real yoland
On 2017/01/12 at 06:23:32, the real yoland wrote: > This is the diff between patch ...
3 years, 11 months ago (2017-01-12 06:24:20 UTC) #17
mikecase (-- gone --)
All nits. Sending out what I have now https://codereview.chromium.org/2568633002/diff/120001/base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java File base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java#newcode30 base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:30: * ...
3 years, 11 months ago (2017-01-13 16:50:44 UTC) #18
jbudorick
partial. Need to go back over ParameterizedRunner and ParameterizedRunnerFactory. https://codereview.chromium.org/2568633002/diff/120001/base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java File base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/120001/base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java#newcode30 base/test/android/javatests/src/org/chromium/base/test/params/BaseJUnit4RunnerDelegate.java:30: ...
3 years, 11 months ago (2017-01-13 21:56:00 UTC) #19
the real yoland
I changed the naming style as John brought up that having the parameterized test look ...
3 years, 11 months ago (2017-01-26 00:49:13 UTC) #20
jbudorick
I did not review the tests. There are two things that really stand out to ...
3 years, 10 months ago (2017-02-03 16:48:13 UTC) #21
the real yoland
https://codereview.chromium.org/2568633002/diff/140001/base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java File base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java (right): https://codereview.chromium.org/2568633002/diff/140001/base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java#newcode16 base/test/android/javatests/src/org/chromium/base/test/params/BlockJUnit4RunnerDelegate.java:16: public class BlockJUnit4RunnerDelegate On 2017/02/03 at 16:48:11, jbudorick wrote: ...
3 years, 9 months ago (2017-03-10 03:08:37 UTC) #22
the real yoland
friendly ping :)
3 years, 9 months ago (2017-03-17 17:43:19 UTC) #35
jbudorick
Once again, I'm sorry about the delay here. I missed that this was ready for ...
3 years, 6 months ago (2017-06-06 16:22:59 UTC) #36
the real yoland
:] https://codereview.chromium.org/2568633002/diff/220001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/220001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java#newcode20 base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:20: * A set of parameters that are used ...
3 years, 5 months ago (2017-07-19 03:44:32 UTC) #39
the real yoland
3 years, 5 months ago (2017-07-21 02:58:16 UTC) #46
mikecase (-- gone --)
Need to look at the test classes. But this looks good as is. https://codereview.chromium.org/2568633002/diff/220001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java File ...
3 years, 5 months ago (2017-07-21 15:56:03 UTC) #47
mikecase (-- gone --)
https://codereview.chromium.org/2568633002/diff/280001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java#newcode127 base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:127: + " ParameterUtil.createList<ParameterSet>();%n" This doc also might need to ...
3 years, 5 months ago (2017-07-21 16:44:57 UTC) #48
mikecase (-- gone --)
lgtm after fixing nits + the 1 actual comment I had. https://codereview.chromium.org/2568633002/diff/280001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedFrameworkMethod.java (right): ...
3 years, 5 months ago (2017-07-21 17:40:57 UTC) #49
the real yoland
https://codereview.chromium.org/2568633002/diff/280001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterAnnotations.java File base/test/android/javatests/src/org/chromium/base/test/params/ParameterAnnotations.java (right): https://codereview.chromium.org/2568633002/diff/280001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterAnnotations.java#newcode49 base/test/android/javatests/src/org/chromium/base/test/params/ParameterAnnotations.java:49: * Annotation for test class, it specifies which ParameteriezRunnerDelegate ...
3 years, 5 months ago (2017-07-21 18:53:32 UTC) #50
the real yoland
friendly ping, PTAL :)
3 years, 4 months ago (2017-08-07 21:33:21 UTC) #55
the real yoland
3 years, 4 months ago (2017-08-08 00:07:49 UTC) #56
jbudorick
Looks very good. Will be an LG once you address ParameterizedRunnerDelegateFactoryTest https://codereview.chromium.org/2568633002/diff/300001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): ...
3 years, 4 months ago (2017-08-08 14:44:27 UTC) #57
the real yoland
Diff from last patch: https://codereview.chromium.org/2568633002/diff2/300001:360001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java https://codereview.chromium.org/2568633002/diff/300001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java File base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java#newcode65 base/test/android/javatests/src/org/chromium/base/test/params/ParameterSet.java:65: @SuppressFBWarnings("NP_LOAD_OF_KNOWN_NULL_VALUE") On 2017/08/08 at ...
3 years, 4 months ago (2017-08-08 23:19:33 UTC) #60
the real yoland
On 2017/08/08 at 14:44:27, jbudorick wrote: > Looks very good. Will be an LG once ...
3 years, 4 months ago (2017-08-08 23:21:09 UTC) #61
jbudorick
at long last: lgtm w/ nit https://codereview.chromium.org/2568633002/diff/300001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/300001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java#newcode73 base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:73: List<FrameworkMethod> returnList = ...
3 years, 4 months ago (2017-08-10 15:53:13 UTC) #66
the real yoland
https://codereview.chromium.org/2568633002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java#newcode97 base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:97: Assert.assertArrayEquals(tagToParameterSetList.keySet().toArray(), On 2017/08/10 at 15:53:13, jbudorick wrote: > nit: ...
3 years, 4 months ago (2017-08-10 21:43:48 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2568633002/380001
3 years, 4 months ago (2017-08-10 21:44:04 UTC) #70
commit-bot: I haz the power
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/397685)
3 years, 4 months ago (2017-08-10 21:48:14 UTC) #72
jbudorick
https://codereview.chromium.org/2568633002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): https://codereview.chromium.org/2568633002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java#newcode97 base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java:97: Assert.assertArrayEquals(tagToParameterSetList.keySet().toArray(), On 2017/08/10 21:43:48, the real yoland wrote: > ...
3 years, 4 months ago (2017-08-10 21:50:21 UTC) #73
the real yoland
On 2017/08/10 at 21:50:21, jbudorick wrote: > https://codereview.chromium.org/2568633002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java > File base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java (right): > > https://codereview.chromium.org/2568633002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/params/ParameterizedRunnerDelegateFactory.java#newcode97 ...
3 years, 4 months ago (2017-08-10 21:53:52 UTC) #74
the real yoland
On 2017/08/10 at 21:53:52, the real yoland wrote: > On 2017/08/10 at 21:50:21, jbudorick wrote: ...
3 years, 4 months ago (2017-08-10 21:54:09 UTC) #75
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2568633002/400001
3 years, 4 months ago (2017-08-10 21:54:24 UTC) #78
the real yoland
3 years, 4 months ago (2017-08-10 22:47:42 UTC) #81
the real yoland
+nyquist for owner stamp
3 years, 4 months ago (2017-08-10 22:47:56 UTC) #82
nyquist
talked to jbudorick@ offline, so rs lgtm, but could you please extend the CL description ...
3 years, 4 months ago (2017-08-10 22:57:36 UTC) #83
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2568633002/400001
3 years, 4 months ago (2017-08-10 23:55:03 UTC) #86
commit-bot: I haz the power
3 years, 4 months ago (2017-08-11 00:03:49 UTC) #89
Message was sent while issue was closed.
Committed patchset #18 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/fafc82e1442b3a6d2c2a7b9e364e...

Powered by Google App Engine
This is Rietveld 408576698