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

[JENKINS-27120] Adding Workflow support for JaCoCo publisher #63

Closed
wants to merge 4 commits into from
Closed
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

975 pom.xml

Large diffs are not rendered by default.

@@ -9,6 +9,9 @@
import java.util.LinkedHashMap;
import java.util.Map;

import hudson.model.Run;
import hudson.model.TaskListener;
import jenkins.model.RunAction2;
import org.jacoco.core.analysis.IBundleCoverage;
import org.jvnet.localizer.Localizable;
import org.kohsuke.stapler.StaplerProxy;
@@ -33,9 +36,9 @@
* @author Jonathan Fuerth
* @author Ognjen Bubalo
*/
public final class JacocoBuildAction extends CoverageObject<JacocoBuildAction> implements HealthReportingAction, StaplerProxy, Serializable {
public final class JacocoBuildAction extends CoverageObject<JacocoBuildAction> implements HealthReportingAction, StaplerProxy, Serializable, RunAction2 {

public final AbstractBuild<?,?> owner;
public transient Run<?,?> owner;

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

this is a backwards incompatable change. Not that I am trying to suggest a BCT transform

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

BCT? I am not sure that this is a breakwards incompatible change. AbstractBuild extends from Run so any AbstractBuild is also a Run.
I know that since it is public everyone could access it, but for what I see this was only used in one test.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 22, 2015

Member

The problem is that existing (already compiled) code may not be able to deal with the type change. I'm not sure in the case of a field though @jtnord , do you know for sure?

BCT is the bytecode compatibility transformer.

This comment has been minimized.

Copy link
@amuniz

amuniz Dec 22, 2015

Member

It is, and it's not only the field but the getOwner method.

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

but a Run is not always an AbstractBuild so anyone expecting this to be an AbstractBuild may now blow up.
This is regardless of any binding that may or not have broken (I think the variable will actually be ok - but the type changing is not). Why this needs to be public baffles me (but that is a different issue)


@Deprecated public transient AbstractBuild<?,?> build;

@@ -45,11 +48,6 @@
private final String[] inclusions;
private final String[] exclusions;

/**
* Non-null if the coverage has pass/fail rules.
*/
private final Rule rule;

/**
* The thresholds that applied when this build was built.
* TODO: add ability to trend thresholds on the graph
@@ -59,23 +57,21 @@
/**
*
* @param owner
* @param rule
* @param ratios
* The available coverage ratios in the report. Null is treated
* the same as an empty map.
* @param thresholds
*/
public JacocoBuildAction(AbstractBuild<?,?> owner, Rule rule,
public JacocoBuildAction(Run<?,?> owner,
Map<CoverageElement.Type, Coverage> ratios,
JacocoHealthReportThresholds thresholds, BuildListener listener, String[] inclusions, String[] exclusions) {
JacocoHealthReportThresholds thresholds, TaskListener listener, String[] inclusions, String[] exclusions) {
logger = listener.getLogger();

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 22, 2015

Member

Incompatible change.

if (ratios == null) {
ratios = Collections.emptyMap();
}
this.inclusions = inclusions;
this.exclusions = exclusions;
this.owner = owner;
this.rule = rule;
this.clazz = getOrCreateRatio(ratios, CoverageElement.Type.CLASS);
this.method = getOrCreateRatio(ratios, CoverageElement.Type.METHOD);
this.line = getOrCreateRatio(ratios, CoverageElement.Type.LINE);
@@ -201,12 +197,12 @@ public Object getTarget() {
}

@Override
public AbstractBuild<?,?> getBuild() {
public Run<?,?> getBuild() {

This comment has been minimized.

Copy link
@amuniz

amuniz Dec 22, 2015

Member

Backward incompatibility here. You should use @WithBridgeMethods, this is an example.

This comment has been minimized.

Copy link
@amuniz

amuniz Dec 22, 2015

Member

However, it does not seem to be used outside, but who knows.

return owner;
}

public JacocoReportDir getJacocoReport() {
return new JacocoReportDir(owner);
return new JacocoReportDir(owner.getRootDir());
}

/**
@@ -274,8 +270,8 @@ public JacocoBuildAction getPreviousResult() {
/**
* Gets the previous {@link JacocoBuildAction} of the given build.
*/
/*package*/ static JacocoBuildAction getPreviousResult(AbstractBuild<?,?> start) {
AbstractBuild<?,?> b = start;
/*package*/ static JacocoBuildAction getPreviousResult(Run<?,?> start) {
Run<?,?> b = start;
while(true) {
b = b.getPreviousBuild();
if(b==null) {
@@ -297,12 +293,12 @@ public JacocoBuildAction getPreviousResult() {
* @throws IOException
* if failed to parse the file.
*/
public static JacocoBuildAction load(AbstractBuild<?,?> owner, Rule rule, JacocoHealthReportThresholds thresholds, BuildListener listener, JacocoReportDir layout, String[] includes, String[] excludes) throws IOException {
public static JacocoBuildAction load(Run<?,?> owner, JacocoHealthReportThresholds thresholds, TaskListener listener, JacocoReportDir layout, String[] includes, String[] excludes) throws IOException {

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

API breaking change.

This comment has been minimized.

Copy link
@amuniz

amuniz Dec 22, 2015

Member

It seems it's not used outside this plugin. It's an acceptable change IMO. Adding the Utils.isOverridden dance makes the code dirty, I would try to avoid it when possible.

//PrintStream logger = listener.getLogger();
Map<CoverageElement.Type,Coverage> ratios = null;

ratios = loadRatios(layout, ratios, includes, excludes);
return new JacocoBuildAction(owner, rule, ratios, thresholds, listener, includes, excludes);
return new JacocoBuildAction(owner, ratios, thresholds, listener, includes, excludes);
}


@@ -359,4 +355,12 @@ public final PrintStream getLogger() {
// does not run the construct and thus leaves the transient variables empty
return System.out;
}

public void onAttached(Run<?, ?> run) {

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

@Override

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

Plugin uses Java5, so I think that @override cannot be used in interface. Also IntelliJ complains about it.

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

the version of Jenkins (1.609.3) you are now targeting uses 1.6 - so you can fix that :-)

this.owner = run;
}

public void onLoad(Run<?, ?> run) {

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

@Override

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

Plugin uses Java5, so I think that @override cannot be used in interface. Also IntelliJ complains about it.

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

the version of Jenkins (1.609.3) you are now targeting uses 1.6 - so you can fix that :-)

this.owner = run;
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.