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

Document parameter resolution conflicts #3829

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jabhatfield
Copy link

Issue: #1190

Overview

User guide updated to explain ParameterResolver conflicts and provide some ways to resolve them.


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

@marcphilipp marcphilipp linked an issue Jun 20, 2024 that may be closed by this pull request
2 tasks
@jabhatfield
Copy link
Author

Please could I get some feedback for this PR?

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.

Sorry for the delay! The new section does a good job at explaining why/when conflicts occur. However, it does not document the best practices mentioned in the issue description. For example, using a custom annotation and/or a custom parameter type. Could you please add that?

==== Parameter Conflicts

If multiple implementations of `ParameterResolver` that support the same type are
declared within a test class, a `ParameterResolutionException` will be thrown, with a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
declared within a test class, a `ParameterResolutionException` will be thrown, with a
registered for a test, a `ParameterResolutionException` will be thrown, with a

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wording updated


If multiple implementations of `ParameterResolver` that support the same type are
declared within a test class, a `ParameterResolutionException` will be thrown, with a
message to indicate that competing types have been discovered. See the following example:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
message to indicate that competing types have been discovered. See the following example:
message to indicate that competing resolvers have been discovered. See the following example:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wording updated

Comment on lines 36 to 37
@RegisterExtension
final FirstIntegerResolver firstIntegerResolver = new FirstIntegerResolver();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using @ExtendWith would be more idiomatic here and below because there's no state or configuration that would justify programmatic extension registration.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to @ExtendWith here and also in the other code examples.

@jabhatfield
Copy link
Author

Sorry for the delay! The new section does a good job at explaining why/when conflicts occur. However, it does not document the best practices mentioned in the issue description. For example, using a custom annotation and/or a custom parameter type. Could you please add that?

Thanks for the feedback! I've added examples of using a custom type and a custom annotation.

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.

Document best practices for implementing ParameterResolvers
2 participants