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

Remove separate artifact for parameterized tests #858

Closed
nipafx opened this issue May 22, 2017 · 8 comments
Closed

Remove separate artifact for parameterized tests #858

nipafx opened this issue May 22, 2017 · 8 comments

Comments

@nipafx
Copy link
Contributor

nipafx commented May 22, 2017

Parameterized tests require a dependency on junit-jupiter-params, which is surprising and will surely be seen as superfluous by many users. Doing some research I only found #728, which references a team call but gives no background. I assume there are good reasons for that decision (in which case this issue might force them into the light) but until then I will make a suggestion based on my ignorance. 😉

I thought the reason may be the dependency on the CSV parser and JUnit's goal to not ship with too many dependencies. If that is indeed the reason, I suggest splitting just those sources into a subproject junit-jupiter-csv-params (or whatever) and move the other types into junit-jupiter-api. That would keep JUnit's dependencies small while giving a lot of functionality out of the box. Having to include a separate artifact for CSV values (very extension-y) makes much more sense than including one for @ParameterizedTest (very core feature-y).

@smoyer64
Copy link
Contributor

@TestTemplate, TestTemplateInvocationContext and TestTemplateInvocationContextProvider are all in junit-jupiter-api and provide the interfaces I needed for upREST. I guess I'm viewing junit-jupiter-params as one implementation of @TestTemplate. To be fair, I've been using the JUnitParams runner for so long that I don't really notice it but I'm wondering which classes you would move to the API. From what I've seen (and I'm no expert) it would be pretty easy to cause cycles in those two modules.

@nipafx
Copy link
Contributor Author

nipafx commented May 23, 2017

Yes, junit-jupiter-params is just one implementation of test templates and maybe that's why it's in its own artifact (although what about @Disabled and execution conditions, then?). I assumed that was not the reason, though. If it is, I will reconsider and maybe make another argument.

If the rationale is indeed to keep out the dependency on Univocity CSV parsing, then my proposal is this:

  • move @ParameterizedTest from jupiter-params to jupiter-api
  • move packages org.junit.jupiter.params.converter, org.junit.jupiter.params.provider, and org.junit.jupiter.params.support from jupiter-params to jupiter-api
  • move remaining types in org.junit.jupiter.params from jupiter-params to jupiter-engine
  • this leaves exactly the classes prefixed with Csv in jupiter-params

This creates no cyclic dependencies. Some follow-up thoughts:

  • the dependency of @ParameterizedTest on ParameterizedTestExtension needs to be broken apart as was done for @Disabled and DisabledCondition
  • packages would likely have to be renamed
  • some of the packages moved to jupiter-api contain classes that could be considered implementation details; they could be moved into jupiter-engine but that requires a similar decoupling step as the one above; this would allow moving org.junit.jupiter.params.support into the engine as well

The decoupling mechanism currently chosen is implicit, manual, and not overly user-friendly - I wonder whether it should get more explicit support because it will be a recurring concern within JUnit Jupiter.

@smoyer64
Copy link
Contributor

I have to admit I like the idea of parameterized tests being "built in".

@marcphilipp marcphilipp added this to the 5.0 M6 milestone Jun 10, 2017
@sbrannen sbrannen changed the title Separate artifact for parameterized tests Remove separate artifact for parameterized tests Jul 11, 2017
@marcphilipp marcphilipp modified the milestones: 5.0 M6, 5.0 RC1 Jul 18, 2017
@marcphilipp marcphilipp modified the milestones: 5.0 RC1, 5.0 GA Jul 30, 2017
@bechte
Copy link
Contributor

bechte commented Sep 7, 2017

Just as a question: Wouldnt it be great if junit-jupiter-params was just a plugin that can be switched on when needed and is not included if not? 😃 I even was thinking of having it as a separate engine as we were starting, but it was just a thought...

@sbrannen
Copy link
Member

sbrannen commented Sep 7, 2017

When you say "plugin", do you perhaps mean "extension"?

If not, what are you saying? 😇

@marcphilipp
Copy link
Member

Well, as a separate artifact it can be added to a project's dependencies when needed. Moreover, the annotation already serves as a switch to enable it for a single method. So, I'd say we're good in this regard.

@bechte
Copy link
Contributor

bechte commented Sep 8, 2017

Wouldnt it be great if junit-jupiter-params was just a plugin that can be switched on when needed and is not included if not?

My question was more of a rethorical one... 😈

When you say "plugin", do you perhaps mean "extension"?

Yes, we call them "extension" in Jupiter. But I was more about speaking the overall idea/concept of having extensible units, that add functionality and features to the JUnit Platform (or to Jupiter specifically) and, therefore, calling it a "plugin", whereas it is not essentially necessary to make use of our extension mechanism in Jupiter (could also be to provide another TestEngine).

I have to admit I like the idea of parameterized tests being "built in".

I don't agree. I see a lot of value in this extensibility, allowing people to select that kind of functionality they want to have in their project. Especially for bigger features like this, we don't want to handle flags and options for turning on and off throughout the code base. The separation of the module is of much value.

So 👍 for Marcs comment and a vote to close this issue by leaving the modules "as-is". And we should try to stick with this route for the future, e.g. for introducing test scenarios, etc. 😃

@marcphilipp
Copy link
Member

Team decision: Keep parameterized tests in separate artifact for the time being.

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

No branches or pull requests

5 participants