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

Move junit libs to managed block. #936

Merged
merged 3 commits into from
Feb 20, 2024

Conversation

altro3
Copy link
Contributor

@altro3 altro3 commented Jan 27, 2024

Fixed #831

@altro3
Copy link
Contributor Author

altro3 commented Jan 27, 2024

@melix @graemerocher Something is wrong with test resources for R2DBC_MYSQL. All builds failed with the same error:
изображение

melix added a commit to micronaut-projects/micronaut-test-resources that referenced this pull request Jan 30, 2024
melix added a commit to micronaut-projects/micronaut-test-resources that referenced this pull request Jan 30, 2024
* Upgrade testcontainers dependency

This should fix the problem with the mysql:latest image not being
compatible, as seen in several places like

micronaut-projects/micronaut-gradle-plugin#927
or micronaut-projects/micronaut-test#936
or https://github.com/micronaut-projects/micronaut-test-resources/pull/503/files

* Revert testcontainers redis dependency

As it is a breaking change
@CLAassistant
Copy link

CLAassistant commented Feb 7, 2024

CLA assistant check
All committers have signed the CLA.

@wetted
Copy link
Contributor

wetted commented Feb 8, 2024

@altro3 Thanks to @timyates catch, this is resolved and merged on #948. If you merge from master the r2dbc test should pass now.

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

don't we expose the JUnit5 BOM?

@altro3
Copy link
Contributor Author

altro3 commented Feb 14, 2024

I think in this case it is necessary for internal use in the miсronaut modules, because everyone is tired of constantly describing the junit5 library versions in the catalog, I would like them to connect from the micronaut-test catalog and be accessible via the mnTest.junit.jupiter.engine for example

@graemerocher
Copy link
Contributor

not sure who everyone refers to here, if the JUnit 5 BOM is already imported then it doesn't impact users. For us we have a way to import catalogs and use JUnit, see https://github.com/micronaut-projects/micronaut-reactor/blob/e671a8653a8f8a0fc316a2ede6b541ea088acd0b/settings.gradle#L29 for an example

@altro3
Copy link
Contributor Author

altro3 commented Feb 14, 2024

Well, the problem is that these libraries are not in the catalog now and in each project you need to describe this library in libs.versions.toml. That is, now you can connect the junit-jupiter-engine and junit-jupiter-params libraries only in this way (example from micronaut-openapi)
изображение

This task was not invented by me, but by @melix , I suspect that only managed libraries get into the version catalog. But this is not 100%

But judging by the description of the task, it is so

@melix
Copy link
Contributor

melix commented Feb 14, 2024

It seems that there are a couple things. The first is that we do import the junit platform BOM already. This means that in any project, you can add a dependency on JUnit without having to declare the version, as long as the micronaut-test bom is imported.

However, since JUnit is not a Micronaut platform, then you won't have accessors like mnTest.junit.platform. For this we need to add managed entries like in this PR. This will obviously only cover the modules we care about.

Strictly speaking, "everyone is tired of constantly describing the junit5 library versions in the catalog" is not a good argument:

  • first you only need to describe the group and artifact, since the version is already managed by the JUnit platform BOM
  • second, as soon as we expose something as managed, then it is exposed to consumers, not only us, but also users

In this case it seems reasonable to add such entries here, I think.

@altro3
Copy link
Contributor Author

altro3 commented Feb 14, 2024

True, but as you correctly noted, we already provide the user with access to the bom file. so I don't see a problem in making it easier to use junit using the version catalog.

@melix
Copy link
Contributor

melix commented Feb 14, 2024

we already provide the user with access to the bom file

It's the other way around ;) The BOM files are primarily meant for users, not us : they will impact the dependency resolution of user app. That's why we must be careful what we add there.

@altro3
Copy link
Contributor Author

altro3 commented Feb 14, 2024

Apparently, there is a complex logic hidden here that I don't really understand. For me, as a user, the only difference will be that I will be able to remove these 2 lines from the file:

junit-jupiter-engine = { module = "org.junit.jupiter:junit-jupiter-engine" }
junit-jupiter-params = { module = "org.junit.jupiter:junit-jupiter-params" }

which will give me pleasure :-)

@altro3
Copy link
Contributor Author

altro3 commented Feb 20, 2024

@graemerocher @melix Could you merge it?

@graemerocher graemerocher requested a review from sdelamo February 20, 2024 07:18
@sdelamo
Copy link
Contributor

sdelamo commented Feb 20, 2024

I don't like this change and I don't think we should follow this pattern in other modules. We are already exposing the BOM.

I see no issue of having in the modules.

junit-jupiter-engine = { module = "org.junit.jupiter:junit-jupiter-engine" }
junit-jupiter-params = { module = "org.junit.jupiter:junit-jupiter-params" }

@sdelamo sdelamo merged commit c28675c into micronaut-projects:master Feb 20, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

JUnit Engine should be managed
6 participants