Always collect test results after running the build in buildPlugin() and buildPluginWithGradle()

[darxriggs]

This should especially fix test results collection for Gradle, where it's not possible to set test.ignoreFailures from the commandline.

I put this for discussion here as I am not 100% sure if it doesn't brake something in the case of errors. E.g. that archiving is triggered but not working as expected.

[oleg-nenashev]

I am not sur ewhether it is still actual, but there is a massive merge conflict we would need to address

[darxriggs]

I just fixed the merge conflict. It has been that massive because the indentation changed.

The pull request can be more easily reviewed if ignoring whitespace changes. Only a try/finally was added around the "Build" and "Archive" stages.

[oleg-nenashev]

Ideally we should detach Gradle build to a separate Pipeline step. There are too many things which are not really compatible with Gradle in the new flow, e.g. Jenkins version specifications. It would be better to have a clean buildGradlePlugin() implementation which focuses on https://github.com/jenkinsci/gradle-jpi-plugin. CC @sghill

If someone is interested to migrate Gradle plugins, I am happy to help with implementing the change

[oleg-nenashev]

FTR I tried to do some refactoring in #21 to enable parallelized builds. Some code can be reused, once we solve the autodiscovery problem (requires extra node() stage in the linked PR)

[sghill]

I don't have a lot of context here, but if this is the original problem:

it's not possible to set test.ignoreFailures from the commandline.

I think you can do this with an initscript that takes a property from the command line. Something like this works:

// init.gradle
allprojects {
    tasks.withType(Test).configureEach {
        it.ignoreFailures = rootProject.findProperty('jenkins.ignoreFailures') ?: false
    }   
}

Running a build with a failing test:

$ ./gradlew -I init.gradle check
> Task :test FAILED
FailsTest > shouldFail FAILED
    java.lang.AssertionError at FailsTest.java:4
1 test completed, 1 failed
[omitted]
BUILD FAILED in 997ms

Same build completed successfully with the property set:

$ ./gradlew -I init.gradle check -Pjenkins.ignoreFailures=true
> Task :test
FailsTest > shouldFail FAILED
    java.lang.AssertionError at FailsTest.java:4
1 test completed, 1 failed
[omitted]
BUILD SUCCESSFUL in 893ms

Ideally we should detach Gradle build to a separate Pipeline step

This seems reasonable to me.

There are too many things which are not really compatible with Gradle in the new flow, e.g. Jenkins version specifications.

Is there a listing of these somewhere I could read through?

[darxriggs]

@sghill Yes the original problem was about

it's not possible to set test.ignoreFailures from the commandline.

...and due to it no test results beeing collected on failure.
Therefore I wrapped the stages in a try/finallyconstruct.

@oleg-nenashev I don't have the capacity to extract the Gradle build into a separate build step as suggested by you. This pull request is the only thing I can provide for now.

[oleg-nenashev]

#104

[v1v]

For some reason, I missed this particular PR and I raised another one, which includes the UTs:

• feat: archive tests even when the build failed #113
  I'm happy to close the above one for the benefit of this one and you can potentially cherry-pick the UTs changes.

[v1v]

Not anymore with this PR:

Suggested change

// TODO do this in a finally-block so we capture all test results even if one branch aborts early

[oleg-nenashev]

Sorry for not handling it. Too many things here and there :(

[darxriggs]

@sghill Based on new information from @wolfs I would even go without test.ignoreFailures now because it is not a good idea with respect to the Gradle build cache - see jenkinsci/gradle-plugin#93 (comment).

[oleg-nenashev]

Maybe you want to set a flag to prevent the Pipeline from failing on missing reports.

[darxriggs]

@v1v You could also follow up on this in your pull request because you already updated the tests.

I think the main question that should be answered is, if the finally block should contain the whole "Archive" stage or only collecting the different test results (JUnit, FindBugs, Checkstyle).

[darxriggs]

@oleg-nenashev I guess you are talking about allowEmptyResults (https://jenkins.io/doc/pipeline/steps/junit/). I would not want to add another option because there are already quite a lot. I think the existing skipTests option should already cover the aspect of not having test reports. It can be read as "do not run tests" and/or "do not expect test reports".

[oleg-nenashev]

I agree with the patch. NNote that it will cause regressions in Gradle-based components which have not yet switched to buildPluginWithGradle(). Should we finish the migration first?

[oleg-nenashev]

#113
I'm happy to close the above one for the benefit of this one and you can potentially cherry-pick the UTs changes.

Should we also add tests from #113 here? Or do we prefer to do it in a follow-up

[darxriggs]

NNote that it will cause regressions in Gradle-based components which have not yet switched to buildPluginWithGradle()

What kind of regressions do you think of? Maybe I am missing something. The intention was that the new try/finally mechanism always tries to collect the test results, even in case of a failing build or failing tests. So for Gradle-based plugins, if something has been failing before it would still fail, and if nothing has been failing it would still not fail.

[darxriggs]

Should we also add tests from #113 here? Or do we prefer to do it in a follow-up

I will have a look, now that I am more familiar with these kind of tests.

[oleg-nenashev]

What kind of regressions do you think of?

No worries, I just misread the code. My apologies

[oleg-nenashev]

I am fine with merging it as is, we can do tests in a follow-up if you prefer so @darxriggs @v1v

[darxriggs]

@oleg-nenashev Then please already merge it.

I will have to prepare 1 or 2 other pull requests to get the tooling ready for the tests.