@BeforeParam/@AfterParam for Parameterized runner #1435

panchenko · 2017-04-01T11:32:48Z

This PR addresses issue #45.
Some @ParamRule could also be introduced.

kcooney

Thanks for the contribution!

Sorry for the delay. I was hoping one of the other maintainers would get to this, since I'm not as familiar with Parameterized.

I pushed some minor style fixes to your branch. I think only thing missing is a bit of test coverage.

kcooney · 2017-04-16T08:04:01Z

src/test/java/org/junit/tests/running/classes/ParameterizedTestTest.java

+    }
+
+    @Test
+    public void beforeParamAndAfterParamAreRun() {


It would be great to have tests for

Multiple @BeforeParam methods

Multiple @AfterParam methods

@AfterParam methods with parameters

@BeforeParam methods without parameters

Parameterized tests with more than one parameter

@kcooney I've added some more tests and validation for number of parameters of the BeforeParam/AfterParam methods.
Do you think it's OK already?

panchenko · 2017-04-18T12:40:36Z

@kcooney Sounds good, I will add more tests.
Could you please clarify one thing on the style changes: the project uses google code style, the line length is 100 there, seems like lines were smaller already, however you still wrapped some, to make them even shorter.

kcooney · 2017-04-18T14:22:59Z

@panchenko yes we allow lines up to 100 characters. Sorry about that, I was reviewing the GitHub diff view when I did the cleanup. The only minor issue is that we do not use final for fields or parameters unless forced to by the compiler (so final on a field is a hint it is used in an anonymous class)

kcooney · 2017-04-18T23:13:15Z

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


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

+        private void evaluateParameters() throws Throwable {


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.

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

kcooney · 2017-04-18T23:19:44Z

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

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


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

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).

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

kcooney · 2017-04-18T23:29:15Z

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

                    getParametersRunnerFactory()));
        }

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


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.

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).

panchenko · 2017-04-19T07:44:04Z

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

        private TestWithParameters createTestWithNotNormalizedParameters(
                String pattern, int index, Object parametersOrSingleParameter) {
            Object[] parameters = (parametersOrSingleParameter instanceof Object[]) ? (Object[]) parametersOrSingleParameter
                    : new Object[] { parametersOrSingleParameter };
+            if (parameterCount == null) {
+                parameterCount = parameters.length;
+            }


As a side effect we could have a check that number of parameters is always the same.

If the number of parameters were different for some parameter sets, you would get a failure when the child runner is run.

and one-shot Iterables.

kcooney

Thanks for adding more tests!

kcooney · 2017-04-19T16:13:04Z

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

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


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

kcooney · 2017-04-20T05:11:57Z

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

        private TestWithParameters createTestWithNotNormalizedParameters(
                String pattern, int index, Object parametersOrSingleParameter) {
            Object[] parameters = (parametersOrSingleParameter instanceof Object[]) ? (Object[]) parametersOrSingleParameter
                    : new Object[] { parametersOrSingleParameter };
+            if (parameterCount == null) {
+                parameterCount = parameters.length;
+            }


If the number of parameters were different for some parameter sets, you would get a failure when the child runner is run.

kcooney · 2017-04-20T05:13:47Z

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

@@ -276,10 +330,15 @@ private ParametersRunnerFactory getParametersRunnerFactory()
            }
        }

+        private Integer parameterCount;


Having this set as a side-effect of calling createRunners() is fragile. It would be better to make this final and assign this at construction time. I added a commit that does ths.

panchenko · 2017-04-20T07:24:14Z

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

+            if (parameters instanceof List) {
+                return (List<Object>) parameters;
+            } else if (parameters instanceof Collection) {
+                return new ArrayList<Object>((Collection<Object>) parameters);


@kcooney IMHO there is no need to handle Collection specially, it can be treated as Iterable.

marcphilipp

Overall, I think it looks great! I would like to get rid of the duplication, though.

In addition, I think we should add an example to the Javadoc of Parameterized.

marcphilipp · 2017-04-20T08:48:31Z

src/main/java/org/junit/runners/parameterized/BlockJUnit4ClassRunnerWithParameters.java

+            }
+            next.evaluate();
+        }
+    }


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

marcphilipp · 2017-04-20T08:49:11Z

src/main/java/org/junit/runners/parameterized/BlockJUnit4ClassRunnerWithParameters.java

+                }
+            }
+            MultipleFailureException.assertEmpty(errors);
+        }


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

marcphilipp

Thanks for making the changes this fast! We're almost there, I have a few additional nitpicks.

marcphilipp · 2017-04-20T13:36:24Z

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

+ * <p>
+ * If your test needs to perform some preparation or cleanup based on the parameters this can be
+ * done by adding public static methods annotated with &#064;BeforeParam/&#064;AfterParam,
+ * such methods should either have no parameters or have same parameters as the test.


Please replace this paragraph with:

* If your test needs to perform some preparation or cleanup based on the * parameters, this can be done by adding public static methods annotated with * {@code @BeforeParam}/{@code @AfterParam}. Such methods should either have no * parameters or the same parameters as the test.

marcphilipp · 2017-04-20T13:37:08Z

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

+ *     System.out.println("Testing " + onlyParameter);
+ * }
+ * </pre>
+ * </p>


Please remove the </p> line.

kcooney · 2017-04-20T15:39:39Z

src/main/java/org/junit/internal/runners/statements/RunAfters.java

                } catch (Throwable e) {
                    errors.add(e);
                }
            }
        }
        MultipleFailureException.assertEmpty(errors);
    }
+
+    protected void invokeMethod(FrameworkMethod method) throws Throwable {


Please add @since 4.13

kcooney · 2017-04-20T15:39:51Z

src/main/java/org/junit/internal/runners/statements/RunBefores.java

        }
        next.evaluate();
    }
+
+    protected void invokeMethod(FrameworkMethod method) throws Throwable {


Please add @since 4.13

marcphilipp

LGTM! 👍

kcooney · 2017-04-21T00:20:21Z

Thanks, @panchenko ! Could you please update the release notes at https://github.com/junit-team/junit4/wiki/4.13-Release-Notes ?

panchenko · 2017-04-21T08:10:05Z

Updated it, thanks @kcooney!

Closes junit-team#45

raffig · 2019-08-22T21:35:21Z

I have one doubt: Currently @BeforeParam is called BEFORE parameters are known (before test instance constructor) thus annotated metod cannot do nothing that depends on parameter value. Of course one can do this in constructor, but I'm not sure if that is by the book. @AfterParam seems to work as expected.

raffig · 2019-08-23T08:09:28Z

And the second doubt: @AfterParam method must be static. If this method is used to cleanup some resources passed as parameters (e.g. test container) it causes the resources must be kept in static field. Won't this break anything like e.g. parallel test execution?

Could you please take a closer look at this:
testcontainers/testcontainers-java#1774

panchenko · 2019-08-23T08:23:50Z

@raffig both must be public static and optionally can take parameters (however that's not clear from the documentation).

raffig · 2019-08-23T09:22:46Z

@panchenko Thanks! Thats a great hint with parameters. I will fix my workaround and paste it in testcontainers thread. However I think maybe @ClassRule/TestRule could be fixed as well, so TestContainer could implement properly the container lifecycle in parametrized test?

panchenko · 2019-08-23T09:54:15Z

@raffig @ClassRule works as expected - for the whole class

raffig · 2019-08-23T10:05:45Z

@panchenko Ah, ok, so if ClassRule is for whole class, so ParamRule is needed right? Do you think it will get introduced in 4.13?

Thanks to your advice I think I've created suitable solution for TestContainers: testcontainers/testcontainers-java#1774 (comment)

Probably the same could be implemented with something like ParamRule.

panchenko · 2019-08-23T10:19:44Z

@raffig It feels like quite a rare case when a TestRule instance is used as a test parameter.

raffig · 2019-08-23T10:31:52Z

@panchenko I didn't think about TestRule as parameter. Rather about something that would allow parameters lifecycle management more out-of-the box as in case of TestContainers. However the longer I'm thinking about this the closer I get to the conclusion that is not possible to do in any useful generic way. As for now I think the @BeforeParam/@AfterParam is the best option to solve this.

Everything started when I found out that ClassRule provided by TestContainers does not work in case of Parameterized tests. That's why I've been searching for solution in the beginning, which I found using new @BeforeParam/@AfterParam. Now I'm wondering if TestContainers could provide something like ClassRule, out-of-the-box, but working with Parameterized tests.

Closes junit-team#45

@BeforeParam/@AfterParam for Parameterized runner

066e1d6

panchenko force-pushed the issue45 branch from 2c30e6d to 066e1d6 Compare April 2, 2017 12:57

Fix minor style issues

45064f7

kcooney force-pushed the issue45 branch from 54a02d0 to 45064f7 Compare April 16, 2017 08:27

kcooney requested changes Apr 16, 2017

View reviewed changes

panchenko added 2 commits April 18, 2017 15:18

test for multiple @BeforeParam/@AfterParam

3390335

test for @BeforeParam/@AfterParam with multiple parameters

1de2311

panchenko added 2 commits April 18, 2017 16:50

Validate number of parameters in @BeforeParam/@AfterParam

15ac6ad

code style

70245b3

kcooney requested changes Apr 18, 2017

View reviewed changes

panchenko added 2 commits April 19, 2017 09:24

iterate parameters once

e270f2c

style changes

570ec30

panchenko commented Apr 19, 2017

View reviewed changes

kcooney added 3 commits April 19, 2017 15:42

Style fixes

754732b

Make RunnersFactory.parameterCount final

35bbd8f

Add test cases for @parameters methods returning Collection

301a6e0

and one-shot Iterables.

kcooney approved these changes Apr 20, 2017

View reviewed changes

panchenko commented Apr 20, 2017

View reviewed changes

marcphilipp requested changes Apr 20, 2017

View reviewed changes

panchenko added 3 commits April 20, 2017 11:12

reuse RunBefores/RunAfters

98e061a

style

f406970

javadoc for @BeforeParam/@AfterParam

f7e3ad3

marcphilipp requested changes Apr 20, 2017

View reviewed changes

panchenko added 3 commits April 20, 2017 16:13

update javadoc

76c1779

update header in javadoc

03e598f

javadoc: "parameters"

4958ea8

kcooney requested changes Apr 20, 2017

View reviewed changes

since 4.13

ab15e7e

marcphilipp approved these changes Apr 20, 2017

View reviewed changes

kcooney approved these changes Apr 20, 2017

View reviewed changes

kcooney merged commit 1bf8438 into junit-team:master Apr 21, 2017

panchenko deleted the issue45 branch April 21, 2017 07:39

raju-saravanan mentioned this pull request Aug 7, 2017

ISSUE:195 Provide SSL support to SchemaRegistryClient hortonworks/registry#221

Merged

dstaraster mentioned this pull request Sep 18, 2017

Add Before and After Parameterized Run annotations #45

Closed

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

@BeforeParam/@AfterParam for Parameterized runner (junit-team#1435)

724ddaf

Closes junit-team#45

jihoonson mentioned this pull request Aug 10, 2018

Further optimize memory for Travis jobs apache/druid#6150

Merged

raffig mentioned this pull request Aug 22, 2019

Container not starting with @ClassRule testcontainers/testcontainers-java#1774

Closed

ndimiduk mentioned this pull request Jan 8, 2020

HBASE-23305: Master based registry implementation apache/hbase#954

Merged

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

@BeforeParam/@AfterParam for Parameterized runner (junit-team#1435)

b7e6d12

Closes junit-team#45

@BeforeParam/@AfterParam for Parameterized runner #1435

@BeforeParam/@AfterParam for Parameterized runner #1435

Conversation

panchenko commented Apr 1, 2017

kcooney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panchenko commented Apr 18, 2017

kcooney commented Apr 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcooney left a 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

marcphilipp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcphilipp left a 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

marcphilipp left a comment

Choose a reason for hiding this comment

kcooney commented Apr 21, 2017

panchenko commented Apr 21, 2017

raffig commented Aug 22, 2019

raffig commented Aug 23, 2019

panchenko commented Aug 23, 2019

raffig commented Aug 23, 2019

panchenko commented Aug 23, 2019

raffig commented Aug 23, 2019

panchenko commented Aug 23, 2019

raffig commented Aug 23, 2019