Surefire/Failsafe plugin configuration propagated to Junit/TestNG launch configuration #1672

treilhes · 2024-02-13T01:02:50Z

The following arguments are supported:

<argLine>,

<environmentVariables>,

<systemPropertyVariables>,

<workingDirectory>,

<enableAssertions>,

Configuration is propagated on unit test launch configuration creation and also when executing maven > update project

By default and if found, the plugin maven-dependency-plugin (goal: properties) is executed before updating the launch configuration to load properties.

If properties set by other plugins are used in the failsafe/surefire plugin configuration it is possible to override the default loading behaviour by providing the list plugins and goals to execute using the property m2e.launch.configuration.prerequisites

The expected format is as follow:

groupId1:artifactId1:goal1[,groupIdX:artifactIdX:goalX]*

Ex:
<m2e.launch.configuration.prerequisites>org.apache.maven.plugins:maven-dependency-plugin:properties,org.codehaus.mojo:properties-maven-plugin:read-project-properties</m2e.launch.configuration.prerequisites>

feature/surefire_plugins_arguments_to_launch_config

treilhes · 2024-02-13T01:06:50Z

Fixes #700

github-actions · 2024-02-13T01:35:15Z

Test Results

0 tests - 665 0 ✅ - 654 0s ⏱️ - 9m 26s
0 suites - 107 0 💤 - 10
0 files - 107 0 ❌ - 1

Results for commit 9e896d2. ± Comparison against base commit 8d3ff09.

♻️ This comment has been updated with latest results.

uncommented testng even if not executed

…ch_config

HannesWell · 2024-02-15T23:38:09Z

@treilhes thank you for this contribution. In general it is interesting and definitely a good thing to support.

Unfortunately I won't have time to review this large contribution over the weekend and will then be on vacation for three weeks.
So unless another committer reviews this your patience is kindly requested.

treilhes · 2024-02-16T07:41:22Z

@HannesWell
Don't worry, my patience will handle this just fine. Have great holidays. thank you for having warned

feature/surefire_plugins_arguments_to_launch_config

…ents_to_launch_config

treilhes · 2024-03-18T06:52:22Z

@HannesWell, I miss you so much, I long to hear from you about your vacation (and to get a bit of review) 💔

…ents_to_launch_config

HannesWell · 2024-03-18T18:19:07Z

@HannesWell, I miss you so much, I long to hear from you about your vacation (and to get a bit of review) 💔

My vacation was great, thanks for asking. I'll do my best to review this at the weekend (when I arrived back home a full ToDo-List hit immediately).

…ch_config

treilhes · 2024-04-25T21:11:09Z

@HannesWell Bump ❤️

HannesWell

Hi @treilhes first of all a big sorry for the delay and thanks for your patience.
It's just currently a lot to do, but I still have you on my TODO list a will try my best that we can complete this before the next release (ideally with some time left).

I had a a very brief overview over this change and I find your solution to adjust the launch config very interesting. I assume this works reliably in various scenarios like creating via a short-cut or modifying an existing config? If all works as expected that's a smart solution.
I'm asking because having a mechanism for that is a longer standing issue:

means for m2e to set system properties for some tests eclipse-jdt/eclipse.jdt.core#1183

Overall I didn't see any blockers and I'm glad you added extensive tests.

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

HannesWell · 2024-04-25T21:48:49Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+     * @param facade the maven project facade
+     * @param monitor the progress monitor
+     */
+    private void loadPrerequisites(ILaunchConfiguration configuration, IMavenProjectFacade facade,


Can't/shouldn't the list of mojos executed before the test-plugin be derived from the computed Maven lifecycle?

This one is a bit more complex as i think it can greatly impact performance. I'm afraid executing all plugins available in the maven lifecycle until this phase without knowing which ones are effectively producing properties is risky.
The only plugin provided by the maven team can safely be handled by default but about the others ....i'm not sure.
On the other hand providing the list of plugins to execute using a property feals more like a dirty hack than a clean solution.

If you have a way to know if a plugin generate some properties or a cleaner way to provide the list of plugin to execute it would be great as i plan to use the same kind of loading in another PR to be able to provide values for --patch-module directives from the compiler plugin to eclipse .classpath file in the fututre.

At this point i only can think of other ways like:

providing a PR to maven surefire/failsafe/compiler plugin to provide the same kind of feature as the property but i'm skeptical the maven team will accept this PR more dedicated to IDE/Eclipse than maven itself

providing some warning markers and some potential "quick fix" into the pom warning the user that plugins aren't executed until some configuration (property/project's preference) is set. (I'm completly clueless about how to do that but it would be cool but also time consuming)

What do you (or any other interested ) think about that?

Hello @HannesWell,
After some research it seems the best way to handle this may be the lifecycle-metadata-mapping.
Currently a plugin may have the following configuration in the mapping:

ignore
execute(runOnConfiguration/runOnIncremental)
generator

So i propose to add a boolean "runOnLaunchConfiguration" or something more generic to fit the compiler case like "provideProperties" to the execute element to mark all plugins that need to be executed before setting up a launch configuration.

About the "generator" case, a new method can be added to org.eclipse.m2e.core.project.configurator.AbstractProjectConfigurator :

public void configureLaunchConfiguration(LaunchConfigurationRequest request, IProgressMonitor monitor) throws CoreException

or something like

public void loadPropertiesInContext(??, IProgressMonitor monitor) throws CoreException

What do you think about that ?

I think leveraging the lifecycle-mapping-metadata is the the better approach.
It would be ideal if we could re-use the existing metadata, but I assume it doesn't fit well in all cases, does it?
If new arguments are added, it should as general as possible and as specific as necessary.

But from what you have described enhancing the https://github.com/codehaus-plexus/plexus-build-api would maybe be another option?

Maybe @laeubi can help here too?

In order to simplify the review of this big change, I think it would be good to move the resolution of variables created by other plugins into a separate PR.
Would that be ok?

…ch_config

HannesWell

I made another pass over this PR and have a couple of suggestions, mainly about reducing code size.

HannesWell · 2024-05-25T12:34:36Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+/********************************************************************************
+ * Copyright (c) 2024, 2024 pascal treilhes and others
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *   pascal treilhes - initial API and implementation
+ ********************************************************************************/


Suggested change

/********************************************************************************

* Copyright (c) 2024, 2024 pascal treilhes and others

*

* This program and the accompanying materials are made available under the

* terms of the Eclipse Public License 2.0 which is available at

* http://www.eclipse.org/legal/epl-2.0.

*

* SPDX-License-Identifier: EPL-2.0

*

* Contributors:

* pascal treilhes - initial API and implementation

********************************************************************************/

/********************************************************************************

* Copyright (c) 2024, 2024 Pascal Treilhes and others

*

* This program and the accompanying materials are made available under the

* terms of the Eclipse Public License 2.0 which is available at

* http://www.eclipse.org/legal/epl-2.0.

*

* SPDX-License-Identifier: EPL-2.0

*

* Contributors:

* Pascal Treilhes - initial API and implementation

********************************************************************************/

HannesWell · 2024-05-25T12:38:11Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+    /**
+     * Get the launch arguments
+     * 
+     * @param configuration the configuration
+     * @param facade the maven project facade
+     * @param monitor the progress monitor
+     * @return the launch arguments
+     * @throws CoreException
+     */


Adding java-doc to internal/private methods is not mandatory and if it just provides the information one can easily derive from the method and parameter names I find it even disturbing.

Therefore in this case and all other cases where the java-doc only provides obvious information please remove it.

Suggested change

/**

* Get the launch arguments

*

* @param configuration the configuration

* @param facade the maven project facade

* @param monitor the progress monitor

* @return the launch arguments

* @throws CoreException

*/

HannesWell · 2024-05-25T13:55:14Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+  /**
+   * Holder for the surefire/failsafe launch arguments
+   */
+  private static class TestLaunchArguments {


This and the other internal class can be reworked to be a one/two-line record.
Documentation each element in this case is probably also not necessary if it just replicates the the component name. Therefore it can be removed in most, if not all cases.

HannesWell · 2024-05-28T20:54:50Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+  /**
+   * Feature flag to enable or disable the support
+   */
+  public static boolean FEATURE_ENABLED = true;


Since we are late in the development cycle (sorry for this) I suggest to disable this feature by default for now and enable it after the next release. Then you can already use it by setting the corresponding VM argument.

Suggested change

public static boolean FEATURE_ENABLED = true;

public static final boolean FEATURE_ENABLED = Boolean

.parseBoolean(System.getProperty("m2e.process.test.configuration", "true"));

HannesWell · 2024-05-28T20:55:51Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+  private static final Set<String> supportedTypes = new HashSet<>();
+
+  static {
+    supportedTypes.add(MavenRuntimeClasspathProvider.JDT_JUNIT_TEST);
+    supportedTypes.add(MavenRuntimeClasspathProvider.JDT_TESTNG_TEST);
+  }


Suggested change

private static final Set<String> supportedTypes = new HashSet<>();

static {

supportedTypes.add(MavenRuntimeClasspathProvider.JDT_JUNIT_TEST);

supportedTypes.add(MavenRuntimeClasspathProvider.JDT_TESTNG_TEST);

}

private static final Set<String> CONSIDERED_LAUNCH_TYPES = Set.of(MavenRuntimeClasspathProvider.JDT_JUNIT_TEST, MavenRuntimeClasspathProvider.JDT_TESTNG_TEST)

and you should ensure that callers never try to get the value of a null key.

HannesWell · 2024-05-28T20:59:01Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+  private static boolean isSupportedType(String id) {
+    return supportedTypes.contains(id);
+  }


I think this can just be inlined.

Suggested change

private static boolean isSupportedType(String id) {

return supportedTypes.contains(id);

}

private static boolean isSupportedType(String id) {

return id!=null && CONSIDERED_LAUNCH_TYPES .contains(id);

}

HannesWell · 2024-05-28T21:00:04Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+  /**
+   * Launch configuration attribute for the main class
+   */
+  public static final String LAUNCH_CONFIG_MAIN_CLASS = "org.eclipse.jdt.launching.MAIN_TYPE";


Please restrict the visibility as far as possible. And some constants are even unused.

HannesWell · 2024-05-28T21:10:44Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+    private static IMavenProjectFacade getMavenProjectFacade(IJavaProject project) {
+      return MavenPlugin.getMavenProjectRegistry().getProject(project.getProject());
+    }


This can be inlined too.

HannesWell · 2024-05-28T21:20:30Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+     * @param facade the maven project facade
+     * @param monitor the progress monitor
+     */
+    private void loadPrerequisites(ILaunchConfiguration configuration, IMavenProjectFacade facade,


I think leveraging the lifecycle-mapping-metadata is the the better approach.
It would be ideal if we could re-use the existing metadata, but I assume it doesn't fit well in all cases, does it?
If new arguments are added, it should as general as possible and as specific as necessary.

But from what you have described enhancing the https://github.com/codehaus-plexus/plexus-build-api would maybe be another option?

Maybe @laeubi can help here too?

HannesWell · 2024-05-28T21:41:05Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java

+     * @param facade the maven project facade
+     * @param monitor the progress monitor
+     */
+    private void loadPrerequisites(ILaunchConfiguration configuration, IMavenProjectFacade facade,


In order to simplify the review of this big change, I think it would be good to move the resolution of variables created by other plugins into a separate PR.
Would that be ok?

treilhes added 3 commits February 13, 2024 01:48

Surefire and failsafe plugins configuration propagated to launch config

4badb49

Updated release not with new feature

2cf9d3a

Merge remote-tracking branch 'origin/master' into

c2d92c4

feature/surefire_plugins_arguments_to_launch_config

treilhes added 2 commits February 13, 2024 02:43

execute test only if junit/testng supprot is installed

80984b7

Added explicit message to assume statement

0aa6b85

uncommented testng even if not executed

treilhes mentioned this pull request Feb 14, 2024

Test JPMS fails #1676

Closed

treilhes added 3 commits February 14, 2024 16:05

Merge branch 'master' into feature/surefire_plugins_arguments_to_laun…

f6ad7aa

…ch_config

fix some typo

2e8e5fa

bumping feature version 2.4.300 to 2.4.301

cc84a99

treilhes added 5 commits February 18, 2024 10:18

Merge remote-tracking branch 'origin/master' into

40a160e

feature/surefire_plugins_arguments_to_launch_config

Merge remote-tracking branch 'origin/master' into

7f021b8

feature/surefire_plugins_arguments_to_launch_config

Some doc fix mainly for relaunching the devops chain

b7c4074

bump version to 2.3.401

4f15ba7

Merge branch 'eclipse-m2e:master' into feature/surefire_plugins_argum…

80d307f

…ents_to_launch_config

Merge branch 'eclipse-m2e:master' into feature/surefire_plugins_argum…

c5d23f2

…ents_to_launch_config

treilhes added 2 commits April 7, 2024 16:56

Merge branch 'master' into feature/surefire_plugins_arguments_to_laun…

4ceb103

…ch_config

Merge branch 'master' into feature/surefire_plugins_arguments_to_laun…

0763ca9

…ch_config

HannesWell reviewed Apr 25, 2024

View reviewed changes

findMethod does not return an optional anymore, updated my name

b298820

treilhes requested a review from HannesWell April 29, 2024 23:53

treilhes and others added 2 commits April 30, 2024 01:55

Merge branch 'master' into feature/surefire_plugins_arguments_to_laun…

7b7de6c

…ch_config

Merge branch 'master' into feature/surefire_plugins_arguments_to_laun…

9e896d2

…ch_config

HannesWell reviewed May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Surefire/Failsafe plugin configuration propagated to Junit/TestNG launch configuration #1672

Surefire/Failsafe plugin configuration propagated to Junit/TestNG launch configuration #1672

treilhes commented Feb 13, 2024

treilhes commented Feb 13, 2024 •

edited by HannesWell

github-actions bot commented Feb 13, 2024 •

edited

HannesWell commented Feb 15, 2024

treilhes commented Feb 16, 2024

treilhes commented Mar 18, 2024

HannesWell commented Mar 18, 2024

treilhes commented Apr 25, 2024

HannesWell left a comment

HannesWell Apr 25, 2024

treilhes Apr 29, 2024

treilhes May 3, 2024

HannesWell May 28, 2024

HannesWell May 28, 2024

HannesWell left a comment

HannesWell May 25, 2024

HannesWell May 25, 2024

HannesWell May 25, 2024

HannesWell May 28, 2024

HannesWell May 28, 2024

HannesWell May 28, 2024

HannesWell May 28, 2024

HannesWell May 28, 2024

HannesWell May 28, 2024

HannesWell May 28, 2024

	public static boolean FEATURE_ENABLED = true;
	public static final boolean FEATURE_ENABLED = Boolean
	.parseBoolean(System.getProperty("m2e.process.test.configuration", "true"));

Surefire/Failsafe plugin configuration propagated to Junit/TestNG launch configuration #1672

Are you sure you want to change the base?

Surefire/Failsafe plugin configuration propagated to Junit/TestNG launch configuration #1672

Conversation

treilhes commented Feb 13, 2024

treilhes commented Feb 13, 2024 • edited by HannesWell

github-actions bot commented Feb 13, 2024 • edited

Test Results

HannesWell commented Feb 15, 2024

treilhes commented Feb 16, 2024

treilhes commented Mar 18, 2024

HannesWell commented Mar 18, 2024

treilhes commented Apr 25, 2024

HannesWell 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

HannesWell 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

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

treilhes commented Feb 13, 2024 •

edited by HannesWell

github-actions bot commented Feb 13, 2024 •

edited