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

Conversation

@lordofthejars
Copy link
Contributor

commented Dec 22, 2015

There is no test since testing JaCoCo plugin in unit test it goes to a chicken-egg problem. JaCoCo needs a classes directory to copy them to its internal structure, so the first idea is to copy those classes before executing the job, but then Jenkins complains that this job has already created a directory and it is not executed. I have tested manually and it works and a test should be added to ATH.

In fact if you look at all JaCoCoPublisher tests that were already created you'll notice that they do nothing or are commented, probably because of the same problem I faced during Workflow integration.

Issue: https://issues.jenkins-ci.org/browse/JENKINS-27120

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

commented Dec 22, 2015

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@lordofthejars lordofthejars changed the title [FIXED JENKINS-27120] Adding Workflow support for JaCoCo publisher [JENKINS-27120] Adding Workflow support for JaCoCo publisher Dec 22, 2015

@recena

This comment has been minimized.

Copy link

commented Dec 22, 2015

@lordofthejars 👍 to review the coding style. Probably you will receive the common message "Unrelated changes", but...

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

Well pom.xml file originally was formatted with tabs, white spaces (2 and 4)

<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-utils</artifactId>
<version>3.0.15</version>

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

no scope?

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

I don't know it was in the original pom, I have not introduced this change.

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

You did introduce this change - thats why making unrelated changes is a bad idea!

see a whitespace ignoring diff

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

It is ok with no scope since it is used in one class. In previous Jenkins dependency (1.424.6) this artifact was resolved with transitivity. Now that we have moved the minimum Jenkins version to 1.609, this dependency is not there anymore and must be added specifically.

@@ -22,495 +22,536 @@ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.
-->
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

the number of unrelated changes in this file is excessive.

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

I know but the problem is that the project was with tabs, white spaces (2 spaces and 3 spaces) so IntelliJ has reordered in some way.

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

in which case a cleanup should have been ideally done before the changes.


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)

@@ -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.

@@ -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 :-)

try {
directoryPaths = workspace.act(new FilePath.FileCallable<FilePath[]>()
{
public void checkRoles(RoleChecker checker) throws SecurityException {

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

this is the wrong thing to do. use the correct sub type - see the javadoc for FileCallable

Warning: implementations must be serializable, so prefer a static nested class to an inner class.
Subtypes would likely want to extend from either MasterToSlaveCallable or SlaveToMasterFileCallable.

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

Ok, this was in original code, so I suppose I need to change it.

* @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,

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 22, 2015

Member

Incompatible change.

@@ -16,8 +16,8 @@
public class JacocoReportDir {
private final File root;

public JacocoReportDir(AbstractBuild<?,?> build) {
root = new File(build.getRootDir(), "jacoco");
public JacocoReportDir(File rootDir) {

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

backwards incompatable change.

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

Same problem as usually Run is not an AbstractBuild

@@ -203,7 +204,7 @@ public Coverage getLineCoverage() {
/**
* Gets the build object that owns the whole coverage report tree.
*/
public abstract AbstractBuild<?,?> getBuild();
public abstract Run<?,?> getBuild();

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

backwards incompatable change.

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

But again with Workflow we need this, so you are suggesting https://github.com/jenkinsci/bytecode-compatibility-transformer#adapting-type

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

I would run a mile from the BCT. What I am suggesting is you reach out to the plugin maintainer and ask them about breaking backwards API compatibility (and then if they agree do it properly :-) )

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

the other way to do it is to deprecate this and introduce a new method getRun() let getBuild() attempt to cast the Run to a AbstractBuild and throw an exception if its not possible - which should be ok as the code that currently calls this would only call it for a non workflow job...

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

@amuniz points out the withBridgeMethods - but if there are no external users and the maintainer is ok with breaking the API - I would just break the API and reduce the scope wherever possible.

This comment has been minimized.

Copy link
@amuniz

amuniz Dec 22, 2015

Member

👍 to break API if needed and possible instead of adding shitty compatibility code (BTW it's useful to search in GitHub under user:jenkinsci before breaking it)

private boolean changeBuildStatus;

@DataBoundConstructor
public JacocoPublisher() {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 22, 2015

Member

The new constructor could just be empty since you have all the new @DataboundSetters. This would put an end to the sequence of @Deprecated constructors.

@jtnord

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

so at least one 🐛 on the RoleSensitive implementation on FileCallable.

The backwards incompatable changes likely warrant another 🐛 unless the maintainer is fine with this - and in this case you should go further with the changes and remove the public field (seeing as it could break pluginsanyway you may as well break them in a good way)

The whole formatting changes made this really hard to review as well - which is almost another bug in itself.

}

protected static String resolveFilePaths(AbstractBuild<?, ?> build, BuildListener listener, String input) {
protected String resolveFilePaths(Run<?, ?> build, TaskListener listener, String input) {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 22, 2015

Member

Incompatible change.

This comment has been minimized.

Copy link
@amuniz

amuniz Dec 22, 2015

Member

It's protected. Acceptable IMO.

This comment has been minimized.

Copy link
@jtnord

jtnord Dec 22, 2015

Member

but the class is public and non-final.


if ((execPattern==null) || (classPattern==null) || (sourcePattern==null)) {
if(build.getResult().isWorseThan(Result.UNSTABLE)) {
public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 22, 2015

Member

Why are you keeping this one around?

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

For compatibility with old method of JaCoCo

}


public static Result checkResult(JacocoBuildAction action) {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 22, 2015

Member

Incompatible change.

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

Why it is only a helper method, I have substitute this call that was done in two places to a single one. It is an internal refactor.

@@ -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.

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

Well I disagree about format changing, since it is only pom.xml, which in reality has been untouched and at least it fixes that something happens in the past that PR were accepted with different formats, but the other I agree with you

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

There is no public plugin depending on this one (except the terrible DotCi starter pack), so the backward compatibility issue may be irrelevant.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

I disagree about format changing, since it is only pom.xml

It's not. It's everywhere and it makes reviews terribly cumbersome.

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

Ok let me fix some of the problems and let's discuss with the maintainer if we want to add BCT or we can leave as is changing the attribute to private or package.

* Loads the configuration set by user.
*/
@DataBoundConstructor
@Deprecated
public JacocoPublisher(String execPattern, String classPattern, String sourcePattern, String inclusionPattern, String exclusionPattern, String maximumInstructionCoverage, String maximumBranchCoverage

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 22, 2015

Member

The next several hundred lines of diff are nothing but formatting changes. Also, it looks like there's some reordering of methods going on as well.


/** End Setters for Workflow **/

This comment has been minimized.

Copy link
@amuniz

amuniz Dec 22, 2015

Member

Well, it's not for workflow. It's in general a good practice to use @DataBoundSetters for fields that have a default value.

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

this is what I have done right?

This comment has been minimized.

Copy link
@amuniz

amuniz Dec 22, 2015

Member

Yes... I mean, you can remove this comment - it's not accurate and it could lead to confusion.

return publishReports(build, build.getBuildVariables(), build.getRootDir(), build.getWorkspace(), launcher, listener);
}

public void perform(@Nonnull Run<?, ?> run, @Nonnull FilePath filePath, @Nonnull Launcher launcher, @Nonnull TaskListener taskListener) throws InterruptedException, IOException {

This comment has been minimized.

Copy link
@amuniz

amuniz Dec 22, 2015

Member

@Override?

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Dec 22, 2015

Author Contributor

It is Java 5

@amuniz

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

Really difficult to read the diff. Sorry @recena I don't agree.

+1 to format the code, but in a separate PR please. I'm sure I missed some important changes because of formatting changes.

@amuniz

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

@lordofthejars

but then Jenkins complains that this job has already created a directory and it is not executed

What's not executed? Did you try to use @LocalData?

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

New Commit (number 2) fixes the @Override problem and updates Java version to 6. Also it creates a static nested class of FileCallableand adds an scope to a dependency.

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

For me the change of AbstractBuild -> Run is ok and it should not be a problem at all. As all of you suggest I prefer to not start using BCT

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Dec 28, 2015

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@jglick

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

With the whitespace changes this is unreviewable. Let us know when you can prepare a minimal diff.

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2016

Well the only thing that I can do is discard the change and rework it again.

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2016

@jglick have you tried passing w=1 to the url? https://github.com/jenkinsci/jacoco-plugin/pull/63/files?w=1 to avoid white space diffs? Next time I will not do automatic format but at least if you can review it with this option I wouldn't have to redo the change.

@centic9

This comment has been minimized.

Copy link
Member

commented Jan 20, 2016

But reviewing without whitespaces will still leave the PR unmergeable due to the huge number of unrelated changes! I have compared the changes locally and created PR #66 with the reduced changes to allow us to make progress on this one...

I suggest you base further work on that branch.

This PR is therefore obsoleted now by #66 .

@centic9 centic9 closed this Jan 20, 2016

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2016

@centic9 Thanks, I started the same last evening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.