Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2273553002:
Create BaseJUnitClassRunner to run junit4 style tests (Closed)
|
Created:
4 years, 4 months ago by the real yoland Modified:
4 years, 2 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate BaseJUnitClassRunner to run junit4 style tests
The example CL to change tests (WebViewLayoutTest) to Junit4 style is this: https://codereview.chromium.org/2266353003
BUG=640116
Committed: https://crrev.com/c300a782cfca83a3486b8ff0d6f58d96db21984a
Cr-Commit-Position: refs/heads/master@{#426889}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Response to comments #Patch Set 3 : gn file change #Patch Set 4 : Add skip checks tests #
Total comments: 18
Patch Set 5 : T #Patch Set 6 : S #Patch Set 7 : M #
Total comments: 3
Patch Set 8 : WIN! #Patch Set 9 : Minor fixes #
Total comments: 3
Patch Set 10 : Support for PreTestHook (e.g. CommandlineFlag) #Patch Set 11 : Support for PreTestHook (e.g. CommandlineFlag) #Patch Set 12 : Rebase #
Total comments: 10
Patch Set 13 : Change after +jbudorick's comment #Patch Set 14 : Change after +jbudorick's comment #
Total comments: 12
Patch Set 15 : Do less in constructor #Patch Set 16 : Do less in constructor #
Total comments: 9
Patch Set 17 : x #Patch Set 18 : Change after +jbudorick's comments #
Total comments: 6
Patch Set 19 : Last change! #Patch Set 20 : Rebase/minor fix #Patch Set 21 : presubmit errors #
Total comments: 1
Patch Set 22 : Stupid import order #Patch Set 23 : Rebase #Patch Set 24 : Add proguard config file #Patch Set 25 : Do not obfuscate junit #Messages
Total messages: 85 (29 generated)
yolandyan@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
Description was changed from ========== Create BaseJUnitClassRunner to run junit4 style tests BUG=640116 ========== to ========== Create BaseJUnitClassRunner to run junit4 style tests The example CL to change tests (WebViewLayoutTest) to Junit4 style is this: https://codereview.chromium.org/2266353003 BUG=640116 ==========
The example CL to change tests (WebViewLayoutTest) to Junit4 style is this: https://codereview.chromium.org/2266353003
https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:35: public boolean shouldSkip(TestCase testCase) { Removing the TestCase version of this will break SkipChecks on all JUnit3-based tests.
https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:35: public boolean shouldSkip(TestCase testCase) { On 2016/08/23 at 16:13:44, jbudorick wrote: > Removing the TestCase version of this will break SkipChecks on all JUnit3-based tests. hmm, I thought it would still work when shouldSkip(testCase) simply gets converted to shouldSkip(frameworkMethod)
https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java:31: protected boolean isIgnored(FrameworkMethod child) { Why is the variable name child? https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:61: a = (T) parentClass.getAnnotation(annotationClass); why do you have to cast this to (T) here but not in the call to getAnnotation above?
https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java:31: protected boolean isIgnored(FrameworkMethod child) { On 2016/08/23 at 17:41:00, mikecase wrote: > Why is the variable name child? copy pasta from parent class ;] Changed https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:61: a = (T) parentClass.getAnnotation(annotationClass); On 2016/08/23 at 17:41:00, mikecase wrote: > why do you have to cast this to (T) here but not in the call to getAnnotation above? After looking into this for eternity, I realized it's because I didn't declare parentClass to be Class<?> parentClass = ... and parentClass is raw type. Changed
I believe this CL is ready to leave "Limbo of forgotten CLs" go to "LGTM heaven"? (or maybe infinite refactor hell) https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:49: protected static <T extends Annotation> List<T> getAnnotations(Method method, Since getAnnotations is always called on Method objects, might as well only supports `getAnnotation(Method m, Class<T>aC)` https://codereview.chromium.org/2273553002/diff/60001/base/test/android/junit... File base/test/android/junit/src/org/chromium/base/test/util/RestrictionSkipCheckTest.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/junit... base/test/android/junit/src/org/chromium/base/test/util/RestrictionSkipCheckTest.java:87: new UnannotatedBaseClass("restrictedMethod"))); These tests remain unchanged since it covers shouldSkip(TestCase t) which calls shouldSkip(FrameworkMethod m). https://codereview.chromium.org/2273553002/diff/60001/base/test/android/junit... File base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java (left): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/junit... base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java:58: public void getAnnotationsForClassNone() { Since `getAnnotation(AnnotatedElement e, Class<T> c)` is gone (one can't use SkipCheck to find Annotations from a class instance), no need for these tests
https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java:33: protected void setSkipChecks() { How will this work with subclass skip checks (e.g. ChromeRestrictionSkipCheck)? https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:38: protected static FrameworkMethod getTestFrameworkMethod(TestCase testCase) { Why is this protected and not private? https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:49: protected static <T extends Annotation> List<T> getAnnotations(Method method, On 2016/08/26 19:39:06, the real yoland wrote: > Since getAnnotations is always called on Method objects, might as well only > supports `getAnnotation(Method m, Class<T>aC)` I'm a bit hesitant about the change -- getAnnotations(AnnotatedElement, Class<T>) seems like a reasonable base implementation. The FrameworkMethod overload could easily be implemented as ... getAnnotations(FrameworkMethod m, Class<T> a) { return getAnnotations(m.getMethod(), a); }
yolandyan@chromium.org changed reviewers: + aluo@chromium.org
this looks good to me. https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. Woohoo! This looks very clean and nice :D https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:24: public boolean shouldSkip(FrameworkMethod method) { nit: maybe change method to testMethod. But doesn't matter to me https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/MinAndroidSdkLevelSkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/MinAndroidSdkLevelSkipCheck.java:27: public boolean shouldSkip(FrameworkMethod method) { nit: consider renaming method to testMethod https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:51: FrameworkMethod fMethod = new FrameworkMethod(method); s/fMethod/frameworkMethod
https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/08/26 at 21:35:15, mikecase wrote: > Woohoo! This looks very clean and nice :D ;) https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitClassRunner.java:33: protected void setSkipChecks() { On 2016/08/26 at 21:07:56, jbudorick wrote: > How will this work with subclass skip checks (e.g. ChromeRestrictionSkipCheck)? It require a new extended class runner let's say, ChromeJUnitClassRunner to extend this class and override the setSkipChecks method. https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:24: public boolean shouldSkip(FrameworkMethod method) { On 2016/08/26 at 21:35:15, mikecase wrote: > nit: maybe change method to testMethod. But doesn't matter to me Done https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/MinAndroidSdkLevelSkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/MinAndroidSdkLevelSkipCheck.java:27: public boolean shouldSkip(FrameworkMethod method) { On 2016/08/26 at 21:35:15, mikecase wrote: > nit: consider renaming method to testMethod Done https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:38: protected static FrameworkMethod getTestFrameworkMethod(TestCase testCase) { On 2016/08/26 at 21:07:56, jbudorick wrote: > Why is this protected and not private? I unittest this method, but I see maybe I should just unittest shouldSkip(TestCase testCase) https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:49: protected static <T extends Annotation> List<T> getAnnotations(Method method, On 2016/08/26 at 21:07:56, jbudorick wrote: > On 2016/08/26 19:39:06, the real yoland wrote: > > Since getAnnotations is always called on Method objects, might as well only > > supports `getAnnotation(Method m, Class<T>aC)` > > I'm a bit hesitant about the change -- getAnnotations(AnnotatedElement, Class<T>) seems like a reasonable base implementation. The FrameworkMethod overload could easily be implemented as > > ... getAnnotations(FrameworkMethod m, Class<T> a) { > return getAnnotations(m.getMethod(), a); > } But getAnnotations only gets called from these test method anyway, I think it's clearer to get annotation from method once and then iteratively get annotation from class, class's parent, etc. https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:49: protected static <T extends Annotation> List<T> getAnnotations(Method method, On 2016/08/26 at 21:07:56, jbudorick wrote: > On 2016/08/26 19:39:06, the real yoland wrote: > > Since getAnnotations is always called on Method objects, might as well only > > supports `getAnnotation(Method m, Class<T>aC)` > > I'm a bit hesitant about the change -- getAnnotations(AnnotatedElement, Class<T>) seems like a reasonable base implementation. The FrameworkMethod overload could easily be implemented as > > ... getAnnotations(FrameworkMethod m, Class<T> a) { > return getAnnotations(m.getMethod(), a); > } Done https://codereview.chromium.org/2273553002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:51: FrameworkMethod fMethod = new FrameworkMethod(method); On 2016/08/26 at 21:35:15, mikecase wrote: > s/fMethod/frameworkMethod Done
lgtm https://codereview.chromium.org/2273553002/diff/120001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2273553002/diff/120001/base/BUILD.gn#newcode2350 base/BUILD.gn:2350: "//third_party/junit", as you mentioned, this isnt test_only, so you'll have to add/remove test_only to things to get this to build.
Removed `test_only=True` from targets and added TODOs https://codereview.chromium.org/2273553002/diff/120001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2273553002/diff/120001/base/BUILD.gn#newcode2350 base/BUILD.gn:2350: "//third_party/junit", On 2016/08/30 at 17:47:19, mikecase wrote: > as you mentioned, this isnt test_only, so you'll have to add/remove test_only to things to get this to build. Done
https://codereview.chromium.org/2273553002/diff/120001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2273553002/diff/120001/base/BUILD.gn#newcode2350 base/BUILD.gn:2350: "//third_party/junit", On 2016/08/30 22:16:58, the real yoland wrote: > On 2016/08/30 at 17:47:19, mikecase wrote: > > as you mentioned, this isnt test_only, so you'll have to add/remove test_only > to things to get this to build. > > Done I don't think this was the right way to fix this. base_java_test_support should be test-only. https://codereview.chromium.org/2273553002/diff/160001/third_party/android_su... File third_party/android_support_test_runner/BUILD.gn (left): https://codereview.chromium.org/2273553002/diff/160001/third_party/android_su... third_party/android_support_test_runner/BUILD.gn:8: testonly = true Bring this back. https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... File third_party/junit/BUILD.gn (left): https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... third_party/junit/BUILD.gn:9: testonly = true Bring this back.
https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... File third_party/junit/BUILD.gn (left): https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... third_party/junit/BUILD.gn:9: testonly = true On 2016/08/30 at 22:31:50, jbudorick wrote: > Bring this back. I recommended removing testonly here, and then in a separate CL adding testonly to everything. Since lots of things depend on base_java_test_support (which isn't currently testonly), testonly will need to be added in lots of places which may take a long time to get reviewed.
https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... File third_party/junit/BUILD.gn (left): https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... third_party/junit/BUILD.gn:9: testonly = true On 2016/08/30 at 22:31:50, jbudorick wrote: > Bring this back. I recommended removing testonly here, and then in a separate CL adding testonly to everything. Since lots of things depend on base_java_test_support (which isn't currently testonly), testonly will need to be added in lots of places which may take a long time to get reviewed.
On 2016/08/30 22:52:42, mikecase wrote: > https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... > File third_party/junit/BUILD.gn (left): > > https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... > third_party/junit/BUILD.gn:9: testonly = true > On 2016/08/30 at 22:31:50, jbudorick wrote: > > Bring this back. > > I recommended removing testonly here, and then in a separate CL adding testonly > to everything. Since lots of things depend on base_java_test_support (which > isn't currently testonly), testonly will need to be added in lots of places > which may take a long time to get reviewed. I disagree with that. If you want to break it up, add testonly to base_java_test_support first.
yolandyan@chromium.org changed reviewers: + perezju@chromium.org
On 2016/09/01 15:37:20, the real yoland wrote: ?
On 2016/09/01 at 16:39:09, jbudorick wrote: > On 2016/09/01 15:37:20, the real yoland wrote: > > ? sorry, was just adding +perezj as a reviewer.
On 2016/08/30 at 23:27:30, jbudorick wrote: > On 2016/08/30 22:52:42, mikecase wrote: > > https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... > > File third_party/junit/BUILD.gn (left): > > > > https://codereview.chromium.org/2273553002/diff/160001/third_party/junit/BUIL... > > third_party/junit/BUILD.gn:9: testonly = true > > On 2016/08/30 at 22:31:50, jbudorick wrote: > > > Bring this back. > > > > I recommended removing testonly here, and then in a separate CL adding testonly > > to everything. Since lots of things depend on base_java_test_support (which > > isn't currently testonly), testonly will need to be added in lots of places > > which may take a long time to get reviewed. > > I disagree with that. If you want to break it up, add testonly to base_java_test_support first. hmm, I am going to create a separate CL for that then (37 targets uses base_java_test_support).
friendly ping :) For +jbudorick's question on using Assume to skip test approach, I updated the comment in design doc (https://bugs.chromium.org/p/chromium/issues/detail?id=640116)
https://codereview.chromium.org/2273553002/diff/220001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2273553002/diff/220001/base/BUILD.gn#newcode2377 base/BUILD.gn:2377: "test/android/javatests/src/org/chromium/base/test/util/PreTestHook.java", This file is not in this CL. https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:29: private final List<PreTestHook> mPreTestHooks = new ArrayList<>(); How do pre-test hooks get added? https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:65: protected void setSkipChecks() { nit: addSkipChecks https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/MinAndroidSdkLevelSkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/MinAndroidSdkLevelSkipCheck.java:33: Log.i(TAG, "Test" + frameworkMethod.getDeclaringClass().getName() + "#" nit: why'd you remove the space after "Test"? https://codereview.chromium.org/2273553002/diff/220001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java (right): https://codereview.chromium.org/2273553002/diff/220001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java:49: private class ExtendsTestCaseClass extends TestCase { nit: drop this below ExtendsAnnotatedBaseClass
https://codereview.chromium.org/2273553002/diff/220001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2273553002/diff/220001/base/BUILD.gn#newcode2377 base/BUILD.gn:2377: "test/android/javatests/src/org/chromium/base/test/util/PreTestHook.java", On 2016/09/15 at 22:52:28, jbudorick wrote: > This file is not in this CL. I was hoping to extract PreTestHook interface out of BaseTestResult.java, and change all the current usage of PreTestHook. But I will do that in a seperate CL Removed https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:29: private final List<PreTestHook> mPreTestHooks = new ArrayList<>(); On 2016/09/15 at 22:52:29, jbudorick wrote: > How do pre-test hooks get added? They can be added by calling addPreTestHooks in the constructor(added). Current they are not here because the BaseJUnit4ClassRunner doesn't have preTestHooks, I added an empty method and javadoc Done https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:65: protected void setSkipChecks() { On 2016/09/15 at 22:52:29, jbudorick wrote: > nit: addSkipChecks Done https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/MinAndroidSdkLevelSkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/220001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/MinAndroidSdkLevelSkipCheck.java:33: Log.i(TAG, "Test" + frameworkMethod.getDeclaringClass().getName() + "#" On 2016/09/15 22:52:29, jbudorick wrote: > nit: why'd you remove the space after "Test"? Changed Done https://codereview.chromium.org/2273553002/diff/220001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java (right): https://codereview.chromium.org/2273553002/diff/220001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java:49: private class ExtendsTestCaseClass extends TestCase { On 2016/09/15 22:52:29, jbudorick wrote: > nit: drop this below ExtendsAnnotatedBaseClass Done.
https://codereview.chromium.org/2273553002/diff/260001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2273553002/diff/260001/base/BUILD.gn#newcode2374 base/BUILD.gn:2374: "test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java", nit: alphabetize https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:25: * The custom runner for JUnit4 tests that checks requirements to conditionally ignore tests. nit: The -> A https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:32: * Create a BaseJUnit4ClassRunner to run {@code testClass} and set an ArrayList of nit: testClass -> klass? https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:39: addSkipChecks(); I would not call these in the constructor. https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:44: * Evaluate whether {@link FrameworkMethod}s are ignored based on {@code SkipCheck}s nit: I think you need to specify the package for {@link FrameworkMethod} to work. https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:82: * TODO(yolandyan): Do not expose the method, only pass in a list of annotations. I don't agree with this TODO. SkipChecks aren't guaranteed to only deal with annotations.
https://codereview.chromium.org/2273553002/diff/260001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2273553002/diff/260001/base/BUILD.gn#newcode2374 base/BUILD.gn:2374: "test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java", On 2016/09/23 at 00:49:42, jbudorick wrote: > nit: alphabetize Done https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:25: * The custom runner for JUnit4 tests that checks requirements to conditionally ignore tests. On 2016/09/23 at 00:49:43, jbudorick wrote: > nit: The -> A Done https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:32: * Create a BaseJUnit4ClassRunner to run {@code testClass} and set an ArrayList of On 2016/09/23 at 00:49:43, jbudorick wrote: > nit: testClass -> klass? Done https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:39: addSkipChecks(); On 2016/09/23 at 00:49:42, jbudorick wrote: > I would not call these in the constructor. Done https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:44: * Evaluate whether {@link FrameworkMethod}s are ignored based on {@code SkipCheck}s On 2016/09/23 at 00:49:42, jbudorick wrote: > nit: I think you need to specify the package for {@link FrameworkMethod} to work. Done https://codereview.chromium.org/2273553002/diff/260001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:82: * TODO(yolandyan): Do not expose the method, only pass in a list of annotations. On 2016/09/23 at 00:49:43, jbudorick wrote: > I don't agree with this TODO. SkipChecks aren't guaranteed to only deal with annotations. Deleted
https://codereview.chromium.org/2273553002/diff/300001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2273553002/diff/300001/base/BUILD.gn#newcode2366 base/BUILD.gn:2366: "//third_party/android_support_test_runner:exposed_instrumentation_api_publish_java", Is this here for InstrumentationRegistry? https://codereview.chromium.org/2273553002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:32: * Create a BaseJUnit4ClassRunner to run {@code klass} and set an ArrayList of nit: update the comment, the SkipChecks are now added later. https://codereview.chromium.org/2273553002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:42: protected final List<FrameworkMethod> getChildren() { This seems like it's abusing the intent of getChildren. I'm wondering if there are other places we could put it. I'm also wondering if we should just have a constructor the takes checks and hooks as parameters and have derived classes pass whatever they want up. That way we could set these up in the ctor without depending on dynamic dispatch. https://codereview.chromium.org/2273553002/diff/300001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java (right): https://codereview.chromium.org/2273553002/diff/300001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java:143: Assert.assertTrue(TextUtils.equals(testMethodName, method.getName())); Why TextUtils.equals instead of String.equals?
https://codereview.chromium.org/2273553002/diff/300001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2273553002/diff/300001/base/BUILD.gn#newcode2366 base/BUILD.gn:2366: "//third_party/android_support_test_runner:exposed_instrumentation_api_publish_java", On 2016/09/29 at 03:45:58, jbudorick wrote: > Is this here for InstrumentationRegistry? Yes https://codereview.chromium.org/2273553002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:32: * Create a BaseJUnit4ClassRunner to run {@code klass} and set an ArrayList of On 2016/09/29 at 03:45:58, jbudorick wrote: > nit: update the comment, the SkipChecks are now added later. Changed https://codereview.chromium.org/2273553002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:42: protected final List<FrameworkMethod> getChildren() { On 2016/09/29 at 03:45:58, jbudorick wrote: > This seems like it's abusing the intent of getChildren. I'm wondering if there are other places we could put it. > > I'm also wondering if we should just have a constructor the takes checks and hooks as parameters and have derived classes pass whatever they want up. That way we could set these up in the ctor without depending on dynamic dispatch. After digging into all kinds of options - Computer: too abstract, used in JUnitCore, which doesn't have controller over tests - putting in other overridable methods: logically does not fit - Constructor: not great style I think initializing the values in constructor is still the best, I changed things around so the method called in constructor should only add value to the private list. https://codereview.chromium.org/2273553002/diff/300001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java (right): https://codereview.chromium.org/2273553002/diff/300001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/util/SkipCheckTest.java:143: Assert.assertTrue(TextUtils.equals(testMethodName, method.getName())); On 2016/09/29 at 03:45:58, jbudorick wrote: > Why TextUtils.equals instead of String.equals? I guess there is no reason to be null safe here Changed
https://codereview.chromium.org/2273553002/diff/300001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/300001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:42: protected final List<FrameworkMethod> getChildren() { On 2016/10/01 at 01:55:20, the real yoland wrote: > On 2016/09/29 at 03:45:58, jbudorick wrote: > > This seems like it's abusing the intent of getChildren. I'm wondering if there are other places we could put it. > > > > I'm also wondering if we should just have a constructor the takes checks and hooks as parameters and have derived classes pass whatever they want up. That way we could set these up in the ctor without depending on dynamic dispatch. > > After digging into all kinds of options > - Computer: too abstract, used in JUnitCore, which doesn't have controller over tests > - putting in other overridable methods: logically does not fit > - Constructor: not great style > > I think initializing the values in constructor is still the best, I changed things around so the method called in constructor should only add value to the private list. Just kidding, ignore this comment. I think I have reached tranquility with constructor parameter approach as you suggested
friendly ping :)
lgtm w/ nits https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:29: private static final List<SkipCheck> DEFAULT_SKIP_CHECKS = Arrays.asList(new SkipCheck[]{ nit: do this as a static method that returns a list rather than a static class member. https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:61: * public ChilcRunner(final Class<?> klass) { nit: Child https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:46: protected static FrameworkMethod getTestFrameworkMethod(TestCase testCase) { Nit: does this even need to exist separately from shouldSkip(TestCase)? It looks like the derived classes that had been using getTestMethod don't need to use getTestFrameworkMethod.
https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:29: private static final List<SkipCheck> DEFAULT_SKIP_CHECKS = Arrays.asList(new SkipCheck[]{ On 2016/10/04 at 19:12:21, jbudorick wrote: > nit: do this as a static method that returns a list rather than a static class member. Done https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:61: * public ChilcRunner(final Class<?> klass) { On 2016/10/04 at 19:12:21, jbudorick wrote: > nit: Child Done https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java (right): https://codereview.chromium.org/2273553002/diff/340001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/SkipCheck.java:46: protected static FrameworkMethod getTestFrameworkMethod(TestCase testCase) { On 2016/10/04 at 19:12:21, jbudorick wrote: > Nit: does this even need to exist separately from shouldSkip(TestCase)? It looks like the derived classes that had been using getTestMethod don't need to use getTestFrameworkMethod. Changed to put it inside shouldSkip(Testcase) Done
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2273553002/#ps360001 (title: "Last change!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by yolandyan@chromium.org
yolandyan@chromium.org changed reviewers: + agrieve@chromium.org
+agrieve for OWNER lgtm on `base/BUILD.gn` file
On 2016/10/04 22:54:36, the real yoland wrote: > +agrieve for OWNER lgtm on `base/BUILD.gn` file BUILD.gn lgtm
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2273553002/#ps380001 (title: "Rebase/minor fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2273553002/#ps400001 (title: "presubmit errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2273553002/diff/400001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java (right): https://codereview.chromium.org/2273553002/diff/400001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java:10: import org.chromium.base.test.BaseTestResult.PreTestHook; android.*, org.junit.*, org.chromium.*, java.*
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2273553002/#ps420001 (title: "Stupid import order")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by yolandyan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yolandyan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2273553002/#ps440001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by yolandyan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/10/08 00:49:38, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ./tools/mb/mb.py gen -m "tryserver.chromium.android" -b "linux_android_rel_ng" <output directory> ninja -C <output directory> [other build args] <targets>
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2273553002/#ps480001 (title: "Do not obfuscate junit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Create BaseJUnitClassRunner to run junit4 style tests The example CL to change tests (WebViewLayoutTest) to Junit4 style is this: https://codereview.chromium.org/2266353003 BUG=640116 ========== to ========== Create BaseJUnitClassRunner to run junit4 style tests The example CL to change tests (WebViewLayoutTest) to Junit4 style is this: https://codereview.chromium.org/2266353003 BUG=640116 Committed: https://crrev.com/c300a782cfca83a3486b8ff0d6f58d96db21984a Cr-Commit-Position: refs/heads/master@{#426889} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/c300a782cfca83a3486b8ff0d6f58d96db21984a Cr-Commit-Position: refs/heads/master@{#426889} |
