-
Notifications
You must be signed in to change notification settings - Fork 53
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
Fix opt-in for Maven Failsafe plugin #462
Conversation
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.
OK as is - comments for consideration
* Custom execution hook for plugins whose parent is {@code org.jvnet.hudson.plugins:analysis-pom}. | ||
* These plugins use Maven Failsafe Plugin in their test suites. | ||
*/ | ||
public class AnalysisPomExecutionHook extends PluginWithFailsafeIntegrationTestsHook { |
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.
CloudBees' has no internal users of PluginWithFailsafeIntegrationTestsHook
(nee PluginWithFailsafeIntegrationTestsHook
)
And as implemented PluginWithFailsafeIntegrationTestsHook
is incorrect as to be generic it should be executing pre-integration-test
then integration-test
and finally post-integration-test
.
the analysis-pom
's configuration of failsafe does not follow this generic nature as it is not spinning anything up.
Perhaps it is worth just removing PluginWithFailsafeIntegrationTestsHook
and then inlining getGoals
here in a way that is specific to Ulli's usage.
public class AnalysisPomExecutionHook extends PluginWithFailsafeIntegrationTestsHook { | |
public class AnalysisPomExecutionHook extends PluginWithIntegrationTestsHook { |
return "io.jenkins.plugins".equals(data.groupId) | ||
&& ARTIFACT_IDS.contains(data.artifactId) | ||
&& "hpi".equals(data.getPackaging()); | ||
} |
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.
} | |
} | |
@Override | |
public Collection<String> getGoals() { | |
return List.of("failsafe:integration-test"); | |
} |
I am here to make it better, not to make it perfect. I am not going to do what was suggested. |
Fixes #461.
PluginWithFailsafeIntegrationTestsHook
was meant to be a way for plugins to opt-in to having their Maven Failsafe plugin tests executed, but the class is not marked asabstract
. Sinceplugin-compat-tester/src/main/java/org/jenkins/tools/test/model/hook/PluginCompatTesterHooks.java
Line 186 in 0838e05
PluginWithFailsafeIntegrationTestsHook
gets executed unconditionally, resulting in workarounds like https://github.com/jenkinsci/bom/blob/c72362731c98313189c04025c3b7bc181da4c121/pct.sh#L22-L25. This PR solves the problem by markingPluginWithFailsafeIntegrationTestsHook
as abstract, which allows the abovementioned workaround to be removed. That, in turn, exposed the fact that Dr Ullrich Hafner's plugins run their tests using a combination of Surefire and Failsafe, so to ensure the status quo is preserved I added a new hook to explicitly opt in to Failsafe for those plugins. And with that, jenkinsci/bom#1764 now passes.