Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@BeforeParam/@AfterParam for Parameterized runner #1435

Merged
merged 18 commits into from
Apr 21, 2017
Merged

@BeforeParam/@AfterParam for Parameterized runner #1435

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
066e1d6
@BeforeParam/@AfterParam for Parameterized runner
panchenko Apr 1, 2017
45064f7
Fix minor style issues
kcooney Apr 16, 2017
3390335
test for multiple @BeforeParam/@AfterParam
panchenko Apr 18, 2017
1de2311
test for @BeforeParam/@AfterParam with multiple parameters
panchenko Apr 18, 2017
15ac6ad
Validate number of parameters in @BeforeParam/@AfterParam
panchenko Apr 18, 2017
70245b3
code style
panchenko Apr 18, 2017
e270f2c
iterate parameters once
panchenko Apr 19, 2017
570ec30
style changes
panchenko Apr 19, 2017
754732b
Style fixes
kcooney Apr 19, 2017
35bbd8f
Make RunnersFactory.parameterCount final
kcooney Apr 20, 2017
301a6e0
Add test cases for @Parameters methods returning Collection
kcooney Apr 20, 2017
98e061a
reuse RunBefores/RunAfters
panchenko Apr 20, 2017
f406970
style
panchenko Apr 20, 2017
f7e3ad3
javadoc for @BeforeParam/@AfterParam
panchenko Apr 20, 2017
76c1779
update javadoc
panchenko Apr 20, 2017
03e598f
update header in javadoc
panchenko Apr 20, 2017
4958ea8
javadoc: "parameters"
panchenko Apr 20, 2017
ab15e7e
since 4.13
panchenko Apr 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 87 additions & 10 deletions src/main/java/org/junit/runners/Parameterized.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.junit.runners;

import java.lang.annotation.Annotation;
import java.lang.annotation.ElementType;
import java.lang.annotation.Inherited;
import java.lang.annotation.Retention;
Expand All @@ -9,10 +10,12 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;

import org.junit.runner.Runner;
import org.junit.runners.model.FrameworkMethod;
import org.junit.runners.model.InvalidTestClassError;
import org.junit.runners.model.TestClass;
import org.junit.runners.parameterized.BlockJUnit4ClassRunnerWithParametersFactory;
import org.junit.runners.parameterized.ParametersRunnerFactory;
Expand Down Expand Up @@ -234,35 +237,105 @@ public class Parameterized extends Suite {
Class<? extends ParametersRunnerFactory> value() default BlockJUnit4ClassRunnerWithParametersFactory.class;
}

/**
* Annotation for {@code public static void} methods which should be executed before
* evaluating tests with a particular parameter.
*
* @see org.junit.BeforeClass
* @see org.junit.Before
* @since 4.13
*/
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface BeforeParam {
}

/**
* Annotation for {@code public static void} methods which should be executed after
* evaluating tests with a particular parameter.
*
* @see org.junit.AfterClass
* @see org.junit.After
* @since 4.13
*/
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface AfterParam {
}

/**
* Only called reflectively. Do not use programmatically.
*/
public Parameterized(Class<?> klass) throws Throwable {
super(klass, RunnersFactory.createRunnersForClass(klass));
this(klass, new RunnersFactory(klass));
}

private Parameterized(Class<?> klass, RunnersFactory runnersFactory) throws Throwable {
super(klass, runnersFactory.createRunners());
validateBeforeParamAndAfterParamMethods(runnersFactory.getParameterCount());
}

private void validateBeforeParamAndAfterParamMethods(Integer parameterCount)
throws InvalidTestClassError {
List<Throwable> errors = new ArrayList<Throwable>();
validatePublicStaticVoidMethods(Parameterized.BeforeParam.class, parameterCount, errors);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we delete validateBeforeParamAndAfterParamMethods() and instead override collectInitializationErrors(List<Throwable>) and call validatePublicStaticVoidMethods there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that before, collectInitializationErrors() is called from the super class constructor and the number of method parameters can't be easily validated (only with some tricks, e.g. using ThreadLocal).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I've never been a fan of calling non-static methods in constructors. It's caused no end of problems in JUnit4. Hopefully they avoided that in the JUnit5 code base

validatePublicStaticVoidMethods(Parameterized.AfterParam.class, parameterCount, errors);
if (!errors.isEmpty()) {
throw new InvalidTestClassError(getTestClass().getJavaClass(), errors);
}
}

private void validatePublicStaticVoidMethods(
Class<? extends Annotation> annotation, Integer parameterCount,
List<Throwable> errors) {
List<FrameworkMethod> methods = getTestClass().getAnnotatedMethods(annotation);
for (FrameworkMethod fm : methods) {
fm.validatePublicVoid(true, errors);
if (parameterCount != null) {
final int methodParameterCount = fm.getMethod().getParameterTypes().length;
if (methodParameterCount != 0 && methodParameterCount != parameterCount) {
errors.add(new Exception("Method " + fm.getName()
+ "() should have 0 or " + parameterCount + " parameter(s)"));
}
}
}
}

private static class RunnersFactory {
private static final ParametersRunnerFactory DEFAULT_FACTORY = new BlockJUnit4ClassRunnerWithParametersFactory();

private final TestClass testClass;

static List<Runner> createRunnersForClass(Class<?> klass)
throws Throwable {
return new RunnersFactory(klass).createRunners();
}
private Iterable<Object> allParameters;

private RunnersFactory(Class<?> klass) {
testClass = new TestClass(klass);
}

private void evaluateParameters() throws Throwable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't thread-safe, which can be a problem if tests are run in parallel. Also, having this method have a side-effect of assigning allParameters is a bit odd.

If we really need to cache the result of calling allParameters() I would prefer that to happen inside of allParameters() itself. You could rename allParameters() to callParametersMethod() and then add this:

private volatile Iterable<Object> allParameters;

private Iterable<Object> allParameters() throws Throwable {
  if (allParameters == null) { // volatile read
    allParameters = callParametersMethod();
  }
  return allParameters;
}

(and then change all the places where you reference allParameters to call allParmeters() like was done before)

This won't guarantee that the the parameters method is only called once, but the only way to guarantee that would be
to have a lock that is held while we call the parameters method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread safety is not an issue here, as an instance of this inner class exists only during the Parameterized constructor invocation.

if (allParameters == null) {
allParameters = allParameters();
}
}

private List<Runner> createRunners() throws Throwable {
evaluateParameters();
Parameters parameters = getParametersMethod().getAnnotation(
Parameters.class);
return Collections.unmodifiableList(createRunnersForParameters(
allParameters(), parameters.name(),
allParameters, parameters.name(),
getParametersRunnerFactory()));
}

private Integer getParameterCount() throws Throwable {
evaluateParameters();
Iterator<Object> iterator = allParameters.iterator();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any guarantee that the Iterable can be iterated more than once. The current code only iterates over the collection once, so this could possibly break people.

I think we might need to wait until we are about to call a BeforeParam or AfterParam method before we check if the method has the right number of parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on iterating it multiple times. I would like to validate these methods once, otherwise an error would in each child runner (they are created for each parameter).

if (iterator.hasNext()) {
return normalizeParameters(iterator.next()).length;
} else {
return null;
}
}

private ParametersRunnerFactory getParametersRunnerFactory()
throws InstantiationException, IllegalAccessException {
UseParametersRunnerFactory annotation = testClass
Expand All @@ -278,10 +351,14 @@ private ParametersRunnerFactory getParametersRunnerFactory()

private TestWithParameters createTestWithNotNormalizedParameters(
String pattern, int index, Object parametersOrSingleParameter) {
Object[] parameters = (parametersOrSingleParameter instanceof Object[]) ? (Object[]) parametersOrSingleParameter
: new Object[] { parametersOrSingleParameter };
return createTestWithParameters(testClass, pattern, index,
parameters);
normalizeParameters(parametersOrSingleParameter));
}

private static Object[] normalizeParameters(Object parametersOrSingleParameter) {
return parametersOrSingleParameter instanceof Object[]
? (Object[]) parametersOrSingleParameter
: new Object[] { parametersOrSingleParameter };
}

@SuppressWarnings("unchecked")
Expand Down
71 changes: 70 additions & 1 deletion src/main/java/org/junit/runners/parameterized/BlockJUnit4ClassRunnerWithParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@

import java.lang.annotation.Annotation;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.List;

import org.junit.runner.RunWith;
import org.junit.runner.notification.RunNotifier;
import org.junit.runners.BlockJUnit4ClassRunner;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameter;
import org.junit.runners.model.FrameworkField;
import org.junit.runners.model.FrameworkMethod;
import org.junit.runners.model.InitializationError;
import org.junit.runners.model.MultipleFailureException;
import org.junit.runners.model.Statement;

/**
Expand Down Expand Up @@ -135,7 +138,73 @@ protected void validateFields(List<Throwable> errors) {

@Override
protected Statement classBlock(RunNotifier notifier) {
return childrenInvoker(notifier);
Statement statement = childrenInvoker(notifier);
statement = withBeforeParams(statement);
statement = withAfterParams(statement);
return statement;
}

private Statement withBeforeParams(Statement statement) {
List<FrameworkMethod> befores = getTestClass()
.getAnnotatedMethods(Parameterized.BeforeParam.class);
return befores.isEmpty() ? statement : new RunBeforeParams(statement, befores);
}

private class RunBeforeParams extends Statement {
private final Statement next;
private final List<FrameworkMethod> befores;

RunBeforeParams(Statement next, List<FrameworkMethod> befores) {
this.next = next;
this.befores = befores;
}

@Override
public void evaluate() throws Throwable {
for (FrameworkMethod before : befores) {
int paramCount = before.getMethod().getParameterTypes().length;
before.invokeExplosively(
null, paramCount == 0 ? (Object[]) null : parameters);
}
next.evaluate();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse org.junit.internal.runners.statements.RunBefores and pass parameters to it?


private Statement withAfterParams(Statement statement) {
List<FrameworkMethod> afters = getTestClass()
.getAnnotatedMethods(Parameterized.AfterParam.class);
return afters.isEmpty() ? statement : new RunAfterParams(statement, afters);
}

private class RunAfterParams extends Statement {
private final Statement next;
private final List<FrameworkMethod> afters;

RunAfterParams(Statement next, List<FrameworkMethod> afters) {
this.next = next;
this.afters = afters;
}

@Override
public void evaluate() throws Throwable {
List<Throwable> errors = new ArrayList<Throwable>();
try {
next.evaluate();
} catch (Throwable e) {
errors.add(e);
} finally {
for (FrameworkMethod each : afters) {
try {
int paramCount = each.getMethod().getParameterTypes().length;
each.invokeExplosively(
null, paramCount == 0 ? (Object[]) null : parameters);
} catch (Throwable e) {
errors.add(e);
}
}
}
MultipleFailureException.assertEmpty(errors);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse org.junit.internal.runners.statements.RunAfters and pass parameters to it?

}

@Override
Expand Down
Loading