[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232501824
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,96 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-jupiter-engine" and its transitive dependencies
+String jupiterVersion = junitJupiterApi.getBaseVersion();
+resolve( providerArtifacts, "org.junit.jupiter",
"junit-jupiter-engine", jupiterVersion );
+}
+
+private void resolveJUnitVintageEngine( Set
providerArtifacts )
+{
+Artifact junit = getProjectArtifactMap().get( "junit:junit" );
+if ( junit == null ) // no api, no engine
+{
+return;
+}
+if ( !junit.getBaseVersion().equals( "4.12" ) ) // not "JUnit
4.12", no engine
+{
+return;
+}
+Artifact junitVintageEngine = getProjectArtifactMap().get(
"org.junit.vintage:junit-vintage-engine" );
+if ( junitVintageEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-vintage-engine" and its transitive dependencies
+// heuristic: from Platform "x.y.z" to Vintage "5" + ".y.z"
Review comment:
@sormuras
A motivation can be e.g. `AbstractSurefireMojoTest` or
`AbstractSurefireMojoJava7PlusTest`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232501377
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,96 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-jupiter-engine" and its transitive dependencies
+String jupiterVersion = junitJupiterApi.getBaseVersion();
+resolve( providerArtifacts, "org.junit.jupiter",
"junit-jupiter-engine", jupiterVersion );
+}
+
+private void resolveJUnitVintageEngine( Set
providerArtifacts )
+{
+Artifact junit = getProjectArtifactMap().get( "junit:junit" );
+if ( junit == null ) // no api, no engine
+{
+return;
+}
+if ( !junit.getBaseVersion().equals( "4.12" ) ) // not "JUnit
4.12", no engine
+{
+return;
+}
+Artifact junitVintageEngine = getProjectArtifactMap().get(
"org.junit.vintage:junit-vintage-engine" );
+if ( junitVintageEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-vintage-engine" and its transitive dependencies
+// heuristic: from Platform "x.y.z" to Vintage "5" + ".y.z"
Review comment:
We are having low coverage without unit tests which is bad reputation for a
test f/w. The green curve goes down but the red one is worse because uncovered
lines grow with this commit since the lack of unit tests. The tests will be a
good platform for making fast experiments rather than executing all the build
and debugging a plugin. Unit testing must be a fun for a developer because he
can see that it works - it's green again.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232501049
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,96 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-jupiter-engine" and its transitive dependencies
+String jupiterVersion = junitJupiterApi.getBaseVersion();
+resolve( providerArtifacts, "org.junit.jupiter",
"junit-jupiter-engine", jupiterVersion );
+}
+
+private void resolveJUnitVintageEngine( Set
providerArtifacts )
+{
+Artifact junit = getProjectArtifactMap().get( "junit:junit" );
+if ( junit == null ) // no api, no engine
+{
+return;
+}
+if ( !junit.getBaseVersion().equals( "4.12" ) ) // not "JUnit
4.12", no engine
+{
+return;
+}
+Artifact junitVintageEngine = getProjectArtifactMap().get(
"org.junit.vintage:junit-vintage-engine" );
+if ( junitVintageEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-vintage-engine" and its transitive dependencies
+// heuristic: from Platform "x.y.z" to Vintage "5" + ".y.z"
Review comment:
It's not really our Maven style. Try to experiment with these calls:
`Artifact.getAvailableVersions()` and `ArtifactVersion.getMajorVersion()`,
minor, incremental, classifier, etc. It's definitely better than
`getBaseVersion().substring( 1 )`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232478958
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,96 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-jupiter-engine" and its transitive dependencies
+String jupiterVersion = junitJupiterApi.getBaseVersion();
+resolve( providerArtifacts, "org.junit.jupiter",
"junit-jupiter-engine", jupiterVersion );
+}
+
+private void resolveJUnitVintageEngine( Set
providerArtifacts )
+{
+Artifact junit = getProjectArtifactMap().get( "junit:junit" );
+if ( junit == null ) // no api, no engine
+{
+return;
+}
+if ( !junit.getBaseVersion().equals( "4.12" ) ) // not "JUnit
4.12", no engine
+{
+return;
+}
+Artifact junitVintageEngine = getProjectArtifactMap().get(
"org.junit.vintage:junit-vintage-engine" );
+if ( junitVintageEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-vintage-engine" and its transitive dependencies
+// heuristic: from Platform "x.y.z" to Vintage "5" + ".y.z"
Review comment:
Who can say that the versions will be always like this?
Somebody has to watch the versions in Maven Central and still update this
code. What if the version is not successfully resolved?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232478805
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,92 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-jupiter-engine" and its transitive dependencies
+String version = junitJupiterApi.getBaseVersion();
+resolve( providerArtifacts, "org.junit.jupiter",
"junit-jupiter-engine", version );
+}
+
+private void resolveJUnitVintageEngine( Set
providerArtifacts )
+{
+Artifact junit = getProjectArtifactMap().get( "junit:junit" );
+if ( junit == null || !junit.getBaseVersion().equals( "4.12" ) )
// no or wrong "junit", no engine
+{
+return;
+}
+Artifact junitVintageEngine = getProjectArtifactMap().get(
"org.junit.vintage:junit-vintage-engine" );
+if ( junitVintageEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-vintage-engine" and its transitive dependencies
+// heuristic: from Platform "x.y.z" to Vintage "5" + ".y.z"
+String junitVintageVersion = "5" +
junitPlatformArtifact.getBaseVersion().substring( 1 );
+resolve( providerArtifacts, "org.junit.vintage",
"junit-vintage-engine", junitVintageVersion );
+}
+
+private void alignJUnitPlatformVersion( Set
providerArtifacts )
+{
+Map providerArtifactMap = new HashMap();
+for ( Artifact artifact : providerArtifacts )
+{
+String key = artifact.getGroupId() + ":" +
artifact.getArtifactId();
+providerArtifactMap.put( key, artifact );
+}
+Artifact defaultLauncher = providerArtifactMap.get(
"org.junit.platform:junit-platform-launcher" );
+logDebugOrCliShowErrors( "JUnit Platform Artifact: " +
junitPlatformArtifact );
+logDebugOrCliShowErrors( "JUnit Platform Launcher: " +
defaultLauncher );
+if ( junitPlatformArtifact.getVersion().equals(
defaultLauncher.getVersion() ) )
+{
+logDebugOrCliShowErrors( "JUnit Platform versions are equal -
proceeding anyway... " );
Review comment:
If it must have `return` then keep it of course.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232477337
##
File path:
surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##
@@ -243,7 +243,7 @@ private StackTraceWriter getStackTraceWriter(
TestIdentifier testIdentifier, Thr
{
String className = getClassName( testIdentifier );
String methodName = getMethodName( testIdentifier ).orElse( "" );
-return new PojoStackTraceWriter( className, methodName, throwable );
+return new LegacyPojoStackTraceWriter( className, methodName,
throwable );
Review comment:
Yes to restore please. We should use the previous `PojoStackTraceWriter`.
The old `LegacyPojoStackTraceWriter` has bad user experience on the console.
Now it has only one purpose within plugin's process. Maybe it could be moved to
`surefire-common`. We will have a chance to break backwards compatibility in
3.0.0.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232477165 ## File path: surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformEnginesIT.java ## @@ -93,12 +93,10 @@ public void platform() throws VerificationException String testClasspath = "[DEBUG] test(compact) classpath:" + " test-classes" + " classes" -+ " junit-jupiter-engine-" + jupiter + ".jar" ++ " junit-jupiter-api-" + jupiter + ".jar" Review comment: Excellent, we should document a new user experience with examples: 1. with engine dependency 2. with API dependency (since of `3.0.0-M2`) Do we have third option we can document with this fix? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232476986
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,92 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-jupiter-engine" and its transitive dependencies
+String version = junitJupiterApi.getBaseVersion();
+resolve( providerArtifacts, "org.junit.jupiter",
"junit-jupiter-engine", version );
+}
+
+private void resolveJUnitVintageEngine( Set
providerArtifacts )
+{
+Artifact junit = getProjectArtifactMap().get( "junit:junit" );
+if ( junit == null || !junit.getBaseVersion().equals( "4.12" ) )
// no or wrong "junit", no engine
Review comment:
I only wanted to say that it is a Clean Code to write expressive code for
places where were previous comments.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232476986
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,92 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-jupiter-engine" and its transitive dependencies
+String version = junitJupiterApi.getBaseVersion();
+resolve( providerArtifacts, "org.junit.jupiter",
"junit-jupiter-engine", version );
+}
+
+private void resolveJUnitVintageEngine( Set
providerArtifacts )
+{
+Artifact junit = getProjectArtifactMap().get( "junit:junit" );
+if ( junit == null || !junit.getBaseVersion().equals( "4.12" ) )
// no or wrong "junit", no engine
Review comment:
I only wanted to say that it is a Clean Code to write expressive code for
places where were previus comments.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232467808
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,92 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
Review comment:
I think `get()` in hashMap is fast enough which value can be used to
evaluate it within IF argument and then continue with the main stuff. It makes
sense to me to warp the algorithm around IF-ELSE without using `return` in the
middle within the algorithm.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232467808
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,92 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
Review comment:
I think `get()` in hashMap is fast enough which value can be used to
evaluate it within IF argument and then cntinue with the main stuff. It makes
sense to me to warp the algorithm around IF-ELSE without using `return` in the
middle within the algorithm.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232467703
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,92 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-jupiter-engine" and its transitive dependencies
+String version = junitJupiterApi.getBaseVersion();
+resolve( providerArtifacts, "org.junit.jupiter",
"junit-jupiter-engine", version );
+}
+
+private void resolveJUnitVintageEngine( Set
providerArtifacts )
+{
+Artifact junit = getProjectArtifactMap().get( "junit:junit" );
+if ( junit == null || !junit.getBaseVersion().equals( "4.12" ) )
// no or wrong "junit", no engine
Review comment:
The comments like this can be substituted with good name of `boolean`
variable which expressed the purpose.
For instance, `boolean hasApplicableJUnit4ArtifactInProject`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232467703
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,92 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
+resolveJUnitJupiterEngine( providerArtifacts );
+resolveJUnitVintageEngine( providerArtifacts );
+return providerArtifacts;
+}
+
+private void resolveJUnitJupiterEngine( Set
providerArtifacts )
+{
+Artifact junitJupiterApi = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-api" );
+if ( junitJupiterApi == null ) // no api, no engine
+{
+return;
+}
+Artifact junitJupiterEngine = getProjectArtifactMap().get(
"org.junit.jupiter:junit-jupiter-engine" );
+if ( junitJupiterEngine != null ) // engine already resolved by
project
+{
+return;
+}
+// resolve "junit-jupiter-engine" and its transitive dependencies
+String version = junitJupiterApi.getBaseVersion();
+resolve( providerArtifacts, "org.junit.jupiter",
"junit-jupiter-engine", version );
+}
+
+private void resolveJUnitVintageEngine( Set
providerArtifacts )
+{
+Artifact junit = getProjectArtifactMap().get( "junit:junit" );
+if ( junit == null || !junit.getBaseVersion().equals( "4.12" ) )
// no or wrong "junit", no engine
Review comment:
The comments like this can be substituted with good name of `boolean`
variable which expressed the purpose.
For instance, `boolean hasApplicableJUnit4InProject`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232467602
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -2846,8 +2849,92 @@ public void addProviderProperties()
@Nonnull
public Set getProviderClasspath()
{
+String provider = "surefire-junit-platform";
String version = surefireBooterArtifact.getBaseVersion();
-return dependencyResolver.getProviderClasspath(
"surefire-junit-platform", version );
+Set providerArtifacts =
dependencyResolver.getProviderClasspath( provider, version );
+alignJUnitPlatformVersion( providerArtifacts );
Review comment:
LGTM
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232467428 ## File path: surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformEnginesIT.java ## @@ -93,12 +93,10 @@ public void platform() throws VerificationException String testClasspath = "[DEBUG] test(compact) classpath:" + " test-classes" + " classes" -+ " junit-jupiter-engine-" + jupiter + ".jar" ++ " junit-jupiter-api-" + jupiter + ".jar" Review comment: Perfect! This makes sense for users. They will see only API. But this reminds me to think about IDE which is not able to run the tests without implementation of engines. Did you try the same test in Eclipse or IntelliJ IDEA? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP]
Resolve missing and align "JUnit 5" artifacts
URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232467317
##
File path:
surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##
@@ -243,7 +243,7 @@ private StackTraceWriter getStackTraceWriter(
TestIdentifier testIdentifier, Thr
{
String className = getClassName( testIdentifier );
String methodName = getMethodName( testIdentifier ).orElse( "" );
-return new PojoStackTraceWriter( className, methodName, throwable );
+return new LegacyPojoStackTraceWriter( className, methodName,
throwable );
Review comment:
Do you have time for this? We talked about it on Slack.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts
Tibor17 commented on a change in pull request #196: [SUREFIRE-1585] [WIP] Resolve missing and align "JUnit 5" artifacts URL: https://github.com/apache/maven-surefire/pull/196#discussion_r232467299 ## File path: surefire-its/src/test/resources/junit-platform/pom.xml ## @@ -70,4 +64,17 @@ + + Review comment: There are no snapshot plugins. Why you added this section? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
