Skip to content
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

avoid deep pattern for maven-invoker-plugin generated junit files #307

Closed
wants to merge 2 commits into from

Conversation

olamy
Copy link

@olamy olamy commented Feb 24, 2022

No description provided.

@dduportal
Copy link
Contributor

Hello 👋 Could you explain the why/what of this change? It's not clear what problem it is solving?

@@ -115,7 +115,7 @@ def call(Map params = [:]) {
infra.runMaven(mavenOptions, jdk, null, null, addToolEnv)
} finally {
if (!skipTests) {
junit('**/target/surefire-reports/**/*.xml,**/target/failsafe-reports/**/*.xml,**/target/invoker-reports/**/*.xml')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly this change, the junit reports that would be in nested directories (for instance target/surefire-reports/foo/test-1.xml) would not be selected anymore.

Am I correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they are actually nested

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surefire xml files are only generated at the top level directory.

@dduportal
Copy link
Contributor

java.lang.AssertionError at BuildPluginStepTests.test_buildPlugin_with_defaults(BuildPluginStepTests.groovy:130)

do not forget to update the unit test ;)

@timja
Copy link
Member

timja commented Feb 24, 2022

Hello 👋 Could you explain the why/what of this change? It's not clear what problem it is solving?

Context: jenkinsci/plugin-pom#508

Copy link
Contributor

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think this change is correct, I doubt it will solve the problem in jenkinsci/plugin-pom#508 which IIUC was not about target/surefire-reports/some/deep/subdir/TEST-Stuff.xml but about some/deep/subdir/target/surefire-reports/TEST-Stuff.xml. And switching to for example target/surefire-reports/*.xml would cause us to lose legitimate test results from multimodule repos.

@olamy
Copy link
Author

olamy commented Feb 24, 2022

While I think this change is correct, I doubt it will solve the problem in jenkinsci/plugin-pom#508 which IIUC was not about target/surefire-reports/some/deep/subdir/TEST-Stuff.xml but about some/deep/subdir/target/surefire-reports/TEST-Stuff.xml. And switching to for example target/surefire-reports/*.xml would cause us to lose legitimate test results from multimodule repos.

the new pattern **/target/surefire-reports/*.xml still starts with ** so that should not be a problem with multimodule repos.

@jglick
Copy link
Contributor

jglick commented Feb 24, 2022

Right, so (AFAICT) this change should not break anything, but neither is it likely to fix the problem in jenkinsci/plugin-pom#508.

@olamy
Copy link
Author

olamy commented Feb 25, 2022

Even if those deep scanning could be avoided, the PR is not needed anymore.

@olamy olamy closed this Feb 25, 2022
@jglick
Copy link
Contributor

jglick commented Feb 25, 2022

jenkinsci/plugin-pom#508 (comment): maybe would be appropriate to look only one level deep?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants