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-30051 - Extend description with more build results #148

Merged
merged 5 commits into from Aug 26, 2015

Conversation

@dawidmalina
Copy link
Contributor

commented Aug 16, 2015

No description provided.

@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2015

Changes inspired by #113

@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2015

@patbos What do you think about this? Early feedback will be helpful.

@patbos

This comment has been minimized.

Copy link
Member

commented Aug 16, 2015

Nice! Looks like it covers https://issues.jenkins-ci.org/browse/JENKINS-22843 also.

@patbos

This comment has been minimized.

Copy link
Member

commented Aug 16, 2015

Tried it out and got this:
screen shot 2015-08-16 at 20 13 37
I guess it is a missing iteration for test results.

if (instance != null) {
return instance.getPlugin(shortName) != null;
}
return true;

This comment has been minimized.

Copy link
@patbos

patbos Aug 16, 2015

Member

If instance is null it means that Jenkins is either initializing or on its way down and if that is the case we cant do that much so throw an IllegalStateException.

@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2015

Fixed. I have plan also add jacoco-plugin (it will cover my internal needs). What I would like to do later is to propose more generic way. Especially inside pipe.js with less code duplicates. Probably will merge all tests into one model with multiple tests result providers. If you have some idea please let me know.

@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2015

@patbos Have you ever faced such error inside injected tests:

testCliSanity(org.jvnet.hudson.test.PluginAutomaticTestBuilder$CliSanityTest) Time elapsed: 0.141 sec <<< ERROR! java.lang.NoClassDefFoundError: org/apache/maven/plugin/MojoNotFoundException

@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2015

BTW I've added Jacoco and cobertura

@patbos

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

It looks like a dependency from Jacoco. It uses the Jacoco Maven plugin to parse the coverage result file.
For now I think junit and results from analysis-core is a good start and do Jacoco and Cobertura in another PR.

Just an idea:
Would be nice to create a Plugin like analysis-core for coverage and if Jacoco and others could use that one for making it easier to integrate.

@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2015

Ok I will split it into 2 different PRs and maybe think about dedicated plugin:). But it won't be on my priority list.

@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2015

I moved to powermock to have ability to mock static classes.

dawidmalina added 2 commits Aug 18, 2015
@dawidmalina dawidmalina changed the title WIP Extend description with more build results Extend description with more build results Aug 20, 2015
@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2015

@patbos @tommysdk change are ready to review.

@dawidmalina dawidmalina changed the title Extend description with more build results JENKINS-30051 - Extend description with more build results Aug 20, 2015
@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2015

Please do a code review and merge if possible:)

@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2015

Rebased. Change can now be merged.

@dawidmalina

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2015

When this change can be merged?

patbos added a commit that referenced this pull request Aug 26, 2015
JENKINS-30051 - Extend description with more build results
@patbos patbos merged commit ada9bc4 into Diabol:master Aug 26, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@patbos

This comment has been minimized.

Copy link
Member

commented Aug 26, 2015

Thanks for the contribution!

@dawidmalina dawidmalina deleted the dawidmalina:more-results branch Aug 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.