Jenkins-2 part of JENKINS-29649 + Fixed JENKINS-37623, JENKINS-38149 #14

anenviousguest · 2016-08-23T07:53:02Z

Thanks for merging #13. Turns out there was missing part there, namely the fix for Jenkins 2 workflow jobs. Would appreciate if you could review it.

Also i put in here the fix for JENKINS-37623 , as the it and the change above modify same file.

TL;DR

For Jenkins 2 workflow jobs, it might happen that JUnit and TAP publishers wrongly use TAP-only (or JUnit-only) results to draw their graphs. Added test case to detect that.

Also, when build history contained runs which failed before any TAP results were published, NullPointerException was occurring during the tooltip generation. The graph was still rendered, but the image map was missing.

Why it happens (JENKINS-29649 part)

In JUnit's code inside TestResultProjectAction, they try to fetch last test result action of specific type before drawing a plot:

As one may see here: https://github.com/jenkinsci/junit-plugin/blob/master/src/main/java/hudson/tasks/test/TestResultProjectAction.java#L93 - the action they search for is supposed to be inherited from AbstractTestResultAction.

And because TapTestResultAction is also derived from AbstractTestResultAction, it may (depending on the action order) be unexpectedly used to plot JUnit graphs, too. Vice versa might also be the case.

This PR's content

The fix for Jenkins-2 part of JENKINS-29649 + test case which is red when fix is not applied. I basically decoupled TapTestResultAction from JUnit and migrated it and its surroundings to use generic Run instead of AbstractProject.
The fix for JENKINS-37623 + test case which is red when fix is not applied.
Because of the test cases above, I had to add test dependency and increase the minimum Jenkins version plugin is built against (still Jenkins 1.xxx though).
The new Jenkins parent seem to bring in InjectedTest which performs basic lint checks on *.jelly files. Had to add Jelly escaping directive to make this test happy.
Few IDE warnings fixed.

… StringBuilder.

…grating AbstractBuild -> Run to better support Jenkins 2 workflow jobs.

kinow · 2016-08-23T08:53:02Z

👍 thanks @anenviousguest will need a few days, as I'm going through a few other plug-ins and pull requests to cut more releases. Feel free to update the pull request in the meantime.

anenviousguest · 2016-09-03T07:00:27Z

hi @kinow
Have you by any chance had a time to look at this PR? Would very much appreciate if you could give your feedback or merge it.
Thanks!

anenviousguest · 2016-09-07T15:48:26Z

hi Bruno @kinow
Let me kindly bump the thread here. Would that be reasonable to assume that you would have time to review this in a foreseeable future?
Thanks!

kinow · 2016-09-07T19:54:22Z

Hi @anenviousguest

Ack :-) Promise that as soon as I'm done with https://github.com/jenkinsci/tap-plugin/tree/performance-fix for the performance issues.

Normally I make little progress, and proceed with other issues. But this time I really want to fix these performance issues. Once it's done, will continue merging the other PR's and working on issues. Yours will be the first PR that I will look into, and with RERO motto in mind, we might have it released shortly after the PR has been merged.

Sorry for the delay

oleg-nenashev

Generally the code should address the issue. There are several improvement areas, minor fixes are required.

I would also recommend to bump major version of the plugin since there are incompatible changes.

CC @olofk, whose issue will be likely solved by this pull request

oleg-nenashev · 2016-09-19T09:33:32Z

pom.xml

@@ -20,7 +20,7 @@
 	<parent>
 		<groupId>org.jenkins-ci.plugins</groupId>
 		<artifactId>plugin</artifactId>
-		<version>1.580.3</version><!-- which version of Jenkins is this plugin 
+		<version>1.637</version><!-- which version of Jenkins is this plugin


🐜 for selecting non-LTS version.
What was the reason for it?

oleg-nenashev · 2016-09-19T09:37:01Z

src/main/java/org/tap4j/plugin/TapTestResultAction.java

@@ -47,26 +50,31 @@
 * @since 0.1
 */
 public class TapTestResultAction
-        extends AbstractTestResultAction<AbstractTestResultAction<?>>
-        implements StaplerProxy, SimpleBuildStep.LastBuildAction {
+        implements StaplerProxy, SimpleBuildStep.LastBuildAction, HealthReportingAction, RunAction2 {


Removal of AbstractTestResultAction inheritance smells like a compatibility issue, but seems it's reasonable in this case

was exactly causing the issue, please see "Why it happens (JENKINS-29649 part)" in the description above.

Yes, I understand why this change is required. The problem is that...

The class cannot be longer casted to AbstractTestResultAction

Usage of non-implemented methods from AbstractTestResultAction will cause LinkageError. E.g. findPreviousCorresponding()

So I would vouch for keeping AbstractTestResultAction inheritance. If you update JUnit plugin dependency to the latest version (1.18), you may see it's interfaces have been adapted to make this class compatible with Pipeline Plugin

hi @oleg-nenashev ,
I've just tested, and upgrading to junit-plugin 1.18 doesn't fix JENKINS-29649 in this plugin.

Everything described in the "why it happens..." at the very beginning of this PR is still relevant to version 1.18.

That is, when a build contains multiple test result actions derived from AbstractTestResultAction, the return value of TestResultProjectAction.getLastTestResultAction, as seen here may depend on the order in which actions were saved, because it searches for actions of a base class.

Consequently, here and here wrong action might be used to draw the graph.

oleg-nenashev · 2016-09-19T09:39:57Z

src/main/java/org/tap4j/plugin/TapTestResultAction.java

+
+    public transient Run<?,?> run;
+    @Deprecated
+    public transient AbstractBuild<?,?> owner;


Well, the binary compatibility is broken in any case

why? these have already been there, just inherited from AbstractTestResultAction. Now that it was removed from inheritance hierarchy, i placed these fields right here.

oleg-nenashev · 2016-09-19T09:41:15Z

src/main/java/org/tap4j/plugin/TapTestResultAction.java

    /**
-     * @param owner
+     * @param r


🐛 It changes param name to the invalid one.
Having of param declarations without description is also a bad practice. Javadoc linter in Java8 will grumble about that.

oleg-nenashev · 2016-09-19T09:42:13Z

src/main/java/org/tap4j/plugin/TapTestResultAction.java

    @Override
-    @Exported(visibility = 2)
-    public int getTotalCount() {


Binary compat as well

i guess it's a diff display issue. The lines highlighted were in the unused descendant class.
getTotalCount is still there in TapTestResultAction

oleg-nenashev · 2016-09-19T09:43:33Z

src/main/java/org/tap4j/plugin/model/TapTestResultResult.java

+     *
+     * @return associated TAP test result action object
+     */
+    private TapTestResultAction getTestResultActionDiverged() {


Good practice: CheckForNull annotation

oleg-nenashev · 2016-09-19T09:48:57Z

The PR also addresses JENKINS-38149

anenviousguest · 2016-09-20T06:14:04Z

@oleg-nenashev thanks for reviewing this. I'll try to accomodate changes where it deems reasonable.

@checkfornull

* Addressing wrong parameter name in Javadoc. * @checkfornull usage. * Depending on LTS parent version in pom.xml

anenviousguest

@oleg-nenashev Thanks for review.

I guess i've addressed all the points, please see comments.

In particular, i think binary compatibility is preserved.

anenviousguest · 2016-09-20T06:37:33Z

src/main/java/org/tap4j/plugin/TapTestResultAction.java

    @Override
-    @Exported(visibility = 2)
-    public int getTotalCount() {


i guess it's a diff display issue. The lines highlighted were in the unused descendant class.
getTotalCount is still there in TapTestResultAction

anenviousguest · 2016-09-20T06:39:42Z

src/main/java/org/tap4j/plugin/model/TapTestResultResult.java

+     *
+     * @return associated TAP test result action object
+     */
+    private TapTestResultAction getTestResultActionDiverged() {


anenviousguest · 2016-09-20T06:40:19Z

src/main/java/org/tap4j/plugin/TapTestResultAction.java

@@ -47,26 +50,31 @@
 * @since 0.1
 */
 public class TapTestResultAction
-        extends AbstractTestResultAction<AbstractTestResultAction<?>>
-        implements StaplerProxy, SimpleBuildStep.LastBuildAction {
+        implements StaplerProxy, SimpleBuildStep.LastBuildAction, HealthReportingAction, RunAction2 {


was exactly causing the issue, please see "Why it happens (JENKINS-29649 part)" in the description above.

anenviousguest · 2016-09-20T06:42:50Z

src/main/java/org/tap4j/plugin/TapTestResultAction.java

+
+    public transient Run<?,?> run;
+    @Deprecated
+    public transient AbstractBuild<?,?> owner;


why? these have already been there, just inherited from AbstractTestResultAction. Now that it was removed from inheritance hierarchy, i placed these fields right here.

anenviousguest · 2016-09-20T06:42:57Z

src/main/java/org/tap4j/plugin/TapTestResultAction.java

    /**
-     * @param owner
+     * @param r


anenviousguest · 2016-09-20T06:50:51Z

pom.xml

@@ -20,7 +20,7 @@
 	<parent>
 		<groupId>org.jenkins-ci.plugins</groupId>
 		<artifactId>plugin</artifactId>
-		<version>1.580.3</version><!-- which version of Jenkins is this plugin 
+		<version>1.637</version><!-- which version of Jenkins is this plugin


anenviousguest · 2016-09-20T06:55:13Z

@kinow @oleg-nenashev does that look better now? Thanks.

oleg-nenashev · 2016-09-20T11:44:10Z

@anenviousguest Provided feedback. I think that the inheritance from AbstractTestResultAction should be retained. Upgrade to one of the latest JUnit plugin versions should help with it

anenviousguest · 2016-09-23T06:35:19Z

@oleg-nenashev thanks! See my reply in the above thread.

oleg-nenashev · 2016-09-23T08:33:06Z

Well, we need to think about potential solutions. Now particular Jenkins features may stop working for TAP Plugin (e.g. aggregation of downstream test results in Freestyle jobs).

P.S: We've tested the change on a Pipeline-only instance, and the plugin is working as we need with this patch.

kinow · 2016-09-23T09:16:40Z

Will try to catch up with the review/comments this weekend :)

anenviousguest · 2016-09-24T05:41:54Z

Hi @oleg-nenashev

I'm struggling to find any immediate elegant solution to work around this check in junit-plugin.

I have no idea if JUnit plugin was meant to provide a base API for test-alike plugins, but if it was, it's probably better to think of fixing the check on its side, in a long term.

oleg-nenashev · 2016-09-24T09:29:24Z

Yes, I should have read the text carefully from the beginning.

The JUnit Issue

I have no idea if JUnit plugin was meant to provide a base API for test-alike plugins, but if it was, it's probably better to think of fixing the check on its side, in a long term.

JUnit plugin has been decoupled from the Jenkins core at some point. So yes, it's actually a kind of API plugin for all test results then just a JUnit plugin.

And because TapTestResultAction is also derived from AbstractTestResultAction, it may (depending on the action order) be unexpectedly used to plot JUnit graphs, too. Vice versa might also be the case.

It is a case.

How to solve it?

Well, it should be solved on the JUnit plugin level. Just because of that: https://github.com/search?q=org%3Ajenkinsci+%22extends+AbstractTestResultAction%22&type=Code

I would just ignore this issue by now. I'll see if I can fix it on the JUnit plugin side by merging results in the plot (or by filtering them)

anenviousguest · 2016-09-27T15:04:09Z

hi @oleg-nenashev

I guess i know what type of fix it might be on junit-plugin side. Should I just submit a PR there and if it's merged, revise this one?

Thanks.

oleg-nenashev · 2016-09-27T15:38:52Z

@anenviousguest Up to you. I think we can go forward with this PR even without waiting for JUnit stuff to be integrated and released

alappe · 2016-10-31T15:10:28Z

Any news here? A fix would be deeply appreciated…

kinow · 2016-10-31T19:11:17Z

@alappe lurking in the discussion between @oleg-nenashev and @anenviousguest :-) happy to review/merge whenever they confirm it's ready

oleg-nenashev · 2016-10-31T19:32:29Z

I think it's ready. The fix is required on the JUnit plugin. Merge of this PR does not increase the technical debt

kinow · 2016-10-31T19:40:53Z

Can we release after merging it @oleg-nenashev ? Or would it be necessary to wait for a merge and release in JUnit plugin first?

oleg-nenashev · 2016-10-31T21:07:08Z

no need in waiting for JUnit plugin IMHO. Just 🚢 🇮🇹 :)

mkhan446 · 2016-11-28T16:56:24Z

Any updates on this pull request? I'm encountering JENKINS-38149 and would love to have it resolved... :)

kinow · 2016-11-28T19:23:59Z

Sorry @mkhan446 got a bit busy with active-choices-plugin, had to digress for a while, but also want to get this merged and released soon. Just give me a few more days and I will sort out and prepare a release to the experimental update center, so others can test it.

anenviousguest · 2016-11-28T21:34:29Z

@oleg-nenashev : thanks!
@kinow : i would very much appreciate if you could merge this, indeed :-) I've been running this fix over the past few months, and it seems to be working, just like @oleg-nenashev mentioned above, too.
Thanks guys for your cooperation!

@oleg-nenashev

… from different plugin. Discussion thread with @oleg-nenashev: * jenkinsci/tap-plugin#14 (comment) * jenkinsci/tap-plugin#14 (comment)

kinow · 2016-11-29T09:58:47Z

Merged. Merge conflicts fixed manually, and all tests passed after that as well. Re-read the issue history, and the only concern is regarding previous behavior that may change after this change. Will push to experimental update center in a few minutes for now, and will play with the new plug-in before cutting a release, likely by the mid or end of December. Any feedback from the version in the experimental update center will definitely help! Thanks @anenviousguest and @oleg-nenashev !

jglick · 2016-12-05T15:36:38Z

@kinow please also amend jenkinsci/pipeline-plugin#403 to indicate the status of the release (fix version).

Vladislav Ponomarev added 5 commits August 22, 2016 17:03

Unused class removed.

676dd48

IDE warnings: variable never null, no string concatenation when using…

349916a

… StringBuilder.

Added red testcase for JENKINS-29649 + Jenkins2 pipeline job.

170bfce

Added Jelly escaping directive to make injected test happy.

32d4a1c

Fixing Jenkins 2 part of JENKINS-29649. Making the testcase green, mi…

2fbcc63

…grating AbstractBuild -> Run to better support Jenkins 2 workflow jobs.

anenviousguest force-pushed the master branch from 36bc454 to 2fbcc63 Compare August 23, 2016 08:27

anenviousguest changed the title ~~Jenkins-2 part of JENKINS-29649. JUnit and TAP publishers were impacting each other on Jenkins 2 workflow job page~~ Jenkins-2 part of JENKINS-29649 + Fixed JENKINS-37623 Aug 23, 2016

Vladislav Ponomarev added 2 commits September 10, 2016 23:44

Red testcase for JENKINS-37623

a919323

[FIXED JENKINS-37623] Tolerate failed builds when generating tooltips.

6cde4f7

anenviousguest force-pushed the master branch from b906d7f to 6cde4f7 Compare September 10, 2016 21:45

oleg-nenashev requested changes Sep 19, 2016

View reviewed changes

oleg-nenashev changed the title ~~Jenkins-2 part of JENKINS-29649 + Fixed JENKINS-37623~~ Jenkins-2 part of JENKINS-29649 + Fixed JENKINS-37623, JENKINS-38149 Sep 19, 2016

Comments to PR:

e609a9b

* Addressing wrong parameter name in Javadoc. * @checkfornull usage. * Depending on LTS parent version in pom.xml

anenviousguest commented Sep 20, 2016

View reviewed changes

oleg-nenashev mentioned this pull request Sep 21, 2016

TAP Plugin needs update to a version with Fixed Pipeline snapshot librecores/librecores-ci-jenkins-server#16

Open

anenviousguest mentioned this pull request Nov 28, 2016

JENKINS-29649-related: AbstractTestResultAction may inadvertently pick action from different plugin jenkinsci/junit-plugin#59

Closed

kinow merged commit e609a9b into jenkinsci:master Nov 29, 2016

Jenkins-2 part of JENKINS-29649 + Fixed JENKINS-37623, JENKINS-38149 #14

Jenkins-2 part of JENKINS-29649 + Fixed JENKINS-37623, JENKINS-38149 #14

Conversation

anenviousguest commented Aug 23, 2016 • edited Loading

TL;DR

Why it happens (JENKINS-29649 part)

This PR's content

kinow commented Aug 23, 2016

anenviousguest commented Sep 3, 2016

anenviousguest commented Sep 7, 2016

kinow commented Sep 7, 2016

oleg-nenashev left a 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

anenviousguest Sep 20, 2016 • 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

anenviousguest Sep 20, 2016 • 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

anenviousguest Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Sep 19, 2016

anenviousguest commented Sep 20, 2016

anenviousguest left a comment

Choose a reason for hiding this comment

anenviousguest Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anenviousguest Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

anenviousguest Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anenviousguest commented Sep 20, 2016

oleg-nenashev commented Sep 20, 2016

anenviousguest commented Sep 23, 2016

oleg-nenashev commented Sep 23, 2016

kinow commented Sep 23, 2016

anenviousguest commented Sep 24, 2016

oleg-nenashev commented Sep 24, 2016

The JUnit Issue

How to solve it?

anenviousguest commented Sep 27, 2016

oleg-nenashev commented Sep 27, 2016

alappe commented Oct 31, 2016

kinow commented Oct 31, 2016 • edited Loading

oleg-nenashev commented Oct 31, 2016

kinow commented Oct 31, 2016

oleg-nenashev commented Oct 31, 2016

mkhan446 commented Nov 28, 2016

kinow commented Nov 28, 2016

anenviousguest commented Nov 28, 2016

kinow commented Nov 29, 2016

jglick commented Dec 5, 2016

anenviousguest commented Aug 23, 2016 •

edited

Loading

anenviousguest Sep 20, 2016 •

edited

Loading

anenviousguest Sep 20, 2016 •

edited

Loading

anenviousguest Sep 20, 2016 •

edited

Loading

anenviousguest Sep 20, 2016 •

edited

Loading

anenviousguest Sep 20, 2016 •

edited

Loading

anenviousguest Sep 20, 2016 •

edited

Loading

kinow commented Oct 31, 2016 •

edited

Loading