You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/11/06 11:16:28 UTC
[GitHub] [maven-surefire] slawekjaranowski opened a new pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
slawekjaranowski opened a new pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393
Before final change I did small refractor for easy testing by unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r748826991
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ProviderDetector.java
##########
@@ -0,0 +1,104 @@
+package org.apache.maven.surefire.providerapi;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.annotation.Nonnull;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.maven.surefire.api.provider.SurefireProvider;
+import org.codehaus.plexus.component.annotations.Component;
+import org.codehaus.plexus.component.annotations.Requirement;
+import org.codehaus.plexus.logging.Logger;
+
+import static java.lang.Thread.currentThread;
+
+/**
+ * @author Kristian Rosenvold
+ */
+@Component( role = ProviderDetector.class )
+public final class ProviderDetector
Review comment:
used in ASM from another package ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r746954542
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ConfigurableProviderInfo.java
##########
@@ -1,4 +1,4 @@
-package org.apache.maven.plugin.surefire;
+package org.apache.maven.surefire.providerapi;
Review comment:
I put all classes responsible for providers detecting in one package.
Before we have:
`org.apache.maven.plugin.surefire.booterclient.ProviderDetector`
which use
`org.apache.maven.surefire.providerapi.ServiceLoader`
and `ProviderDetector` was used only by `AbstractSurefireMojo` so one responsibility was in many package.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#issuecomment-974372072
Pls do not change the code conventions with another purpose of the PR because then the reviewer has more work to check every line. Some contributors use to add hacks along another purpose and they think we would not find it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#issuecomment-968355971
@slawekjaranowski
The Jenkins build is under investigation in the `jenkins` branch
https://ci-maven.apache.org/job/Maven/job/maven-box/job/maven-surefire/job/jenkins/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r746920580
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ProviderDetector.java
##########
@@ -0,0 +1,104 @@
+package org.apache.maven.surefire.providerapi;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.annotation.Nonnull;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.maven.surefire.api.provider.SurefireProvider;
+import org.codehaus.plexus.component.annotations.Component;
+import org.codehaus.plexus.component.annotations.Requirement;
+import org.codehaus.plexus.logging.Logger;
+
+import static java.lang.Thread.currentThread;
+
+/**
+ * @author Kristian Rosenvold
+ */
+@Component( role = ProviderDetector.class )
+public final class ProviderDetector
Review comment:
Nicely extracted class. I want to ask about the modifiers `public`. Are they important?
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -1197,13 +1200,12 @@ private void executeAfterPreconditionsChecked( @Nonnull DefaultScanResult scanRe
throws MojoExecutionException
{
Artifact junitDepArtifact = getJunitDepArtifact();
- return new ProviderList( new DynamicProviderInfo( null ),
- new JUnitPlatformProviderInfo( getJUnit5Artifact(), testClasspath ),
- new TestNgProviderInfo( getTestNgArtifact() ),
- new JUnitCoreProviderInfo( getJunitArtifact(), junitDepArtifact ),
- new JUnit4ProviderInfo( getJunitArtifact(), junitDepArtifact ),
- new JUnit3ProviderInfo() )
- .resolve();
+ return providerDetector.resolve( new DynamicProviderInfo( null ),
Review comment:
Why this line has changed? Previously the line was `return new ProviderList( new DynamicProviderInfo( null ),`.
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ConfigurableProviderInfo.java
##########
@@ -22,7 +22,7 @@
/**
* @author Kristian Rosenvold
*/
-interface ConfigurableProviderInfo
+public interface ConfigurableProviderInfo
Review comment:
@slawekjaranowski This interface is only used ASM class or you think that other project which extends the ASM class can instantiate it in the subclass of ASM class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r749697086
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ServiceLoader.java
##########
@@ -42,7 +45,8 @@
*
* @since 2.20
*/
-public final class ServiceLoader
+@Component( role = ServiceLoader.class )
+public class ServiceLoader
Review comment:
@slawekjaranowski Can this Component be also `public final` class as the other classes in this package? So that we indicate that the component class cannot be extended by the user in another project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r746916936
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ConfigurableProviderInfo.java
##########
@@ -1,4 +1,4 @@
-package org.apache.maven.plugin.surefire;
+package org.apache.maven.surefire.providerapi;
Review comment:
@slawekjaranowski Why the package has changed? There are probably many classes in the new packages, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 merged pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 merged pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#issuecomment-974372072
Pls do not change the code conventions with another purpose of the PR because then the reviewer has more work to check every line. Some contributors use to add hacks along another purpose and they think we would not find it. It's better to have another PR for coding conventions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r749727367
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ServiceLoader.java
##########
@@ -42,7 +45,8 @@
*
* @since 2.20
*/
-public final class ServiceLoader
+@Component( role = ServiceLoader.class )
+public class ServiceLoader
Review comment:
I use mock `ServiceLoader` of in `ProviderDetectorTest` but final clases can't be mocked.
I can introduce interfaces for components and mock on interface will be posible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#issuecomment-973349950
@slawekjaranowski
Pls rebase to remote master and resolve the conflicts in `AbstractSurefireMojo` and `AbstractSurefireMojoTest`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r746916936
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ConfigurableProviderInfo.java
##########
@@ -1,4 +1,4 @@
-package org.apache.maven.plugin.surefire;
+package org.apache.maven.surefire.providerapi;
Review comment:
@slawekjaranowski Why the package has changed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#issuecomment-965662886
Hi @slawekjaranowski , I am just having a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r746916936
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ConfigurableProviderInfo.java
##########
@@ -1,4 +1,4 @@
-package org.apache.maven.plugin.surefire;
+package org.apache.maven.surefire.providerapi;
Review comment:
@slawekjaranowski Why the package has changed? There are probably many classes in the new package, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r748827350
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -1197,13 +1200,12 @@ private void executeAfterPreconditionsChecked( @Nonnull DefaultScanResult scanRe
throws MojoExecutionException
{
Artifact junitDepArtifact = getJunitDepArtifact();
- return new ProviderList( new DynamicProviderInfo( null ),
- new JUnitPlatformProviderInfo( getJUnit5Artifact(), testClasspath ),
- new TestNgProviderInfo( getTestNgArtifact() ),
- new JUnitCoreProviderInfo( getJunitArtifact(), junitDepArtifact ),
- new JUnit4ProviderInfo( getJunitArtifact(), junitDepArtifact ),
- new JUnit3ProviderInfo() )
- .resolve();
+ return providerDetector.resolve( new DynamicProviderInfo( null ),
Review comment:
now providerDetector is component, so we use service method
Code from inner class `ProviderList` are in `ProviderDetector`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r748826837
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ConfigurableProviderInfo.java
##########
@@ -22,7 +22,7 @@
/**
* @author Kristian Rosenvold
*/
-interface ConfigurableProviderInfo
+public interface ConfigurableProviderInfo
Review comment:
before we have, in package: `org.apache.maven.plugin.surefire`
- interface `ConfigurableProviderInfo` extends `ProviderInfo`
- public interface `ProviderInfo`
both used in `AbstarctSurefireMojo` for implementing inner class like xxxProviderInfo
Now are moved to `providerapi` and both must be public for technical purpose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#issuecomment-974378554
@slawekjaranowski gitbox is down, so i have to wait with your Jira ticket to close it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#issuecomment-973939680
Rebased
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#issuecomment-962652369
Hi @Tibor17
I hope we can merge such improvement before change some of feature. It allow easier unit tests to do.
I used Plexus IoC ... because project use maven 3.0 and for JSR330 probably 3.1+ is required ... in next steps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r749695292
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ConfigurableProviderInfo.java
##########
@@ -1,4 +1,4 @@
-package org.apache.maven.plugin.surefire;
+package org.apache.maven.surefire.providerapi;
Review comment:
yes, I see there are now 5 classes. LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #393: [SUREFIRE-1954] move inner class ProviderList to upper level
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #393:
URL: https://github.com/apache/maven-surefire/pull/393#discussion_r749728925
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ServiceLoader.java
##########
@@ -42,7 +45,8 @@
*
* @since 2.20
*/
-public final class ServiceLoader
+@Component( role = ServiceLoader.class )
+public class ServiceLoader
Review comment:
ah ok, so let it be, the tests with mock are more important
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org