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

TestInstancePostProcessor cannot be registered via @ExtendWith on a non-static field #3437

Closed
1 task
sbrannen opened this issue Aug 27, 2023 · 6 comments · Fixed by #3895
Closed
1 task

Comments

@sbrannen
Copy link
Member

sbrannen commented Aug 27, 2023

Overview

As discovered by @graydavid in #3433, there is an issue regarding extension registration via @ExtendWith on instance fields.

The following is a simplification of the RandomNumberDemo from the User Guide which demonstrates the problem.

// @ExtendWith(RandomNumberExtension.class)
class RandomNumberDemoTest {

	@Random
	Integer randomNumber;

	@Test
	void test() {
		assertThat(this.randomNumber).isNotNull();
	}
}

The above test fails; however, if you annotate the test class with @ExtendWith(RandomNumberExtension.class) the test then passes.

The reason is that ClassBasedTestDescriptor#instantiateAndPostProcessTestInstance() invokes ExtensionUtils.registerExtensionsFromFields() too late.

I even added the following comment when I introduced that support.

// In addition, we register extensions from instance fields here since the
// best time to do that is immediately following test class instantiation
// and post processing.

In retrospect, that is not the "best time to do that" when a field is annotated (or meta-annotated in the case of @Random) with an @ExtendWith annotation that registers an extension that implements TestInstancePostProcessor.

Related Issues

Deliverables

  • Revise the usage of ExtensionUtils.registerExtensionsFromFields() so that a TestInstancePostProcessor can be registered via @ExtendWith on a non-static field.
@Michael1993
Copy link

Michael1993 commented Sep 15, 2023

Should I be able to inject a @RegisterExtension value with a field-level TestInstancePostProcessor?

Example:

class BasicTests {

  @ExtendWith(ExtensionInjector.class)
  ExtensionInjector injector;

  @RegisterExtension
  ParameterResolver extension;

  // some tests...
}

Also: can I work on this issue? Other than this one question, I think I can handle it.

Michael1993 added a commit to Michael1993/junit5 that referenced this issue Sep 15, 2023
sbrannen added a commit that referenced this issue Sep 16, 2023
With this commit, the RandomNumberExtension example in the User Guide
now properly supports non-static field injection.

See #3437
Closes #3433
sbrannen added a commit that referenced this issue Sep 16, 2023
With this commit, the RandomNumberExtension example in the User Guide
now properly supports non-static field injection.

See #3437
Closes #3433
@sbrannen
Copy link
Member Author

Hi @Michael1993,

Should I be able to inject a @RegisterExtension value with a field-level TestInstancePostProcessor?

Ideally, yes, I believe so. At least, generally speaking a TestInstancePostProcessor should be capable of doing that.

I believe I demonstrated that once with Spring. Let me see if I can find it...

@sbrannen
Copy link
Member Author

I believe I demonstrated that once with Spring. Let me see if I can find it...

https://stackoverflow.com/a/50251190/388980

@sbrannen
Copy link
Member Author

Also: can I work on this issue? Other than this one question, I think I can handle it.

Thanks for volunteering!

However, this issue is assigned to me and not labeled as up-for-grabs.

In addition, it may turn out to be more challenging than you expect.

In light of that, I would like to keep this assigned to me.

FWIW, I took a glance at your spike, and I'd like to point out that we have no plans to add specific handling for a particular extension API (such as TestInstancePostProcessor). Rather, I will implement a general solution that applies to any extension type registered via @ExtendWith on both static and non-static fields, and I've already begun work on this issue.

@Michael1993
Copy link

Sorry, I don't know how I missed that. I will take a look at the up-for-grabs issues instead, then. 🙂

@sbrannen sbrannen modified the milestones: 5.11 M1, 5.11 M2 Apr 8, 2024
@marcphilipp marcphilipp modified the milestones: 5.11 M2, 5.11 M3 May 17, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Jul 19, 2024
This commit is truly just an "experiment". The actual "fix" might take
a completely different approach.

Various tests in ExtensionRegistrationViaParametersAndFieldsTests fail
with the changes in this commit.

Issue: junit-team#3437
@sbrannen
Copy link
Member Author

I pushed an "experiment" to the following feature branch.

Note, however, that this is truly a work in progress. A few existing tests fail, and we may need to take a completely different approach.

Thus, I've only pushed this to allow others (for example, @marcphilipp) to take a look at this for potential inspiration.

main...sbrannen:junit5:issues/3437-ExtendWith-on-fields-early-registration

marcphilipp added a commit that referenced this issue Jul 30, 2024
This PR changes extensions declared via @ExtendWith on non-static fields
to be registered before the test instance is instantiated. Since these 
declarative extensions are documented to be sorted along with 
programmatic extensions registered via @RegisterExtension the latter 
are also registered earlier. Since, however, non-static fields are not 
initialized until the test class is instantiated, this PR introduces the
internal concept of "uninitialized extensions" that are registered 
early and initialized whenever an instance of the test class is created.

Fixes #3437. Resolves #3463.

Co-authored-by: Sam Brannen <104798+sbrannen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants