Provide TempDirFactory access to the source element (parameter or field)

[marschall]

I'm working on a TempDirFactory implementation based on my own memory file system implementation over at marschall/memoryfilesystem-junit-provider. One of the features I would like to offer is configuration, allowing the user more control over the kind of file system they get (Windows, Unix, macOS, …). I imagine this configuration to be done through additional annotations on the element annotated with @TempDir, something like this

@TempDir(factory = MemoryFileSystemTempDirFactory.class)
@MemoryTempDirType(WINDOWS)
Path tempDirectory;

(this can me made more compact with the help of meta-annotations)

For this to work, the TempDirFactory implementation needs access to this annotated element. Unfortunately TempDirFactory is only passed an ExtensionContext which only gives access to the test, not the element (method or field) annotated with @TempDir. What is needed is something like ParameterContext in ParameterResolver.

Deliverables

• extend TempDirFactory API to give access to annotated source element

[sbrannen]

That's an interesting proposal.

However, if we want to have support for this... we'd better do it as a "last minute" addition for 5.10 GA.

Otherwise, it will become more challenging to alter the TempDirFactory API in a backward-compatible way (even though the API will remain "experimental" for the time being).

@junit-team/junit-5, thoughts?

[scordio]

The corresponding AnnotatedElement is already not "too far" in the call hierarchy:

https://github.com/scordio/junit5/blob/34248ab949388e73e57834d1e9e3f796f3f2eda1/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java#L235-L246

Would that be enough?

@marschall in case the team wants to proceed in this direction, would you like to compose such changes by yourself? Otherwise, I am happy to help with it.

[sbrannen]

@scordio, thanks for volunteering.

I also came to the conclusion that we could propagate the sourceElement to the TempDirFactory, revising the API as follows.

Path createTempDirectory(ExtensionContext context, AnnotatedElement sourceElement) throws Exception;

(or switching the order of the ExtensionContext and AnnotatedElement parameters`)

However, it's not that straightforward.

For example, if @TempDir is used to annotate a constructor argument for a nested test class, annotation lookups will fail for any code compiled using Java 8 due a bug in javac. See the Javadoc for ParameterContext.findAnnotation(...) for details.

Thus, we would in fact have to come up with a solution similar to ParameterContext.

Furthermore, I'm not sure if this is a one-off feature.

Disclaimer: I'm not sure, simply because I have not yet put enough thought into it.

In any case, if we have to go to the extent of implementing and supporting something analogous to ParameterContext, it might be beneficial to figure out where else it could be useful to extension authors. Along the same line of thinking, it may then be beneficial to introduce access to this new "context" via the existing ExtensionContext (if feasible/possible).

Thus, deciding what should be done requires a bit of research into the available options.

@scordio, if you'd like to volunteer to research that, please do!

[marschall]

@scordio please feel free to go ahead, I'm not confident I have the time this deserves to put into it, @sbrannen made some valid points

A default method could provide some sort of limited backwards compatibility should we miss 5.10 GA.

[scordio]

Hi @sbrannen, sure, happy to research it further. Do you know roughly when 5.10 GA will be released?

[marcphilipp]

@scordio We were going to release it end of the week but we can postpone if need be

[marschall]

Please don´t postpone just for me. I consider this a nice use-case and can live with breaking changes, especially for an experimental API. In the mean time I can use other workarounds like different factory classes or abstract factory classes.

[scordio]

@sbrannen, thanks a lot for the pointers. Furthermore, the reading of #1345 helped me a lot in understanding the problem better.

In any case, if we have to go to the extent of implementing and supporting something analogous to ParameterContext, it might be beneficial to figure out where else it could be useful to extension authors. Along the same line of thinking, it may then be beneficial to introduce access to this new "context" via the existing ExtensionContext (if feasible/possible).

I tried to follow your idea but I'm afraid I'm not familiar enough with all the features of JUnit to identify a suitable solution in this direction.

For this reason, I'd like to try a slightly different approach and would be happy to hear your thoughts.

My approach would be the following:

• A new org.junit.jupiter.api.extension.AnnotatedElementContext interface would be defined, with methods:
  • isAnnotated
  • findAnnotation
  • findRepeatableAnnotations
• ParameterContext would extend AnnotatedElementContext
• The TempDirFactory API would become:

Path createTempDirectory(AnnotatedElementContext elementContext, ExtensionContext extensionContext) throws Exception;

• TempDirectory would define a nested FieldContext class that implements AnnotatedElementContext
• A new FieldContext instance would be created by TempDirectory::injectFields and passed down to the createTempDirectory invocation

I'm not quite ready with a draft PR but I should be able to submit something in a couple of days.

Two questions though:

• Would adding AnnotatedElementContext as a parent of ParameterContext be a potential breaking change? To the best of my knowledge, it shouldn't have any impact on binary compatibility, but I have limited experience with this.
• Would it be fine to declare AnnotatedElementContext as an experimental API, while ParameterContext is already stable?

[sbrannen]

@sbrannen, thanks a lot for the pointers.

You're welcome!

Furthermore, the reading of #1345 helped me a lot in understanding the problem better.

Yeah... that was a tricky one. 😱

Regarding AnnotatedElementContext, that sounds good to me. Though it should also have an AnnotatedElement getAnnotatedElement() method, analogous to ExtensionContext.getElement() (but not Optional).

I'm busy with other things at the moment, so I think @marcphilipp will be able to review your PR.