Add BlockJUnit4ClassRunner#createTest(FrameworkMethod)

[petergeneric]

Adding BlockJUnit4ClassRunner#createTest(FrameworkMethod) to allow implementations to provide a custom instantiation of the test class for each FrameworkMethod invocation - see issue #1036

[kcooney]

Thanks for the pull. Can you think of a test that would cover this change?

[petergeneric]

I've updated the pull request with two tests:

1. To confirm createTest(FrameworkMethod) works - it passes the FrameworkMethod back in to the test class and the test methods check that the stored FrameworkMethod's underlying method name is what they expect
2. To confirm createTest() is still called when createTest(FrameworkMethod) is not overridden - it sets a boolean flag to indicate that the instance was created by a createTest() call

Neither seem particularly neat, but hopefully they're suitable

[petergeneric]

Travis marked this pull as failing on oraclejdk7 under Linux, it looks from the build log that it was a problem pulling down the dependencies (perhaps due to increased load after the DNS outage that impacted Travis?), but I forced a re-build anyway by amending the commit to get a different git rev id, Travis is now showing green.

[marcphilipp]

@petergeneric Thank you for the pull requests. I squashed your commits and merged your changes into master in bd5b90f.

[marcphilipp]

@petergeneric Can you please add a short description to https://github.com/junit-team/junit/wiki/4.13-Release-Notes?

[petergeneric]

Thanks, I've added a section in the release notes.

[marcphilipp]

Perfect, thanks again!

[Tibor17]

Maybe you had some reason in Guice and maybe this could be solved other way,
But I have a problem with the code itself. You have introduced a method with parameter which is simply ignored. The caller of this method would expect some mapping between param method<>test_instance but the method createTest(FM) does not do that, And bad things start happening so that the developer starts analyzing JUnit code. It's always unwanted to let other to analyze the code as soon as he finds out that the method signature does other things than supposed.

[kcooney]

@Tibor17 I don't understand the concern. Peter had a perfectly reasonable need to have the FrameworkMethod passed in. The change doesn't make the code harder to maintain because we need it for other reasons. Peter documented the methods clearly. This class is full of methods that are only exist and/or only are visible to enable extension points.

The alternative proposed by Peter would lead to a lot of duplicate code on his side. Did you have another alternative?

[Tibor17]

I know it's not that easy to find cool hook within few minutes, but take a look at these two signatures.

Many developers don't read documentations because they rely on proper API design. I think that's still correct attitude of developers due to the code should be self explaining just as you read the names of methods or classes. No need to read any additional text in documentation saying that this method defaults to another one.

And what happened.
Now we have two methods with different signatures doing exactly the same things:
Object createTest(FrameworkMethod method) throws Exception
Object createTest() throws Exception

[kcooney]

@Tibor17 I don't have a lot of sympathy for developers who don't read documentation, but I do like self-describing APIs. So your concern is that this method should have a different name? If so, can you suggest a better name?

If giving the method a different name wouldn't resolve your concern, then could you give us a specific suggestion?

Incidentally, a completely different solution would be something like TestNG's Factory annotation (possibly as a meta-annotation) so Peter could add this functionality without requiring his users to use a specific runner.

[marcphilipp]

We could deprecate createTest() and remove it in the next major release (>= 6.0).

[Tibor17]

Of course deprecating the method is one solution.
Finally, you got it guys!
You are committers and you are responsible for code clarity.
Sometimes the PRs take months to commit and here "a perfect solution" only 6 days.

[kcooney]

Removing the method is likely to break many people. Is it really worth the pain? I don't see a large benefit to removing the method.

[marcphilipp]

@Tibor17 Do you want us to go back to month-long pull requests? 😏

[kcooney]

We have informally decided that we would not remove a deprecated API until two years after the release where it was deprecated, and we would only remove the APIs in a major version.