-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Surefire/Failsafe plugin configuration propagated to Junit/TestNG launch configuration #1672
Surefire/Failsafe plugin configuration propagated to Junit/TestNG launch configuration #1672
Conversation
Test Results 216 files + 2 216 suites +2 20m 1s ⏱️ + 3m 0s For more details on these failures, see this check. Results for commit 5763fc6. ± Comparison against base commit a4387fc. ♻️ This comment has been updated with latest results. |
@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. |
@HannesWell |
@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). |
@HannesWell Bump ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made another pass over this PR and have a couple of suggestions, mainly about reducing code size.
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/UnitTestSupport.java
Outdated
Show resolved
Hide resolved
85966cd
to
afc83e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@treilhes thanks for your adjustments and your patience.
I applied multiple changes to stream-line this PR a bit but it should not have changed anything from a functional point of view.
With that this looks fine to me and I would like to integrate this into the M2E release for the upcoming Eclipse simultaneous 2024-09 release.
If you are fine with these changes I'll squash them into one commit and submit it.
I just have one minor question below.
boolean isMavenProject = configuration | ||
.getAttribute(IJavaLaunchConfigurationConstants.ATTR_CLASSPATH_PROVIDER, "") | ||
.equals(MavenRuntimeClasspathProvider.MAVEN_CLASSPATH_PROVIDER); | ||
//TODO: why not check the nature?! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be sufficient to just test if the configuration's project has the m2e/maven nature, which is done below?
6aabd05
to
4f57779
Compare
4f57779
to
5763fc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@treilhes thank you for this contribution, that's really a nice extension to m2e and good work.
I'll leave the last question open for now because I would like to have this in the RC1 for the upcoming m2e release that I'm currently preparing.
We can still adjust this later.
Hello @HannesWell, |
Thank you for the clarification. |
Hello @HannesWell, |
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>