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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce1f732
[FIXED JENKINS-27120] Adding Workflow support for JaCoCo publisher
lordofthejars Dec 22, 2015
b872988
fixes @Overrides, inner-class to static class, dependency scope.
lordofthejars Dec 22, 2015
d279a2d
make plexus compile scope
lordofthejars Dec 22, 2015
4fde003
removes unnecessary comments.
lordofthejars Dec 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
975 changes: 508 additions & 467 deletions pom.xml
View file
Open in desktop

Large diffs are not rendered by default.

40 changes: 23 additions & 17 deletions src/main/java/hudson/plugins/jacoco/JacocoBuildAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Expand All @@ -45,11 +48,6 @@ public final class JacocoBuildAction extends CoverageObject<JacocoBuildAction> i
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
Expand All @@ -59,23 +57,21 @@ public final class JacocoBuildAction extends CoverageObject<JacocoBuildAction> i
/**
*
* @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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Expand Down Expand Up @@ -201,12 +197,12 @@ public Object getTarget() {
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
}

/**
Expand Down Expand Up @@ -274,8 +270,8 @@ public Map<Coverage,Boolean> getCoverageRatios(){
/**
* 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) {
Expand All @@ -297,12 +293,12 @@ public Map<Coverage,Boolean> getCoverageRatios(){
* @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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}


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

@Override
public void onAttached(Run<?, ?> run) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Override

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.owner = run;
}

@Override
public void onLoad(Run<?, ?> run) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Override

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.owner = run;
}
}
Loading