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

@TestFactory methods should evaluate ContainerExecutionCondition extensions instead of TestExecutionCondition extensions #625

Closed
marcphilipp opened this issue Jan 7, 2017 · 4 comments

Comments

@marcphilipp
Copy link
Member

marcphilipp commented Jan 7, 2017

Currently, @TestFactory nodes provide a TestExtensionContext and evaluate TestExecutionConditions. Since they are containers they should rather provide a ContainerExtensionContext and evaluate ContainerExecutionConditions.

Implementation Note

After changing this, JupiterTestDescriptor can implement shouldBeSkipped like this:

@Override
public final SkipResult shouldBeSkipped(JupiterEngineExecutionContext context) throws Exception {
   return isContainer() ? shouldContainerBeSkipped(context) : shouldTestBeSkipped(context);
}
@sbrannen
Copy link
Member

sbrannen commented Jan 7, 2017

Good catch! 👍

@sbrannen
Copy link
Member

The problem with making such a change is that extensions for @TestFactory methods would no longer have access to the test instance (which is currently only available via the TestExtensionContext API), even though there is in fact an instance.

Consequently, such extensions would no longer have access to instance state (i.e., fields that may have been initialized or injected).

In other words, such a change would reduce the scope of what such extensions can do.

@junit-team/junit-lambda, let's discuss the pros and cons.

@marcphilipp
Copy link
Member Author

If we merged ContainerExtensionContext and TestExtensionContext (which we will probably have to do for #455 and #48 anyway), this would no longer be a problem. Since #455 is already in progress, I'm setting this issue to blocked.

@marcphilipp
Copy link
Member Author

This issue has become obsolete since [Container|Test]ExtensionContext have been merged into ExtensionContext in #901.

@sbrannen sbrannen removed this from the 5.0 M5 milestone Jul 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants