Junit5: Discussion: constructor parameter resolution, scope, etc.

[arend-von-reinersdorff]

Thanks for adding TempDirectory to JUnit 5.4.0-M1. This will make migration from JUnit 4 easier in my projects :-)

One point looks wrong to me though:
A @TempDir constructor parameter is treated like a parameter of a @BeforeAll method, ie its value is shared between all tests. See Javadoc of TempDirectory:

The temporary directory will be shared by all tests in a class when the annotation is present on a parameter of a @BeforeAll method or the test class constructor. Otherwise, e.g. when only used on test or @BeforeEach or @AfterEach methods, each test will use its own temporary directory.

I would expect a @TempDir constructor parameter to be treated like parameter of a @BeforeEach method, ie a new value for each test.

I'm not sure about the finer points of parameter resolving in JUnit, so maybe the current behavior is as intended. Reasons why I think a @TempDir constructor parameter should have a new value for each test:

1. According to the User Guide "JUnit creates a new instance of each test class before executing each test method"
   New instance -> new constructor call -> new value for constructor parameter (my expectation)
2. @BeforeAll = by default a static method = global state
   @BeforeEach = instance method = single test state
   constructor = instance bound = single test state
3. I normally want a clean temp directory for each test. Sometimes a shared temp directory might be needed, but it's the exception.
4. Isn't @TestInstance(Lifecycle.PER_CLASS) the normal way to share constructor state between all tests?

By the way, the Javadoc is really thorough. Very helpful!

[sbrannen]

Thanks for raising the issue.

We'll see if we can clarify the behavior.

By the way, the Javadoc is really thorough. Very helpful!

Thanks!

And... I just improved it _slightly_ in 8f565705af92e596d1b74a1f224bf3aa7c1f7991.

[sbrannen]

Tentatively slated for 5.4 RC1 for clarification.

[sbrannen]

FYI: this issue effectively questions the validity of the following test -- which verifies the current behavior.

https://github.com/junit-team/junit5/blob/269301e820e2eccbb9a19b6db2789717f2695082/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/support/io/TempDirectoryTests.java#L80-L84

[sbrannen]

I'm not sure about the finer points of parameter resolving in JUnit, so maybe the current behavior is as intended.

The current behavior is in fact to be expected. Let me explain...

When a constructor is invoked, the ExtensionContext supplied to ParameterResolver extensions for that constructor does not have access to the test method that will be subsequently invoked. Thus, the TempDirectory extension does not have a means to create a temporary directory that is specific to the about-to-be-invoked test method.

So, that clarifies the status quo, and I do not foresee that changing.

However, the JUnit team is currently discussing alternative approaches to addressing the issue you have raised.

[sbrannen]

@marcphilipp suggested we give field injection a try, so I put together a proof of concept that everyone can take a look at here:

https://github.com/junit-team/junit5/compare/issues/1731-temp-dir-field-injection

@junit-team/junit-lambda, thoughts?

[sbrannen]

_in progress_

[sbrannen]

Update:

The current proposal in my feature branch introduces a new TempDirContext that replaces direct usage of ParameterContext by a custom ParentDirProvider.

The TempDirContext provides access to the Field or the ParameterContext as appropriate.

[sbrannen]

@arend-von-reinersdorff, I have renamed this issue to reflect the direction we are currently considering taking to resolve the issue you described.

Does the proposed field injection support meet your needs?

[arend-von-reinersdorff]

@sbrannen thanks for taking the time to answer my question :-)

The current behavior is in fact to be expected. Let me explain...

When a constructor is invoked, the ExtensionContext supplied to ParameterResolver extensions for that constructor does not have access to the test method that will be subsequently invoked. Thus, the TempDirectory extension does not have a means to create a temporary directory that is specific to the about-to-be-invoked test method.

So, that clarifies the status quo, and I do not foresee that changing.

However, the JUnit team is currently discussing alternative approaches to addressing the issue you have raised.

OK. So this behavior is not specific to @TempDirectory but the standard behavior of all ParameterResolvers. I still find it confusing for the reasons noted above.

Ideas
Maybe the current behavior could be changed by eg:

• Give the ParameterResolver for constructors access to the test method, same as the parameter resolver for @BeforeEach. (Only for Lifecycle.PER_METHOD, not possible for Lifecycle.PER_CLASS)
• Extend the ExtensionContext hierarchy : AllContext > ConstructorContext > TestMethodContext. Constructor context wouldn't need to have access to the test method, but would still be created fresh for each test method for Lifecycle.PER_METHOD.

Again, I don't know JUnit 5 extensions well, so it's probably not that easy...

Documentation
As a compromise, how about extending the documentation for ExtensionContext:

• How does the context hierarchy look? AllContext > TestMethodContext
• Parameters for which methods (@BeforeAll, constructor, @BeforeEach, @Test, etc.) belong to which context?

Field injection
Regarding field injection: This is an interesting proposal. But I see it as unrelated to this ticket.
My problem is that I find the resolution of constructor parameters confusing. Field injection doesn't help with this. And to get parameter resolution from test method context, I can already use @BeforeEach methods.

[sbrannen]

Hi @arend-von-reinersdorff,

Thanks for the feedback. We will take that into account.

Field injection
Regarding field injection: This is an interesting proposal. But I see it as unrelated to this ticket.
My problem is that I find the resolution of constructor parameters confusing. Field injection doesn't help with this. And to get parameter resolution from test method context, I can already use @BeforeEach methods.

Well, I suppose it's a matter or perspective.

If you use field injection to obtain the temporary directory (File or Path), you can then access that from @BeforeEach, @Test, and @AfterEach methods without having to duplicate @TempDir Path tempDir in each method.

So, I think field injection is superior to duplicating a parameter declaration.

Having said that, however, I (and the rest of the JUnit team) do see your point about the confusion around parameter resolution and constructor injunction.

In light of that, I will open a new issue to address field injection for @TempDir, and we will continue to use _this_ issue to discuss the points you have raised.

Cheers,

Sam

[sbrannen]

"Support field injection in TempDirectory" has been moved to #1738.

[sbrannen]

This issue has been renamed and moved to the 5.5 backlog.

[wilkinsona]

I just found this issue while trying to learn why @TempDir isn't permitted on a test class's constructor. FWIW, I share @arend-von-reinersdorff's confusion around constructor parameter injection and would welcome them being test method scoped in the future.