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

Add capturing checks publisher, make publishChecks withChecksName aware #55

Merged
merged 4 commits into from
Dec 22, 2020

Conversation

mrginglymus
Copy link
Contributor

@mrginglymus mrginglymus commented Dec 20, 2020

Addresses #54

/**
* Tests that the step "publishChecks" can be used in pipeline script.
*
* @throws IOException if fails get log from run
*/
@SuppressWarnings("OptionalGetWithoutIsPresent")
Copy link
Member

Choose a reason for hiding this comment

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

I would just extract the output to a variable and chuck and orElseThrow instead of .get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'll be a lot of orElseThrows! Might make it a bit less readable.

@mrginglymus
Copy link
Contributor Author

Is there a way to shift this code into the checks-api-tests.jar to neaten up the production jar?

@XiongKezhi
Copy link
Contributor

Is there a way to shift this code into the checks-api-tests.jar to neaten up the production jar?

Do we have any other places that need it?

@timja
Copy link
Member

timja commented Dec 21, 2020

Is there a way to shift this code into the checks-api-tests.jar to neaten up the production jar?

There's a few plugins that ship jars for this under the classifier 'tests' jcasc used to, and some of the pipeline ones do, search the bom for the tests classifier: https://github.com/jenkinsci/bom

@uhafner
Copy link
Member

uhafner commented Dec 21, 2020

Is there a way to shift this code into the checks-api-tests.jar to neaten up the production jar?

Yes, just move the code to the tests folder and add an inclusion to the pom, see example. Custom assertions are already picked up by the test jar.

@mrginglymus
Copy link
Contributor Author

Is there a way to shift this code into the checks-api-tests.jar to neaten up the production jar?

Do we have any other places that need it?

It's only needed in tests, but it will be useful for testing in the consumers (I'm adding some details to consumers-guide.md) such as junit, where I originally added this. I was going to add support to withChecks to some of the other consumers but realised it would be a lot easier with this!

in the `test` classifier, as `io.jenkins.plugins.checks.api.test.CapturingChecksPublisher`.

Adding the factory for this publisher as a `TestExtension` will allow inspection of published checks after running a job
on a `JenkinsRule`.
Copy link
Member

Choose a reason for hiding this comment

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

I would add the link from the javadoc here as well,

to io.jenkins.plugins.checks.steps.PublishChecksStepITest

@timja timja added the tests label Dec 21, 2020

ChecksDetails details = PUBLISHER_FACTORY.getPublishedChecks().get(0);

assertThat(details.getName().get()).isEqualTo("customized-check");
Copy link
Member

Choose a reason for hiding this comment

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

Typically, one would use in AssertJ:

Suggested change
assertThat(details.getName().get()).isEqualTo("customized-check");
assertThat(details.getName()).isPresent().get().isEqualTo("customized-check");

This will also not require the annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, neat! Thanks.

ChecksDetails extractChecksDetails() throws IOException, InterruptedException {
// If a checks name has been provided as part of the step, use that.
// If not, check to see if there is an active ChecksInfo context (e.g. from withChecks).
String checksName = Optional.ofNullable(Util.fixEmpty(step.getName())).orElse(
Copy link
Contributor

Choose a reason for hiding this comment

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

For better grammar

Suggested change
String checksName = Optional.ofNullable(Util.fixEmpty(step.getName())).orElse(
StringUtils.defaultIfEmpty(step.getName(),
Optional.ofNullable(getContext().get(ChecksInfo.class))
.map(ChecksInfo::getName)
.orElse(StringUtils.EMPTY))

@mrginglymus
Copy link
Contributor Author

New mystery - why is the Windows build hanging between reporting integration test results and running the run-revapi step...

@XiongKezhi
Copy link
Contributor

New mystery - why is the Windows build hanging between reporting integration test results and running the run-revapi step...

Not a big deal, there are always some problems with the windows build on our community ci (due to resources and so on); normally I only take a look at the github builds and the linux build on the community server.

@XiongKezhi XiongKezhi merged commit 3a298a6 into jenkinsci:master Dec 22, 2020
@XiongKezhi XiongKezhi linked an issue Dec 22, 2020 that may be closed by this pull request
@mrginglymus mrginglymus deleted the checks-capture-publisher branch December 22, 2020 11:29
<executions>
<execution>
<goals>
<goal>test-jar</goal>
Copy link
Member

Choose a reason for hiding this comment

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

@XiongKezhi this is normally accomplished just with

<no-test-jar>false</no-test-jar>

For some reason you are inheriting from analysis-pom, which does things differently, rather than directly from org.jenkins-ci.plugins:plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add publisher for test.
5 participants