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

Add support for additional configuration parameters resources #3345

Merged

Conversation

robinjhector
Copy link
Contributor

@robinjhector robinjhector commented Jun 8, 2023

Overview

This commit adds support for specifying a properties file, for use with configuration parameters.

  • Added a new field configurationParametersResources of type List to store the configuration parameters resources.
  • Added new methods configurationParametersResource(String propertiesFile) and configurationParametersResources(List propertiesFiles) to add configuration parameters resources to the request builder.
  • Updated the buildLauncherConfigurationParameters() method to include the configurationParametersResources in the Builder instance.

Related to issue: #3340

Open questions:

  • The @API annotation values, what should they be? Are they experimental or set to STABLE at once? Which version nr?

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


Definition of Done

@robinjhector
Copy link
Contributor Author

Question: I seem to have forgot about the picocli console runner. Should I add new picocli options for selecting a config resource file? (--config-resource perhaps?) in TestDiscoveryOptionsMixin.RuntimeConfigurationOptions

@sbrannen
Copy link
Member

sbrannen commented Jun 9, 2023

Question: I seem to have forgot about the picocli console runner. Should I add new picocli options for selecting a config resource file? (--config-resource perhaps?) in TestDiscoveryOptionsMixin.RuntimeConfigurationOptions

Yes, that sounds like a good idea.

Though maybe --config-resources which accepts a comma-separated list of resource paths.

Copy link
Member

@sbrannen sbrannen 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 submitting the PR.

I requested a few minor changes.

@robinjhector
Copy link
Contributor Author

robinjhector commented Jun 9, 2023

Yes, that sounds like a good idea.

Though maybe --config-resources which accepts a comma-separated list of resource paths.

Or just make --config-resource repeatable, like the existing --configcli option? I think I would prefer that, instead of CSVing it.

Also; I couldn't help but notice that the CLI configuration parameters have a strict validation of unique key-value pairs (#1308). And am a bit confused how configuration resources should behave with that in mind.

I would prefer a similar approach as the suite annotations, eg. the explicit parameters take precedence over any property file based ones. Any suggestions or does that sound good?

Edit: Nevermind, I don't think it applies to this anyway. The launcher request builder is used from here, so the logic will be the same regardless.
I've updated the PR with a commit for new CLI options 👍

@robinjhector robinjhector force-pushed the configuration-parameter-resources branch 2 times, most recently from 628a605 to 1d12ffc Compare June 9, 2023 09:31
@robinjhector

This comment was marked as outdated.

@sbrannen

This comment was marked as outdated.

Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

This is coming along nicely. 👍

I went ahead and reviewed the status quo and added a few comments.

Thank you for your efforts!

Copy link

If you would like us to be able to process this pull request, please provide the requested information or make the requested changes. If the information is not provided or the requested changes are not made within the next 3 weeks, we will be unable to proceed and this pull request will be closed.

Copy link

github-actions bot commented Jul 3, 2024

Closing due to lack of requested feedback. If you would like to proceed with your contribution, please provide the requested information or make the requested changes, and we will re-open this pull request.

@github-actions github-actions bot closed this Jul 3, 2024
@sbrannen
Copy link
Member

sbrannen commented Jul 3, 2024

Hi @robinjhector,

I realize quite a bit of time has passed since you originally worked on this, but do you think you'll find time to make the suggested changes and complete this PR?

If not, I imagine that someone in the team can pick it up from here.

Please let us know how you'd like to proceed.

Thanks

@sbrannen sbrannen reopened this Jul 3, 2024
This commit adds support for specifying a properties file, for use with configuration parameters.

- Added a new field configurationParametersResources of type List<String> to store the configuration parameters resources.
- Added new methods configurationParametersResource(String propertiesFile) and configurationParametersResources(List<String> propertiesFiles) to add configuration parameters resources to the request builder.
- Updated the buildLauncherConfigurationParameters() method to include the configurationParametersResources in the Builder instance.

Related to issue: junit-team#3340
Adjusted annotation parameter name to `value()`.
Scrapped duplicate configurationParametersResources method on discovery request builder. (Is now var-args)
@robinjhector robinjhector force-pushed the configuration-parameter-resources branch from 228d6cb to a219358 Compare July 3, 2024 10:57
@robinjhector robinjhector force-pushed the configuration-parameter-resources branch from a219358 to 07b0921 Compare July 3, 2024 11:09
@robinjhector
Copy link
Contributor Author

Hey @sbrannen ! Thanks for getting back to me, I have rebased and adjusted the code + documentation after your review. Sorry about the delay!

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.

Looking good! I'll take it from here.

@marcphilipp marcphilipp changed the title Add support for configuration parameters resources Add support for additional configuration parameters resources Jul 19, 2024
@marcphilipp marcphilipp merged commit be8ae70 into junit-team:main Jul 19, 2024
16 checks passed
@marcphilipp
Copy link
Member

@robinjhector Thank you for your contribution! 👍

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

Successfully merging this pull request may close these issues.

Introduce @ConfigurationParametersResource for use with @Suite classes
3 participants