Conversation
| <type>pom</type> | ||
| <scope>import</scope> | ||
| </dependency> | ||
| <!-- Test dependencies --> |
There was a problem hiding this comment.
Can we move this line before junit-bom?
|
Can you please add more info regarding the skipped test classes? Maybe add comments to BaseSerializationTest and PR description? |
java-core/google-cloud-core/src/test/java/com/google/cloud/BaseSerializationTest.java
Show resolved
Hide resolved
java-core/pom.xml
Outdated
| <groupId>org.junit</groupId> | ||
| <artifactId>junit-bom</artifactId> | ||
| <version>5.10.2</version> | ||
| <type>pom</type> | ||
| <scope>import</scope> |
There was a problem hiding this comment.
Might be missing some context about dependency management with java-core and/or test jars, but I would have assumed this version would be been defined in pom-parent or shared-deps instead of having to declare it here.
There was a problem hiding this comment.
I think declare a test dependency in java-core is fine since it allows other modules to use different testing framework.
That being said, we can declare test dependencies in a centralized place, e.g., third-party dependency. However, this should be a separate task.
There was a problem hiding this comment.
@suztomo Do you have any suggestions of what's the best way to manage junit 5 dependencies?
There was a problem hiding this comment.
Avoid declaring junit bom in the dependencyManagement section of pom.xml files that would go to the Libraries BOM, such as the pom-parent or gapic-generator-java-bom. That would control library users' junit usage.
Third-party-dependencies is good but java-core, I believe, does not import it.
How about declaring the JUnit version as a property, not dependencyManagement section, in the pom-parent?
java-core/pom.xml
Outdated
| <artifactId>junit-bom</artifactId> | ||
| <version>${junit.version}</version> | ||
| <type>pom</type> | ||
| <scope>import</scope> |
There was a problem hiding this comment.
Per @suztomo's suggestion, I don't think we want to import junit bom in java-core, as I think it would go into shared-dependencies. So I think we should declare those three test dependencies individually and manage the version through the parent pom, is that correct @suztomo ?
There was a problem hiding this comment.
I don't think we want to import junit bom in java-core, as I think it would go into shared-dependencies
java-core it not imported by the shared dependencies BOM.
Having JUnit in third-party-dependencies is a good place. Because third-party-dependencies BOM does not go to the Libraries BOM.
There was a problem hiding this comment.
java-core it not imported by the shared dependencies BOM.
I think it is included in the first-party-dependencies, am I missing something?
There was a problem hiding this comment.
java-core imports shared dependencies in parant pom, why this doesn't cause a circular dependency?
There was a problem hiding this comment.
I think it is included in the first-party-dependencies, am I missing something?
Suppose a project, A, imports shared dependencies, then A doesn't need to declare java-core version (imported from google-cloud-core-bom), but A needs to declare junit version since import BOM doesn't work for transitive dependencies.
There was a problem hiding this comment.
am I missing something?
Importing a BOM and declaring dependencies are different. For a BOM, only imported BOMs matters.
There was a problem hiding this comment.
IIUC, first-party-dependencies imports google-cloud-core bom, which also imports junit bom, so junit bom would ends up in libraries bom?
There was a problem hiding this comment.
IIUC, first-party-dependencies imports google-cloud-core bom, which also imports junit bom, so junit bom would ends up in libraries bom?
Actually, junit bom will not end up in libraries bom since only artifacts declared in google-cloud-core-bom will end up in libraries bom. In this case, they are google-cloud-core, google-cloud-core-grpc, google-cloud-core-http.
junit, however, is a transitive dependency, which does not appear in google-cloud-core-bom.
There was a problem hiding this comment.
So I think we should declare those three test dependencies individually and manage the version through the parent pom
I think use junit-bom is better because junit-platform-launcher has a different version than junit-jupiter-engine and junit-vintage-engine.
java-core/google-cloud-core/src/test/java/com/google/cloud/ServiceOptionsTest.java
Outdated
Show resolved
Hide resolved
java-core/google-cloud-core/src/test/java/com/google/cloud/TimestampTest.java
Show resolved
Hide resolved
java-core/google-cloud-core/src/test/java/com/google/cloud/testing/VersionTest.java
Outdated
Show resolved
Hide resolved
|
|
Fix #2726. `BaseSerializationTest` will not migrate to Junit 5 because downstream libraries, e.g., java-logging, are extending this class and these libraries still use Junit 4. Migrating this class to Junit 5 will cause test failures in downstream libraries.
Fix #2726. `BaseSerializationTest` will not migrate to Junit 5 because downstream libraries, e.g., java-logging, are extending this class and these libraries still use Junit 4. Migrating this class to Junit 5 will cause test failures in downstream libraries.
4 participants
Fix #2726.
BaseSerializationTestwill not migrate to Junit 5 because downstream libraries, e.g., java-logging, are extending this class and these libraries still use Junit 4. Migrating this class to Junit 5 will cause test failures in downstream libraries.