-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix #944: provide the resolved parameters in the ExtensionContext #3929
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I'll bring this up with the team to discuss the high-level design before we get too deep.
I'm not sure, if this ordering could be inverted: first the parameter resolvers, then the
BeforeEach
handling.
Yes, I think we'd have to explicitly resolve the test method's parameters (and only those!) before calling any method-level extensions or lifecycle callbacks.
@@ -135,6 +135,7 @@ private static Object resolveParameter(ParameterContext parameterContext, Execut | |||
ParameterResolver resolver = matchingResolvers.get(0); | |||
Object value = resolver.resolveParameter(parameterContext, extensionContext); | |||
validateResolvedType(parameterContext.getParameter(), value, executable, resolver); | |||
recordResolvedParameter(extensionContext, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also record parameters for methods invoked via ExtensionContext.getExecutableInvoker()
, right? I think we need to ensure that we only record them for the test method that's being executed, e.g. by checking executable
equals extensionContext.getTestMethod()
.
@@ -135,6 +135,7 @@ private static Object resolveParameter(ParameterContext parameterContext, Execut | |||
ParameterResolver resolver = matchingResolvers.get(0); | |||
Object value = resolver.resolveParameter(parameterContext, extensionContext); | |||
validateResolvedType(parameterContext.getParameter(), value, executable, resolver); | |||
recordResolvedParameter(extensionContext, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a benefit of recording these one-by-one rather than in a single go?
Overview
As (almost) suggested in #944, I've added a method
ExtensionContext#getResolvedParameters
(andgetRequiredResolvedParameters
) and an interfaceResolvedParameters
. TheAbstractExtensionContext
can store and return those. TheParameterResolutionUtils
store them, but I had to introduce another but internal interfaceResolvedParametersStore
to make this possible without hurting the visibility scopes betweendescriptor
andexecution
.The problem I ran into is that
TestMethodTestDescriptor#execute
calls the parameter resolvers after allBeforeEach
callbacks and methods. So they can't see them, but that was the purpose of it all.I'm not sure, if this ordering could be inverted: first the parameter resolvers, then the
BeforeEach
handling.I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@API
annotations