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

fix #944: provide the resolved parameters in the ExtensionContext #3929

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

t1
Copy link
Contributor

@t1 t1 commented Aug 17, 2024

Overview

As (almost) suggested in #944, I've added a method ExtensionContext#getResolvedParameters (and getRequiredResolvedParameters) and an interface ResolvedParameters. The AbstractExtensionContext can store and return those. The ParameterResolutionUtils store them, but I had to introduce another but internal interface ResolvedParametersStore to make this possible without hurting the visibility scopes between descriptor and execution.

The problem I ran into is that TestMethodTestDescriptor#execute calls the parameter resolvers after all BeforeEach 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

Copy link
Member

@marcphilipp marcphilipp left a 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);
Copy link
Member

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);
Copy link
Member

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?

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

Successfully merging this pull request may close these issues.

Provide access to arguments of parameterized tests in lifecycle callback methods
2 participants