Support withChecks #721

mrginglymus · 2020-12-26T17:57:16Z

This PR adds support for the withChecks step recently added to the checks-api plugin, that allows for customization of published checks names by context.

codecov · 2020-12-26T20:34:33Z

Codecov Report

Merging #721 (5770309) into master (11841d9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #721      +/-   ##
============================================
+ Coverage     79.86%   79.88%   +0.01%     
- Complexity     1505     1506       +1     
============================================
  Files           241      241              
  Lines          5489     5493       +4     
  Branches        397      397              
============================================
+ Hits           4384     4388       +4     
  Misses          945      945              
  Partials        160      160

Impacted Files	Coverage Δ	Complexity Δ
...plugins/analysis/core/steps/PublishIssuesStep.java	`57.86% <100.00%> (+0.26%)`	`29.00 <0.00> (ø)`
...ns/analysis/core/steps/WarningChecksPublisher.java	`97.46% <100.00%> (+0.09%)`	`23.00 <2.00> (+1.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11841d9...5770309. Read the comment docs.

mrginglymus · 2020-12-27T10:08:00Z

@uhafner the Auto Assign PR check failed to auto assign PRs, so dropping you a quick ping in case you missed this otherwise. I'm not sure on the GitHub CI failure - looks environmental?

uhafner

We also need to add that code in https://github.com/jenkinsci/warnings-ng-plugin/blob/master/plugin/src/main/java/io/jenkins/plugins/analysis/core/steps/RecordIssuesStep.java#L1071.

It will be a little bit more complicated since the FreeStyle recorder uses the same code in https://github.com/jenkinsci/warnings-ng-plugin/blob/master/plugin/src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesRecorder.java#L753

uhafner · 2020-12-28T17:08:25Z

plugin/src/main/java/io/jenkins/plugins/analysis/core/steps/PublishIssuesStep.java

+                String checksName = Optional.ofNullable(getContext().get(ChecksInfo.class)).map(ChecksInfo::getName).orElse(null);
+                WarningChecksPublisher checksPublisher = new WarningChecksPublisher(action, getTaskListener(), checksName);


I would rather prefer it look like:

WarningChecksPublisher checksPublisher = new WarningChecksPublisher(action, getTaskListener(), getContext().get(ChecksInfo.class));

Then we can place the logic inside of WarningChecksPublisher and reuse it for the recordIssues step. (PublishIssuesStep is a different step with the name publishIssues).

uhafner · 2020-12-28T17:10:25Z

plugin/src/main/java/io/jenkins/plugins/analysis/core/steps/WarningChecksPublisher.java


-    WarningChecksPublisher(final ResultAction action, final TaskListener listener) {
+    WarningChecksPublisher(final ResultAction action, final TaskListener listener, final String checksName) {


See above: parameter should be of type ChecksInfo.
And do not add a second constructor since we need the same call for the IssueRecorder (which is used by the recordIssues step).

mrginglymus · 2020-12-28T18:14:06Z

Ahh, I knew there was another place. With RecordIssues, do we want to not use ChecksInfo when publishing multiple checks? Otherwise they will all get (slightly) overwritten - the annotations should be preserved, but other bits will get overwritten.

mrginglymus · 2020-12-28T18:15:10Z

Tests for recordIssues to follow.

uhafner

Thanks, this PR looks already great! I only found some minor things left to do.

plugin/src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesRecorder.java

uhafner · 2020-12-28T21:35:32Z

plugin/src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesRecorder.java

@@ -591,6 +594,10 @@ public void setFilters(final List<RegexpFilter> filters) {
        this.filters = new ArrayList<>(filters);
    }

+    public void setChecksInfo(final ChecksInfo checksInfo) {


Suggested change

public void setChecksInfo(final ChecksInfo checksInfo) {

public void setChecksInfo(@CheckForNull final ChecksInfo checksInfo) {

uhafner · 2020-12-28T21:36:15Z

plugin/src/main/java/io/jenkins/plugins/analysis/core/steps/WarningChecksPublisher.java


-    WarningChecksPublisher(final ResultAction action, final TaskListener listener) {
+    WarningChecksPublisher(final ResultAction action, final TaskListener listener, final ChecksInfo checksInfo) {


Suggested change

WarningChecksPublisher(final ResultAction action, final TaskListener listener, final ChecksInfo checksInfo) {

WarningChecksPublisher(final ResultAction action, final TaskListener listener, @CheckForNull final ChecksInfo checksInfo) {

plugin/src/main/java/io/jenkins/plugins/analysis/core/steps/WarningChecksPublisher.java

uhafner · 2020-12-28T21:57:24Z

plugin/src/test/java/io/jenkins/plugins/analysis/core/steps/WarningChecksPublisherITest.java

+        WorkflowJob project = createPipeline();
+        copySingleFileToWorkspace(project, NEW_CHECKSTYLE_REPORT);


Suggested change

WorkflowJob project = createPipeline();

copySingleFileToWorkspace(project, NEW_CHECKSTYLE_REPORT);

WorkflowJob project = createPipelineWithWorkspaceFiles(NEW_CHECKSTYLE_REPORT);

The file name needs to be renamed automatically, otherwise it is not found.

uhafner · 2020-12-28T22:05:42Z

plugin/src/test/java/io/jenkins/plugins/analysis/core/steps/WarningChecksPublisherITest.java

+
+        List<ChecksDetails> publishedChecks = PUBLISHER_FACTORY.getPublishedChecks();
+
+        assertThat(publishedChecks.size()).isEqualTo(2);


Suggested change

assertThat(publishedChecks.size()).isEqualTo(2);

assertThat(publishedChecks).hasSize(2); // add a small comment why there are two results?

plugin/src/test/java/io/jenkins/plugins/analysis/core/steps/WarningChecksPublisherITest.java

uhafner · 2020-12-30T00:29:30Z

Thanks! Your CapturingChecksPublisher.Factory really is a very helpful concept, makes testing of the checks much easier! I already used it in

warnings-ng-plugin/plugin/src/test/java/io/jenkins/plugins/analysis/core/steps/WarningChecksPublisherITest.java

Line 275 in 8cc24ca

public void shouldReportSelectedIssues() {

(part of #724).

mrginglymus added 4 commits December 26, 2020 17:51

Add test for withChecks support

f34574d

Support withChecks

c37169c

Update readme

4110deb

Satisfy linters

5770309

uhafner requested changes Dec 28, 2020

View reviewed changes

Pass around checksInfo, not checksName; support recordIssues

36ee313

Add test for withChecks/recordIssues

979f792

mrginglymus requested a review from uhafner December 28, 2020 18:56

uhafner requested changes Dec 28, 2020

View reviewed changes

Review feedback

c46e911

mrginglymus requested a review from uhafner December 29, 2020 17:03

uhafner approved these changes Dec 29, 2020

View reviewed changes

uhafner merged commit f641419 into jenkinsci:master Dec 29, 2020

uhafner added the enhancement Enhancement of existing functionality label Dec 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support withChecks #721

Support withChecks #721

mrginglymus commented Dec 26, 2020 •

edited

Loading

codecov bot commented Dec 26, 2020 •

edited

Loading

mrginglymus commented Dec 27, 2020

uhafner left a comment

uhafner Dec 28, 2020

uhafner Dec 28, 2020

mrginglymus commented Dec 28, 2020

mrginglymus commented Dec 28, 2020

uhafner left a comment

uhafner Dec 28, 2020

uhafner Dec 28, 2020

uhafner Dec 28, 2020

uhafner Dec 28, 2020

uhafner commented Dec 30, 2020

		String checksName = Optional.ofNullable(getContext().get(ChecksInfo.class)).map(ChecksInfo::getName).orElse(null);
		WarningChecksPublisher checksPublisher = new WarningChecksPublisher(action, getTaskListener(), checksName);


		WarningChecksPublisher(final ResultAction action, final TaskListener listener) {
		WarningChecksPublisher(final ResultAction action, final TaskListener listener, final String checksName) {

	public void setChecksInfo(final ChecksInfo checksInfo) {
	public void setChecksInfo(@CheckForNull final ChecksInfo checksInfo) {

		WorkflowJob project = createPipeline();
		copySingleFileToWorkspace(project, NEW_CHECKSTYLE_REPORT);

	WorkflowJob project = createPipeline();
	copySingleFileToWorkspace(project, NEW_CHECKSTYLE_REPORT);
	WorkflowJob project = createPipelineWithWorkspaceFiles(NEW_CHECKSTYLE_REPORT);


		List<ChecksDetails> publishedChecks = PUBLISHER_FACTORY.getPublishedChecks();

		assertThat(publishedChecks.size()).isEqualTo(2);

	assertThat(publishedChecks.size()).isEqualTo(2);
	assertThat(publishedChecks).hasSize(2); // add a small comment why there are two results?

Support withChecks #721

Support withChecks #721

Conversation

mrginglymus commented Dec 26, 2020 • edited Loading

codecov bot commented Dec 26, 2020 • edited Loading

Codecov Report

mrginglymus commented Dec 27, 2020

uhafner left a comment

Choose a reason for hiding this comment

uhafner Dec 28, 2020

Choose a reason for hiding this comment

uhafner Dec 28, 2020

Choose a reason for hiding this comment

mrginglymus commented Dec 28, 2020

mrginglymus commented Dec 28, 2020

uhafner left a comment

Choose a reason for hiding this comment

uhafner Dec 28, 2020

Choose a reason for hiding this comment

uhafner Dec 28, 2020

Choose a reason for hiding this comment

uhafner Dec 28, 2020

Choose a reason for hiding this comment

uhafner Dec 28, 2020

Choose a reason for hiding this comment

uhafner commented Dec 30, 2020

mrginglymus commented Dec 26, 2020 •

edited

Loading

codecov bot commented Dec 26, 2020 •

edited

Loading