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.

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

Introduce extension point before test instantiation #748

Conversation

codecholeric
Copy link
Contributor

To migrate our existing framework to JUnit 5 I'd like to hook into the lifecycle of JUnit 5 test execution before any test instance is created.

We want to configure the test environment for each test right before the test instance is created.

  • TestInstancePostProcessor is too late, as instance fields have already been initialized.
  • BeforeAll is not appropriate, as we need to configure the environment for each test run.

I hereby agree to the terms of the JUnit Contributor License Agreement.

Overview

Please describe your changes here and list any open questions you might have.


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

To migrate our existing framework to JUnit 5 I'd like to hook into the lifecycle of JUnit 5 test execution before any test instance is created.

We want to configure the test environment for each test right before the test instance is created.

 * TestInstancePostProcessor is too late, as instance fields have already been initialized.
 * BeforeAll is not appropriate, as we need to configure the environment for each test run.

 ---
 I hereby agree to the terms of the JUnit Contributor License Agreement.
@sbrannen
Copy link
Member

TestInstancePostProcessor is too late, as instance fields have already been initialized.

That's not necessarily true. The primary use case for a TestInstancePostProcessor is in fact to set fields (i.e., dependency injection).

TestInstancePostProcessor is too late

Can you please expound on what you mean by that statement?

It's too late to achieve what?

@sbrannen sbrannen changed the title Add new extension point right before test instance instantiation Introduce extension point before test instantiation Mar 25, 2017
@codecholeric
Copy link
Contributor Author

codecholeric commented Mar 28, 2017

Short story:

For our test infrastructure to create test data, we must distinguish calls during the static initialization of each test class from calls during test instantiation for each test method.
This could be solved by the proposed BeforeTestInstantiationCallback.

Long story:

When we write our tests, we want to restrict the data that have to be set explicitly to those that the specific test requires. All other properties are set randomly, but valid. Naturally using random data may introduce randomly failing tests, so we need the means to easily reproduce test failures that e.g. happen in the CI environment.
Reproducing such failures requires knowing the seed values for our random data when the specific test was executed. This is where we want to take advantage of a JUnit 5 extension. It has to store one seed value at the class level, for example if static constants are initialized using our random mechanism. In addition, it has to store the seed value at the instance level, for each test run. If a test should fail, the respective seed values must be reported in the failure message. If the failure is related to the use of random data, all this together allows us to reproduce it in any other environment. Example:

private static final Address staticAddress = 
    createRandomAddress(); // determined by the class seed
 
private Address instanceAddress = 
    createRandomAddressBuilder() // determined by the instance seed
        .withStreet("some street")
        .build();
 
@Test
void someTest() {
    Address anotherAddress = 
        createRandomAddress(); // also determined by the instance seed
    // ...
}

We note the class seed in a BeforeAllCallback, which is the perfect place. The switch to using the instance seed however, has to be right before the test instance is created, to cover random data used for instance field initialization, which is a rather common use case in our tests.
Obviously, the TestInstancePostProcessor callback is too late for switching from the class seed to the instance seed. Currently we have an ugly workaround, where we check the stacktrace to switch to the instance seed, as soon as we detect a call from the test's constructor. If there are no such calls we switch to the instance seed in the TestInstancePostProcessor callback.
The proposed BeforeTestInstantiationCallback would be the natural place to make the required switch to the instance seed trivial.
Do you think, adding the BeforeTestInstantiationCallback would be harmful?

@marcphilipp
Copy link
Member

IIRC there has also been some discussion about providing an extension point for test class instantiation, i.e. it would return the test class instance, but I cannot remember the issue at the moment. That would be more powerful and I think we'd need only one.

@smoyer64 @junit-team/junit-lambda Do you remember the issue?

@marcphilipp
Copy link
Member

I believe the issue I was looking for is #672.

@jonashoef
Copy link
Contributor

I fully agree. An extension point for test instance creation would certainly solve our problem and provide more flexibility than the one we proposed.

@smoyer64
Copy link
Contributor

@marcphilipp - Yes, I think #672 (a Weld use-case) is the closest match. In that issue's discussion, you reference #203 (now closed) and following the chain of issues, @jlink references #201 (a PowerMock use-case).

Looking back at that issue, the @sbrannen quote:

With that in mind, I would propose a new extension -- call it InstanceFactory or similar -- that can only be registered once per test container. Any attempt for a second extension to register itself as such a factory would then result in an exception.

should probably be clarified. Does "test container" refer to the top-level test container or does it refer to (at a minimum) the test's immediate parent container? I can certainly imagine scenarios where you might use different a InstanceFactory for different tests. It definitely makes sense that for each test, only one InstanceFactory can provide the actual instance. While my original question to the Weld team (performing CDI bootstrap and initialization for a provided instance) couldn't be done, I think the idea of an InstanceDecorator might also provide value ... since I have no use-cases to provide for this concept, I won't propose it (at least not yet).

In general, the JUnit 4 rules that I've written have predominately been doing some form of decorating/initializing but many of those cases can be handled by TestInstancePostProcessor extensions (and perhaps all).

Thanks for keeping this "on the radar", I'd love to be able to use Weld in JUnit 5 tests!

@jonashoef
Copy link
Contributor

I fully understand that the proposed InstanceFactory extension may exists only once per test (container).
So, to that respect the additional flexibility does impose restrictions that our original proposal doesn't: Several extensions implementing the BeforeTestInstantiationCallback may be freely combined, but only a single extension per test (container) may implement the InstanceFactory. I'm afraid this restriction is inherent, or do you have an idea how it could be avoided?

@nipafx
Copy link
Contributor

nipafx commented May 3, 2017

I typed a longer answer with different follow-up questions and then realized that this is a PR. I don't know how the JUnit team stands on this but I would prefer discussing the general feasibility of such an extension point on an issue and use the PR to discuss the concrete code change.

@codecholeric Could you open a new issue with the feature request and the short and long story as background?

@codecholeric
Copy link
Contributor Author

codecholeric commented May 5, 2017

@nicolaiparlog Done -> #839

@marcphilipp
Copy link
Member

I think this use case should be solvable using TestInstanceFactory introduced in 5.3. Feel free to comment if you feel that this one should not have been closed.

@codecholeric
Copy link
Contributor Author

@marcphilipp I finally had a possibility to check it out. TestInstanceFactory does solve our problem in a certain context, but it still is not as clean as it should be.
As @jonashoef mentioned, we really only care about a hook before the test instance is created. Yes, we can implement TestInstanceFactory, execute the hook and return testClass.newInstance(). But due to the nature of TestInstanceFactory, this can now not be combined anymore with any other extension that wants to create the test instance, even though for our use case we would not need to control how the test instance is created at all.
Nevertheless I see your point if our project is the only project in the world that could use such a hook 😉

@marcphilipp
Copy link
Member

@codecholeric Thanks for checking! Feel free to open a new issue should this lack of composability become a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants