[SUREFIRE-1972] Use current version of surefire-shared-utils #443

Tibor17 · 2022-01-14T21:18:14Z

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

Tibor17 · 2022-01-14T21:27:41Z

@michael-o
I hope the maven-release-plugin would not crash on this

<surefire-shared-utils.version>${project.version}</surefire-shared-utils.version>

  <dependencies>
    <dependency>
      <groupId>org.apache.maven.surefire</groupId>
      <artifactId>surefire-shared-utils</artifactId>
      <version>${surefire-shared-utils.version}</version>
      <classifier>shaded</classifier>
    </dependency>
  </dependencies>

michael-o · 2022-01-14T21:31:44Z

@michael-o I hope the maven-release-plugin would not crash on this

<surefire-shared-utils.version>${project.version}</surefire-shared-utils.version>

  <dependencies>
    <dependency>
      <groupId>org.apache.maven.surefire</groupId>
      <artifactId>surefire-shared-utils</artifactId>
      <version>${surefire-shared-utils.version}</version>
      <classifier>shaded</classifier>
    </dependency>
  </dependencies>

Why should it?

Tibor17 · 2022-01-14T21:37:20Z

@michael-o
Good answer! So we should not have any blocker here, right?

michael-o · 2022-01-14T21:43:55Z

Likely not. Let see what happens.

Tibor17 · 2022-01-16T22:08:33Z

In here I found out that the class path contains other artifacts however the shadefire does not have any transitive dependencies. So, we will verify it and fix it separately.

Currently, the internal surefire and failsafe plugins, which are used for internal testing purposes, should use the old version 3.0.0-M3. The reason is that M3 does not have surefire-shared-utils and the later versions have it but the artifact is not shaded in surefire-shadefire and therefore may produce conflicts in the class path if M4+ is used.

eolivelli · 2022-01-17T16:40:27Z

surefire-shared-utils/pom.xml

+                                </goals>
+                                <configuration>
+                                    <artifactId>surefire-shared-utils</artifactId>
+                                    <version>3-SNAPSHOT</version>


why 3-SNAPSHOT ? is this a random name ?

Because this version is not used in deployment or elsewhere. Basically the number also does not matter ;-)
The only reason why this section is here is the fact that Intellij IDEA and maybe Eclipse too, is not able to work with Shaded Jar in module surefire-shared-utils. So the IDEA is not able to load the shaded jar even if Maven attached it to the POM. So this is only the trick that we can enable this profile ide-development and identical Jar is loaded by IDEA from local repo and then we do not have the red lines in the code. Of course Maven build does not need it because Maven can work with shaded and attached Jar. But IDEA has this problem, pity.

@eoliveli I can change this number. Let me know pls.

I suppose that nobody will not install project with special profile ...

@slawekjaranowski
I spoke about it with @miachael-o , it is not Maven problem. The IDEA is an open source project and can be fixed, see the ticket. These are two separate issues and they are in progress..

eolivelli · 2022-01-17T16:40:46Z

maven-failsafe-plugin/pom.xml

@@ -266,12 +266,12 @@
                                    <postBuildHookScript>verify</postBuildHookScript>
                                    <settingsFile>src/it/settings.xml</settingsFile>
                                    <skipInvocation>${skipTests}</skipInvocation>
-                                    <streamLogs>true</streamLogs>


why are we changing this line ?

Because the info logs do not help. And debug logs cannot be read in the main logs.
Several days ago we fixed logs archives and so these logs of this module can be downloaded as a zip.

eolivelli · 2022-01-17T16:40:53Z

maven-failsafe-plugin/pom.xml

                                    <showErrors>true</showErrors>
                                    <properties>
                                        <integration-test-port>${failsafe-integration-test-port}</integration-test-port>
                                        <integration-test-stop-port>${failsafe-integration-test-stop-port}</integration-test-stop-port>
                                    </properties>
+                                    <debug>true</debug>


is this needed ?

Sometimes I need to have debug logs if I investigate a problem. For instance failures of downloading artifacts of plugin parameters, etc.

eolivelli · 2022-01-17T16:41:54Z

surefire-booter/pom.xml

@@ -115,11 +115,12 @@
      </plugin>
      <plugin>
        <artifactId>maven-surefire-plugin</artifactId>
+        <version>3.0.0-M3</version>


why are we using this older version "3.0.0-M3" ? what's the reasoning about choosing this one ? could it be 2.x ?

I can try with 2.22.2. Let's see what happens...

@eolivelli
I have used the 2.22.2 but it gives me bad result
[INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0
Sorry!

slawekjaranowski

For me it is too complicated ...

If I good understand reason for creating surefire-shared-utils is to hide used shared classes from testing code to avoid conflicts on it.

But I think that we can consider to drop surefire-shared-utils with all problems and use m-shade-p at the last components.
It will be more clear and easier.

Another case which I don't like we build shared-utils with many commons library packed in one black box .. and we don't know which common library and where is needed.

slawekjaranowski · 2022-01-17T20:07:41Z

pom.xml

+    <profile>
+        <!-- First, install the project without tests -> mvn install -DskipTests
+        This is a workaround for IntelliJ IDEA, see https://youtrack.jetbrains.com/issue/IDEA-148573
+        IDEA is able to recognize external artifacts with classifiers. But IDEA expects modules and their artifacts
+        without classifier. If the version differs from project, the idea would understand it as external artifact.
+        -->
+        <id>ide-development</id>
+        <properties>
+            <surefire-shared-utils.version>3-SNAPSHOT</surefire-shared-utils.version>
+        </properties>
+    </profile>


I suppose that nobody will not install project with special profile ...

@slawekjaranowski
Slawomir, we were talking about it in previous PR. There is no other way. I was explaining it that the IDE and Maven work different way, the Maven understands shading and attaching artifacts but the IDEs don't and that's the reason why it has to be like this. We don't live in ideal world but we cannot wait forever until Jetbrain would fix the issue. If the developers does not care about red line, okay, but I don't because I do not want to have red lines in the code and also I do not want to copy paste the source code from external libraries because it is not maintainable way. So this is the explanation. Regarding the version 3-SNAPSHOT, it can be anything, that's only cosmetic issue.

slawekjaranowski · 2022-01-17T20:08:56Z

...refire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireDependencyResolver.java

+    private static final String[][] PROVIDER_CLASSPATH_ORDER = {
+        // groupId, artifactId [, classifier]
+        {"org.apache.maven.surefire", "surefire-junit3"},
+        {"org.apache.maven.surefire", "surefire-junit4"},
+        {"org.apache.maven.surefire", "surefire-junit47"},
+        {"org.apache.maven.surefire", "surefire-testng"},
+        {"org.apache.maven.surefire", "surefire-junit-platform"},
+        {"org.apache.maven.surefire", "surefire-api"},
+        {"org.apache.maven.surefire", "surefire-logger-api"},
+        {"org.apache.maven.surefire", "surefire-shared-utils"},
+        {"org.apache.maven.surefire", "common-java5"},
+        {"org.apache.maven.surefire", "common-junit3"},
+        {"org.apache.maven.surefire", "common-junit4"},
+        {"org.apache.maven.surefire", "common-junit48"},
+        {"org.apache.maven.surefire", "common-testng-utils"}


Is it related to subject of PR?

yes, I know, this was related to the thinking process with classifier.
Let me pls squash the commits, it's easier to avoid changing this class afterwards.

slawekjaranowski · 2022-01-17T20:11:50Z

...refire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireDependencyResolver.java

        {
            Iterator<Artifact> providerArtifactsIt = providerArtifacts.iterator();
            while ( providerArtifactsIt.hasNext() )
            {
+                String groupId = coordinates[0];
+                String artifactId = coordinates[1];
+                String classifier = coordinates.length == 3 ? coordinates[2] : null;


iterate by static PROVIDER_CLASSPATH_ORDER, I don't see items with classifier ...

slawekjaranowski · 2022-01-17T20:13:23Z

surefire-shared-utils/pom.xml

+                                </goals>
+                                <configuration>
+                                    <artifactId>surefire-shared-utils</artifactId>
+                                    <version>3-SNAPSHOT</version>


I suppose that nobody will not install project with special profile ...

Tibor17 · 2022-01-17T20:42:22Z

@slawekjaranowski
Slawomir, you probably really do not understand why it is here.
It was here since the beginning but you do not know the history.
The reason behind it is the fact that the external libraries also use Maven and we use ourselves in the internal tests. This is nothing wrong and nothing atypical in Maven. It avoids conflicts on the classpath, and as I said before, it was here always even before me, and it had the purpose. We live in real world and maven-shade-plugin is used in Maven with purpose.

Tibor17 · 2022-01-17T21:39:39Z

@slawekjaranowski
SurefireDependencyResolver was removed from the commit.
Squashed commits.

Tibor17 mentioned this pull request Jan 14, 2022

[SUREFIRE-1972] Use current version of surefire-shared-utils #426

Closed

8 tasks

Tibor17 requested a review from slawekjaranowski January 14, 2022 21:37

Tibor17 requested a review from eolivelli January 15, 2022 07:56

Tibor17 force-pushed the jira-1972-2 branch from aae9471 to 9bdc848 Compare January 16, 2022 21:55

eolivelli reviewed Jan 17, 2022

View reviewed changes

slawekjaranowski reviewed Jan 17, 2022

View reviewed changes

[SUREFIRE-1972] Use current version of surefire-shared-utils

ad5f7a1

Tibor17 force-pushed the jira-1972-2 branch from 46250e7 to ad5f7a1 Compare January 17, 2022 21:23

[github] max-parallel: 4, timeout-minutes: 7 hours

cee232d

slawekjaranowski approved these changes Jan 18, 2022

View reviewed changes

Tibor17 merged commit 4b549e4 into master Jan 18, 2022

Tibor17 deleted the jira-1972-2 branch January 18, 2022 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SUREFIRE-1972] Use current version of surefire-shared-utils #443

[SUREFIRE-1972] Use current version of surefire-shared-utils #443

Tibor17 commented Jan 14, 2022

Tibor17 commented Jan 14, 2022 •

edited

michael-o commented Jan 14, 2022

Tibor17 commented Jan 14, 2022

michael-o commented Jan 14, 2022

Tibor17 commented Jan 16, 2022

eolivelli Jan 17, 2022

Tibor17 Jan 17, 2022

Tibor17 Jan 17, 2022

slawekjaranowski Jan 17, 2022

Tibor17 Jan 17, 2022

eolivelli Jan 17, 2022

Tibor17 Jan 17, 2022

eolivelli Jan 17, 2022

Tibor17 Jan 17, 2022

eolivelli Jan 17, 2022

Tibor17 Jan 17, 2022

Tibor17 Jan 17, 2022

slawekjaranowski left a comment

slawekjaranowski Jan 17, 2022

Tibor17 Jan 17, 2022

slawekjaranowski Jan 17, 2022

Tibor17 Jan 17, 2022

slawekjaranowski Jan 17, 2022

slawekjaranowski Jan 17, 2022

Tibor17 commented Jan 17, 2022

Tibor17 commented Jan 17, 2022

[SUREFIRE-1972] Use current version of surefire-shared-utils #443

[SUREFIRE-1972] Use current version of surefire-shared-utils #443

Conversation

Tibor17 commented Jan 14, 2022

Tibor17 commented Jan 14, 2022 • edited

michael-o commented Jan 14, 2022

Tibor17 commented Jan 14, 2022

michael-o commented Jan 14, 2022

Tibor17 commented Jan 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slawekjaranowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 commented Jan 17, 2022

Tibor17 commented Jan 17, 2022

Tibor17 commented Jan 14, 2022 •

edited