Apply suite configuration to tests selected by unique ID

[mpkorstanje]

Overview

Applies suite configuration to tests selected by unique ID.

Tests in a suite selected by id did not receive their configuration because this was only applied to the discovery request builder when a suite class was selected. By splitting the processing of configuration annotations and select and filter annotations, the configuration can now always be applied prior to discovery.

Fixes: #3693

---

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

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[sbrannen]

Thanks for the super fast PR! 👍

I think it looks pretty solid, but I've requested a few minor enhancements.

[sbrannen]

I just realized we don't have any Javadoc for SuiteLauncherDiscoveryRequestBuilder. 😱

So we should definitely address that, but rather in a different issue.

In any case, I'd like to see some Javadoc on this new method.

In addition, let's come up with a more descriptive name, perhaps something like one of the following.

• configurationParametersForSuite(Class<?>)
• configurationParametersForSuiteClass(Class<?>)
• configurationParametersFromSuite(Class<?>)
• applyConfigurationParametersFromSuite(Class<?>)

I know the last one is a bit long and doesn't start with configurationParameter like the existing methods; however, I do think it aptly describes what it does (which is a bit special compared to the existing configurationParameter() and configurationParameters() methods).

Thoughts?

[mpkorstanje]

I skipped the Javadoc because most classes marked as internal were lacking it. I'll add some for new methods.

Namingwise, I am considering deprecating the existing suite(Class<?>) method and replacing it with two methods:

• applyConfigurationParametersFromSuite(Class<?>)
• applySelectorsAndFiltersFromSuite(Class<?>)

And then also look at a way to avoid invoking applyConfigurationParametersFromSuite() over and over again for every unique ID.

In hindsight these are two rather distinct operations. It just didn't occur to me when I wrote the suite engine.

[sbrannen]

I skipped the Javadoc because most classes marked as internal were lacking it. I'll add some for new methods.

Indeed, it is marked as internal even though it's public. I had overlooked that.

In any case, I think it's still worth documenting (even if it's "just for us").

I raised #3695 to address that.

Speaking of which, would you like to address #3695?

I'm asking because someone else has already volunteered to do that, and I want to avoid duplicate work.

[sbrannen]

Namingwise, I am considering deprecating the existing suite(Class<?>) method and replacing it with two methods:

* `applyConfigurationParametersFromSuite(Class<?>)`

* `applySelectorsAndFiltersFromSuite(Class<?>)`

I like that. 👍

I'm also wondering if we should add a precondition check which requires that the supplied Class is annotated with @Suite.

Or... perhaps that's over-engineering. 🤷

And then also look at a way to avoid invoking applyConfigurationParametersFromSuite() over and over again for every unique ID.

Yes, please do that.

In hindsight these are two rather distinct operations. It just didn't occur to me when I wrote the suite engine.

No worries. It's a learning process, and APIs are meant to evolve (in a non-breaking manner). 😉

[mpkorstanje]

I'm asking because someone else has already volunteered to do that, and I want to avoid duplicate work.

That's quick! This looks like a good issue for a first time contributor, so I'm happy to see someone else try.

[mpkorstanje]

I'm also wondering if we should add a precondition check which requires that the supplied Class is annotated with @suite.

Or... perhaps that's over-engineering. 🤷

That would be a breaking change. The @Suite annotation is optional for the JUnitPlatform runner.

[marcphilipp]

Looks great! 👍

[sbrannen]

Looks great! 👍

I second that.

Thanks again for raising the issue and for the well-executed PR. 👍

p.s. I polished a bit in 2a5c6b2.

[mpkorstanje]

Cheers! You're welcome. I think I've said before, it's nice to work on a codebase where everything is clear and well organized.