Skip to content
Permalink
Browse files

[FIXED JENKINS-23263] JUnit reporter moved to a plugin.

  • Loading branch information
jglick committed Aug 11, 2014
1 parent ec22212 commit 16197ea502bc2f370f70b65242b9ccb2f0583372
Showing 471 changed files with 41 additions and 45,006 deletions.
@@ -55,7 +55,9 @@
<!-- Record your changes in the trunk here. -->
<div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
<ul class=image>
<li class=>
<li class="major rfe">
Moved JUnit reporting functionality to a plugin.
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-23263">issue 23263</a>)
</ul>
</div><!--=TRUNK-END=-->

@@ -298,7 +298,8 @@ private void fix(Attributes atts, List<PluginWrapper.Dependency> optionalDepende
new DetachedPlugin("matrix-auth","1.535.*","1.0.2"),
new DetachedPlugin("windows-slaves","1.547.*","1.0"),
new DetachedPlugin("antisamy-markup-formatter","1.553.*","1.0"),
new DetachedPlugin("matrix-project","1.561.*","1.0")
new DetachedPlugin("matrix-project","1.561.*","1.0"),
new DetachedPlugin("junit","1.577.*","1.0")
);

/**
@@ -54,8 +54,6 @@
import hudson.tasks.Builder;
import hudson.tasks.Fingerprinter.FingerprintAction;
import hudson.tasks.Publisher;
import hudson.tasks.test.AbstractTestResultAction;
import hudson.tasks.test.AggregatedTestResultAction;
import hudson.util.*;
import jenkins.model.Jenkins;
import org.kohsuke.stapler.HttpResponse;
@@ -1041,15 +1039,23 @@ public Calendar due() {
/**
* @deprecated Use {@link #getAction(Class)} on {@link AbstractTestResultAction}.
*/
public AbstractTestResultAction getTestResultAction() {
return getAction(AbstractTestResultAction.class);
public Action getTestResultAction() {

This comment has been minimized.

Copy link
@uschindler

uschindler Aug 22, 2014

Hi,
this causes the following bug in Email-Ext plugin (I am running the RC build to fix some other bugs). Now the results are no longer sent by email because of changed signature:

Build step 'Invoke Ant' marked build as failure
[description-setter] Description set: Java: 64bit/jdk1.7.0_67 -XX:-UseCompressedOops -XX:+UseParallelGC
Archiving artifacts
Recording test results
Email was triggered for: Failure - Any
Sending email for trigger: Failure - Any
ERROR: Publisher hudson.plugins.emailext.ExtendedEmailPublisher aborted due to exception
java.lang.NoSuchMethodError: hudson.model.AbstractBuild.getTestResultAction()Lhudson/tasks/test/AbstractTestResultAction;
    at hudson.plugins.emailext.plugins.content.FailedTestsContent.evaluate(FailedTestsContent.java:47)
    at org.jenkinsci.plugins.tokenmacro.DataBoundTokenMacro.evaluate(DataBoundTokenMacro.java:189)
    at org.jenkinsci.plugins.tokenmacro.TokenMacro.expand(TokenMacro.java:182)
    at org.jenkinsci.plugins.tokenmacro.TokenMacro.expandAll(TokenMacro.java:233)
    at hudson.plugins.emailext.plugins.ContentBuilder.transformText(ContentBuilder.java:71)
    at hudson.plugins.emailext.ExtendedEmailPublisher.getContent(ExtendedEmailPublisher.java:597)
    at hudson.plugins.emailext.ExtendedEmailPublisher.createMail(ExtendedEmailPublisher.java:476)
    at hudson.plugins.emailext.ExtendedEmailPublisher.sendMail(ExtendedEmailPublisher.java:290)
    at hudson.plugins.emailext.ExtendedEmailPublisher._perform(ExtendedEmailPublisher.java:281)
    at hudson.plugins.emailext.ExtendedEmailPublisher.perform(ExtendedEmailPublisher.java:233)
    at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
    at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:770)
    at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:734)
    at hudson.model.Build$BuildExecution.cleanUp(Build.java:192)
    at hudson.model.Run.execute(Run.java:1786)
    at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
    at hudson.model.ResourceController.execute(ResourceController.java:89)
    at hudson.model.Executor.run(Executor.java:240)

This comment has been minimized.

Copy link
@uschindler

uschindler Aug 22, 2014

Should I open issue in email-ext plugin for a workaround?

This comment has been minimized.

This comment has been minimized.

Copy link
@slide

slide Aug 22, 2014

Member

Email-ext is built against the LTS, so this won't necessarily be fixed soon .

This comment has been minimized.

Copy link
@uschindler

uschindler Aug 22, 2014

What would be the suggestion to fix?

This comment has been minimized.

Copy link
@slide

slide Aug 22, 2014

Member

I don't know yet.

This comment has been minimized.

Copy link
@jglick

jglick Aug 25, 2014

Author Member

I already did what I think was an exhaustive search for such incompatibilities among @jenkinsci plugins and fixed them all in trunk sources. I did not cut new releases of all patched plugins, however; that is up to the maintainers.

This comment has been minimized.

Copy link
@slide

slide via email Aug 25, 2014

Member

This comment has been minimized.

Copy link
@uschindler

uschindler via email Aug 25, 2014

This comment has been minimized.

Copy link
@slide

slide via email Aug 25, 2014

Member

This comment has been minimized.

Copy link
@jglick

jglick Aug 26, 2014

Author Member

jenkinsci/email-ext-plugin@26b1e1b merely inlining the now-deprecated methods.

Not sure what the comment about the Token Macro plugin was about; as far as I know this need not be touched.

This comment has been minimized.

Copy link
@uschindler

uschindler Aug 26, 2014

Not sure what the comment about the Token Macro plugin was about; as far as I know this need not be touched.

My fault when anlyzing the stack trace! I thought that the token-ext plugin gets the values directly from the failed tests, but its indeed only the email-ext one.

The code should have been written like that since long time (as getTestResultAction is deprecated). If it also works for the LTS release, this is an easy change.

Just one question: In Jenkins Core there are still the deprecated methods. But as their signatures changed, why not remove them completely? The getAction() stuff works much better without any casts and type-safe.

This comment has been minimized.

Copy link
@jglick

jglick Aug 26, 2014

Author Member

Already replied in JENKINS-24395 but to repeat here: the deprecated methods should still work when called from Jelly or (dynamically-typed) Groovy.

try {
return getAction(Jenkins.getInstance().getPluginManager().uberClassLoader.loadClass("hudson.tasks.test.AbstractTestResultAction").asSubclass(Action.class));
} catch (ClassNotFoundException x) {
return null;
}
}

/**
* @deprecated Use {@link #getAction(Class)} on {@link AggregatedTestResultAction}.
*/
public AggregatedTestResultAction getAggregatedTestResultAction() {
return getAction(AggregatedTestResultAction.class);
public Action getAggregatedTestResultAction() {
try {
return getAction(Jenkins.getInstance().getPluginManager().uberClassLoader.loadClass("hudson.tasks.test.AggregatedTestResultAction").asSubclass(Action.class));
} catch (ClassNotFoundException x) {
return null;
}
}

/**
@@ -24,7 +24,6 @@
package hudson.model;

import hudson.Functions;
import hudson.tasks.test.TestResultProjectAction;

/**
* Object that contributes additional information, behaviors, and UIs to {@link ModelObject}
@@ -46,7 +45,7 @@
* it will be displayed as a floating box on the top page of
* the target {@link ModelObject}. (For example, this is how
* the JUnit test result trend shows up in the project top page.
* See {@link TestResultProjectAction}.)
* See {@code TestResultProjectAction}.)
*
* <p>
* On the target {@link ModelObject} page, actions are rendered as an item in the side panel
@@ -26,7 +26,6 @@
import hudson.tasks.BuildStep;
import hudson.tasks.Recorder;
import hudson.tasks.Builder;
import hudson.tasks.junit.JUnitResultArchiver;
import hudson.scm.SCM;
import javax.annotation.Nonnull;

@@ -57,7 +56,7 @@
*
* <h2>Example</h2>
* <p>
* {@link JUnitResultArchiver} provides a good example of how a {@link Recorder} can
* {@code JUnitResultArchiver} provides a good example of how a {@link Recorder} can
* depend on its earlier result.
*
* @author Kohsuke Kawaguchi
@@ -127,9 +126,9 @@ public void report() {
*
* <ol>
* <li>Build #1, #2, and #3 happens around the same time
* <li>Build #3 waits for check point {@link JUnitResultArchiver}
* <li>Build #3 waits for check point {@code JUnitResultArchiver}
* <li>Build #2 aborts before getting to that check point
* <li>Build #1 finally checks in {@link JUnitResultArchiver}
* <li>Build #1 finally checks in {@code JUnitResultArchiver}
* </ol>
*
* <p>

11 comments on commit 16197ea

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev replied Aug 11, 2014

Great!
Thanks a lot for your efforts

@jtnord

This comment has been minimized.

Copy link
Member

jtnord replied Aug 11, 2014

why has the hudson.tasks.tests been moved as well as the hudson.tasks.junit?

Surely the tests is the base classes that concrete implementations need to extend -
Moving the to a separate plugin is like omving abstractBuild from the core.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick replied Aug 11, 2014

why has the hudson.tasks.tests been moved as well as the hudson.tasks.junit?

Because they could be. Plugins defining other publishers, or accessing test results, need merely depend on the new plugin.

As to why these are not in two plugins, well really they should be, but I could not find a way to disentangle them compatibly, so I think we are stuck with them together.

like moving AbstractBuild from the core

That has in fact been proposed.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck replied Aug 11, 2014

need merely depend on the new plugin

"Want to use XUnit? Just install and enable JUnit!"

This really seems weird...

@jtnord

This comment has been minimized.

Copy link
Member

jtnord replied Aug 11, 2014

beacuse they could be

lots of things can be - it does not mean they should be.
hudson.tasks.tests should be decoupled form hudson.tasks.junit - see https://issues.jenkins-ci.org/browse/JENKINS-19898

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev replied Aug 11, 2014

"Want to use XUnit? Just install and enable JUnit!"

Actually, XUnit just converts the data to the JUnit format and then uses the functionality of JUnit publisher.
I'd say such dependency is valid even if it is non-obvious for users

IMHO, the decoupling of test reporting facilities (and their bugs) from the core is worth efforts in any case. 2 plugins would be better, but seems there's no way till Jenkins 2.0.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick replied Aug 11, 2014

@daniel-beck well xunit-plugin already depended on hudson.tasks.junit. Probably it ought to be modified to use only hudson.tasks.test, which ought to be all it needs to render test results, but that is not my business. At any rate since junit becomes a bundled plugin, you would not explicitly install it. (Nor would you even if it were not bundled, since the plugin manager handles that automatically.)

@jtnord w.r.t. JENKINS-19898 yes that helped, but not enough to physically separate them.

lots of things can be

How I wish that were true. Some functionality is just so deeply tangled into the signatures of lower-level classes that it seems impossible to move without introducing serious compatibility problems.

does not mean they should be

We are striving to split everything into an independent plugin that possibly can be. There are many reasons for this, but the foremost is that it allows critical bug fixes to be put in users’ hands far more quickly and safely. For example, if junit-plugin had been split before, people could pick up the fix for JENKINS-23945 (a critical bug for some CloudBees customers) by just updating that plugin, rather than running a custom version of core.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck replied Aug 11, 2014

Actually, XUnit just converts the data to the JUnit format and then uses the functionality of JUnit publisher.

Just a badly chosen example. Still doesn't make sense for plugins building on top of the JUnit independent types.

Nor would you even if it were not bundled, since the plugin manager handles that automatically

But I cannot disable it when I don't want to use it, because it provides infrastructure for other plugins.

There are many reasons for this, but the foremost is that it allows critical bug fixes to be put in users’ hands far more quickly and safely

Making LTS stability irrelevant as everything that could possibly break is in a plugin that has no LTS concept.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev replied Aug 11, 2014

But I cannot disable it when I don't want to use it, because it provides infrastructure for other plugins.

We could create junit-api-plugin and junit-publisher-plugin (just for UI components)

@jtnord

This comment has been minimized.

Copy link
Member

jtnord replied Aug 11, 2014

you can use cucumber as the example instead of xUnit - it requires h.t.tests but not not h.t.junit.
I'm sure there are other instances out there.

Making LTS stability irrelevant as everything that could possibly break is in a plugin that has no LTS concept.

I also worry about the possible future this invents that is "plugin hell" akin to jar hell / dll hell....

@jglick

This comment has been minimized.

Copy link
Member Author

jglick replied Aug 11, 2014

everything that could possibly break is in a plugin

Well, we are a long way from that of course.

that has no LTS concept

Quite true. It would be very useful to create one.

Please sign in to comment.
You can’t perform that action at this time.