Support for evaluating additional test goals #258

imonteroperez · 2020-09-23T16:26:12Z

This PR provides support for evaluating additional test goals as test results.

Context

A new PCT hook to launch integration tests for warnings-ng plugin was introduced in the past. See it here.
Although this PCT hook is able to launch new test types (as a complement to default ones based on surefire:test goal) some additional actions are required to provide support for it as well as for others that extends PluginWithIntegrationTestsHook in the future in order to evaluate properly the results obtained from the PCT.

Problem statement

Before this PR, when PCT is generating its report and some integration tests fail for warnings-ng it setup it as SUCCESS because it was only evaluating results for surefire:test goal.
In addition, detected that if test is skipped, it was marked as failure on test report too (for all test types - including default one based on surefire).
Also, if new integration tests for additional plugins are going to be included in the future, current design is extremely coupled to warnings-ng plugin needs.

Solution and implementation details

Created a new hook inheritance design for PluginWithIntegrationTestsHook that makes feasible to setup new integration test suites and plugins in an easy way, just providing an implementation for two abstract methods that requires which goals and test types your plugin requires in order to execute integration tests.
Status of PCT report is based on the status of the executed tests
Skipped tests are considered as ok instead of failure
Integration tests are included inside PCT report in case it fails (as usual for default ones based in surefire)
Example of output with integration tests failures for warnings-ng:

[INFO] Results:
[INFO] 
[INFO] Executed: 856
[...]    // 417 unit tests launched by surefire:test + 439 integration tests launched by failsafe:integration-test
[INFO] - io.jenkins.plugins.analysis.core.charts.CompositeBuildResultTest.shouldMergeNumberOfFixedIssues
[INFO] - io.jenkins.plugins.analysis.core.charts.CompositeBuildResultTest.shouldMergeNumberOfIssuesByOrigin
[INFO] - io.jenkins.plugins.analysis.core.charts.CompositeBuildResultTest.shouldMergeNumberOfNewIssues
[INFO] - io.jenkins.plugins.analysis.core.charts.CompositeBuildResultTest.shouldMergeNumberOfNewIssuesBySeverity
[INFO] - io.jenkins.plugins.analysis.core.charts.CompositeBuildResultTest.shouldMergeNumberOfTotalIssues
[INFO] - io.jenkins.plugins.analysis.core.charts.CompositeBuildResultTest.shouldMergeTotalNumberOfIssuesBySeverity
[...]
[INFO] - io.jenkins.plugins.analysis.warnings.tasks.TaskScannerTest.shouldScanFileWithWords
[INFO] - io.jenkins.plugins.analysis.warnings.tasks.TaskScannerTest.shouldScanFileWithoutTasks
[INFO] - io.jenkins.plugins.analysis.warnings.tasks.TaskScannerTest.shouldUseDefaults
[INFO] 
[INFO] Failed: 1
[INFO] - io.jenkins.plugins.analysis.warnings.steps.StepsITest.shouldShowWarningsOfGroovyParser

Example of generated report:

<entry>
      <pluginInfos>
        <pluginName>warnings-ng</pluginName>
        <pluginVersion>8.4.1</pluginVersion>
        <pluginUrl>jar:file:/pct/tmp/jenkins.war!/WEB-INF/plugins/warnings-ng.hpi</pluginUrl>
      </pluginInfos>
      <list>
        <compatResult>
          [...]
          <status>TEST_FAILURE</status>
          [...]
          <testDetails class="sorted-set">
                    <string>io.jenkins.plugins.analysis.warnings.steps.StepsITest.shouldShowWarningsOfGroovyParser</string>
          </testDetails>
          [...]          
        </compatResult>
      </list>
</entry>

@raul-arabaolaza @bmunozm @fcojfernandez

raul-arabaolaza

@imonteroperez Can you add a description of what is to be fixed by this PR and a bit of context? I known the reason of the change for other reasons, but that does not apply to any other person but me or @bmunozm that may want to review this.

Long story short, we found that when running the PCT for warnings-ng plugin the results of integration tests of the plugin were not properly recognized by the PCT. Those tests were launched by using maven failsafe plugin and triggered by using this hook

plugin-compat-tester/plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/PluginWithIntegrationTestsHook.java

Line 11 in 378f2f9

    
           public abstract class PluginWithIntegrationTestsHook extends PluginCompatTesterHookBeforeExecution {

I am requesting changes because I disagree with the approach, hook interface is very simple and generic by purpose, you are adding methods to it to cover the needs of one plugin, not to hook into the default PCT lifecycle (checkout, compile, run tests)

If test results are not properly recognized that is something to be done in a particular hook executed after the tests are run, for example with a preexecution one that prevents default execution from the pct or similar approach, but PCT main code should be perfectly unaware of this.

Also I have my doubts if we should even run those integration tests on the PCT, are the commands used to update test dependencies before surefire:test also working for failsafe tests?

raul-arabaolaza · 2020-09-24T12:32:05Z

...pat-tester-model/src/main/java/org/jenkins/tools/test/model/hook/PluginCompatTesterHook.java

+     /**
+      * Provides a list of test types executed under the hook (i.e. hooks used to launch other tests rather than surefire.
+      */
+     default Collection<String> getTestTypes() {


This is a particular need for some plugins, not part (IMO) of any general build cycle process hook mechanism. I do not believe is a good idea to start adding particular methods to cover particular plugins needs to the base interface of the hook mechanism which is aimed to provide a way to override PCT logic for certain plugins. As such it has to be generic, particular needs can be implemented in each individual hook.

For example, what is the purpose of this method in a precompilehook or for a precheckout one?

Added description inside PR and applied your proposal. Thanks a lot! It improved the previous solution design a lot and simplify implementation. Kudos!

…warnings-ng

varyvol · 2020-09-25T13:16:31Z

...-compat-tester/src/main/java/org/jenkins/tools/test/hook/PluginWithIntegrationTestsHook.java

 */
 public abstract class PluginWithIntegrationTestsHook extends PluginCompatTesterHookBeforeExecution {

+    /** Inform about goals to execute integration tests */
+    abstract public Collection<String> getGoals();


Are we sure these are not being used outside? Also having nullability annotations would be nice I think.

Yes, AFAIK seems only used here (see also: https://github.com/search?q=PluginWithIntegrationTestsHook&type=code) Added NonNull annotation too here b692261

varyvol · 2020-09-25T13:18:59Z

plugins-compat-tester/src/main/java/org/jenkins/tools/test/util/ExecutedTestNamesSolver.java

+
+        for (int j = 0; j < nodeList.getLength(); j++) {
+            if (nodeList.item(j).getNodeName().equals("skipped")) {
+                return false;


So if there is one skipped test, this is saying there are no test failures?

Only the specific test that is skipped will not be considered as a failure. Before this PR it was considered as a failure.

recena · 2020-09-25T17:52:10Z

...-compat-tester/src/main/java/org/jenkins/tools/test/hook/PluginWithIntegrationTestsHook.java

        return info;
    }
-
+    


Extra spaces 😄

Thanks! fixed in a917eb1

imonteroperez · 2020-10-06T11:39:06Z

Seems PR builder failures are same than master https://ci.jenkins.io/blue/organizations/jenkins/jenkinsci-libraries%2Fplugin-compat-tester/detail/master/147/pipeline, so unrelated with this PR

raul-arabaolaza · 2020-10-06T13:25:52Z

Enough approvals or time for others to block this, so merging

imonteroperez added 5 commits September 23, 2020 18:25

Support for evaluating additional test goals

a9d87b4

Adapt tags for warnings-ng

a4c2773

Check status

6086715

Remove confussing comment

5c7d3ef

Dealing with test status properly

1fe25fb

raul-arabaolaza requested changes Sep 24, 2020

View reviewed changes

imonteroperez added 10 commits September 24, 2020 15:01

Support for skipped

0638ccd

Address feedback from raul-arabaolaza

0f0f354

Complete previous commit

827497d

Simplify hooks management

87c31a3

Restored PluginCompatTesterHooks, no changes needed there

0ab39cd

Restored PluginCompatTesterHook, no changes needed there

3041c89

Remove unused imports

82dd955

Remove some empty spaces

dff74d9

Merge remote-tracking branch 'upstream/master' into integration-test-…

9cb9872

…warnings-ng

Unit test

1d409e2

imonteroperez marked this pull request as ready for review September 25, 2020 11:39

imonteroperez requested a review from raul-arabaolaza September 25, 2020 11:39

varyvol reviewed Sep 25, 2020

View reviewed changes

Force non-null values

b692261

imonteroperez requested a review from varyvol September 25, 2020 15:28

recena reviewed Sep 25, 2020

View reviewed changes

Removed extra space

a917eb1

raul-arabaolaza approved these changes Oct 1, 2020

View reviewed changes

bmunozm approved these changes Oct 6, 2020

View reviewed changes

raul-arabaolaza merged commit 38ee180 into jenkinsci:master Oct 6, 2020

imonteroperez mentioned this pull request Feb 8, 2021

Bugfix on retrieving plugin test types #271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for evaluating additional test goals #258

Support for evaluating additional test goals #258

imonteroperez commented Sep 23, 2020 •

edited

Loading

raul-arabaolaza left a comment •

edited

Loading

raul-arabaolaza Sep 24, 2020

imonteroperez Sep 25, 2020

varyvol Sep 25, 2020

imonteroperez Sep 25, 2020

varyvol Sep 25, 2020

imonteroperez Sep 25, 2020

recena Sep 25, 2020

imonteroperez Sep 28, 2020

imonteroperez commented Oct 6, 2020

raul-arabaolaza commented Oct 6, 2020

Support for evaluating additional test goals #258

Support for evaluating additional test goals #258

Conversation

imonteroperez commented Sep 23, 2020 • edited Loading

Context

Problem statement

Solution and implementation details

raul-arabaolaza left a comment • edited Loading

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

imonteroperez commented Oct 6, 2020

raul-arabaolaza commented Oct 6, 2020

imonteroperez commented Sep 23, 2020 •

edited

Loading

raul-arabaolaza left a comment •

edited

Loading