Parameterized reuses TestClass #1449

kcooney · 2017-05-05T21:52:40Z

No description provided.

kcooney · 2017-05-23T07:11:18Z

@stefanbirkner could you review this?

I would prefer to merge it myself (I split it into multiple commits intentionally)

stefanbirkner · 2017-05-23T07:37:12Z

I have a look.

panchenko · 2017-05-23T10:58:36Z

src/main/java/org/junit/runners/Suite.java

@@ -14,6 +14,7 @@
 import org.junit.runner.notification.RunNotifier;
 import org.junit.runners.model.InitializationError;
 import org.junit.runners.model.RunnerBuilder;
+import org.junit.runners.model.TestClass;


This import seems not needed.

panchenko · 2017-05-23T11:04:24Z

src/main/java/org/junit/internal/Checks.java

+     */
+    public static <T> T notNull(T value, String message) {
+        if (value == null) {
+            throw new NullPointerException(String.valueOf(message));


I would suggest passing message directly without String.valueOf().

message can be null

In this case this method would behave exactly as the one above, similarly to java.util.Objects.requireNonNull()

I conform to @panchenko's suggestion. I normally expect that someMethod(someValue) and someMethod(someValue, null) behave the same.

The behavior I have here matches Guava's Preconditions.checkNotNull(T, String) method. I think the Guava team has given this much more thought than I have, but I can take a stab at explaining why the behavior I have is better.

IMHO it is a programmer error to pass in a null as the second parameter. If we have this method behave like notNull(T) when you pass in a null message we would be ignoring that programmer error. Having the message be "null" makes it clear that 1) someone tried to specify a real message, and 2) they messed up. Hopefully someone would notice and fix it.

Edit: Just checked, and Objects.requireNotNull() just passes the message to the NullPointerException constructor, regardless of whether it is null, so I guess if you guys feel strongly about it I can do that here.

Yes, please, lets keep it simple. Also the 1.8 version with Supplier<String> passes the result directly to the NullPointerException constructor.

Changed notNull(T, String) to not use String.valueOf() on the message.

stefanbirkner · 2017-05-23T18:25:31Z

src/main/java/org/junit/internal/builders/JUnit4Builder.java

This change is unrelated to the reuse of TestClass. It should be a separate commit.

It is a separate commit. See 11822c4

I have this commit in this pull since there's no reason for ParentRunner.createTestClass() to be called for JUnit4-style tests that aren't annotated with RunWith.

Sorry, my fault. I was not aware that the PR is made up of multiple commits.

stefanbirkner · 2017-05-23T18:36:58Z

src/main/java/org/junit/internal/Checks.java

+package org.junit.internal;
+
+/** @since 4.13 */
+public final class Checks {


I think we should not extract a Checks class and simply inline the validations, in order to not provide methods someone may use and depend on. I know that is a very weak argument. I leave it up to you whether you want to keep it or not.

As you can see in the commits, we have multiple classes that do null checks. Quite often when I write code in JUnit I find myself wanting a one-liner to check for null.

I doubt anyone would depend on this class, since 1) it's in an internal package, 2) it has an odd name, and 3) most everyone that is using JDK 6 or later would use Objects.requireNotNull() or Guava's Preconditions.checkNotNull().

stefanbirkner

A few nit-picks only.

stefanbirkner · 2017-05-23T18:51:22Z

src/main/java/org/junit/internal/Checks.java

+     */
+    public static <T> T notNull(T value, String message) {
+        if (value == null) {
+            throw new NullPointerException(String.valueOf(message));


I conform to @panchenko's suggestion. I normally expect that someMethod(someValue) and someMethod(someValue, null) behave the same.

The `BlockJUnit4ClassRunnerWithParameters` created a new instance of `TestClass` for each parameter set. This led to repeated class scanning and noticeable memory allocation. Reusing the `TestClass` avoids theses side effects. Fixes junit-team#1046.

This avoids repeated class scanning.

…t annotated with @RunWith.

Closes junit-team#1449

kcooney · 2017-05-25T16:27:10Z

Thanks for the reviews! I rebased and waiting for Travis to complete before doing a merge.

Closes junit-team#1449

tasomaniac · 2020-01-02T10:37:49Z

How does this work when we provide the parameters through constructor? In such cases, the properties in test instance are private final

panchenko · 2020-01-02T11:22:38Z

@tasomaniac This issue is about a runner for parameterizzed tests - the tests are developed the same way as before.

tasomaniac · 2020-01-02T11:46:43Z

I see. The title says reuses TestClass. I thought TestClass is the instance of the test class we create.

Closes junit-team#1449

kcooney mentioned this pull request May 5, 2017

Parameterized reuses testclass #1057

Closed

kcooney force-pushed the parameterized-reuses-testclass branch 2 times, most recently from fbbae1e to c6bae86 Compare May 5, 2017 22:07

kcooney added the 4.13 label May 5, 2017

kcooney requested a review from stefanbirkner May 5, 2017 22:44

kcooney force-pushed the parameterized-reuses-testclass branch from e9719c9 to db629a0 Compare May 5, 2017 22:47

kcooney mentioned this pull request May 5, 2017

Allow custom test runners to create their own TestClasses and customize the scanning of annotations. #763

Merged

kcooney force-pushed the parameterized-reuses-testclass branch from db629a0 to 99f4378 Compare May 16, 2017 04:47

panchenko reviewed May 23, 2017

View reviewed changes

stefanbirkner reviewed May 23, 2017

View reviewed changes

stefanbirkner requested changes May 23, 2017

View reviewed changes

kcooney force-pushed the parameterized-reuses-testclass branch from 99f4378 to 8755bc1 Compare May 24, 2017 04:40

stefanbirkner approved these changes May 24, 2017

View reviewed changes

kcooney force-pushed the parameterized-reuses-testclass branch from 8755bc1 to 637b240 Compare May 25, 2017 04:37

panchenko approved these changes May 25, 2017

View reviewed changes

kcooney added 4 commits May 25, 2017 09:19

Update Theories to pass the TestClass to BlockJUnit4ClassRunner.

de3404d

This avoids repeated class scanning.

Avoid unnecessary call to ParentRunner.createTestClass() for tests no…

efdcb37

…t annotated with @RunWith.

Update ParentRunner(TestClass) to check for a null parameter.

f34c443

Closes junit-team#1449

kcooney force-pushed the parameterized-reuses-testclass branch from 637b240 to f34c443 Compare May 25, 2017 16:24

kcooney added the needs release notes update label May 25, 2017

kcooney merged commit f34c443 into junit-team:master May 26, 2017

kcooney modified the milestone: 4.13 Aug 6, 2017

kcooney removed the 4.13 label Aug 6, 2017

kcooney deleted the parameterized-reuses-testclass branch August 7, 2017 02:51

sebasjm pushed a commit to sebasjm/junit4 that referenced this pull request Mar 11, 2018

Update ParentRunner(TestClass) to check for a null parameter.

2b38983

Closes junit-team#1449

stefanbirkner removed the needs release notes update label Oct 1, 2018

aristotle0x01 pushed a commit to aristotle0x01/junit4 that referenced this pull request Jun 27, 2022

Update ParentRunner(TestClass) to check for a null parameter.

a2f6fc1

Closes junit-team#1449

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameterized reuses TestClass #1449

Parameterized reuses TestClass #1449

kcooney commented May 5, 2017

kcooney commented May 23, 2017 •

edited

Loading

stefanbirkner commented May 23, 2017

panchenko May 23, 2017

kcooney May 24, 2017

panchenko May 23, 2017

kcooney May 23, 2017

panchenko May 23, 2017

stefanbirkner May 23, 2017

kcooney May 24, 2017 •

edited

Loading

panchenko May 24, 2017

kcooney May 25, 2017

stefanbirkner May 23, 2017

kcooney May 24, 2017

stefanbirkner May 24, 2017 •

edited

Loading

stefanbirkner May 23, 2017

kcooney May 24, 2017

stefanbirkner left a comment

stefanbirkner May 23, 2017

kcooney commented May 25, 2017

tasomaniac commented Jan 2, 2020

panchenko commented Jan 2, 2020

tasomaniac commented Jan 2, 2020

Parameterized reuses TestClass #1449

Parameterized reuses TestClass #1449

Conversation

kcooney commented May 5, 2017

kcooney commented May 23, 2017 • edited Loading

stefanbirkner commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcooney May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanbirkner May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanbirkner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcooney commented May 25, 2017

tasomaniac commented Jan 2, 2020

panchenko commented Jan 2, 2020

tasomaniac commented Jan 2, 2020

kcooney commented May 23, 2017 •

edited

Loading

kcooney May 24, 2017 •

edited

Loading

stefanbirkner May 24, 2017 •

edited

Loading