Default checks name to be ' / '-joined enclosing blocks names #211

mrginglymus · 2020-11-18T08:45:00Z

This hopefully goes some way to address @jglick's concerns over the current behaviour (#210).

The default now (hopefully!) is that checks are published per junit invocation with a name derived from the enclosing blocks, unless otherwise overridden. This means that, assuming the idiom of one junit invocation per stage is followed, checks will not be overwritten losing data.

src/main/resources/hudson/tasks/junit/JUnitResultArchiver/help-checksName.html

src/main/resources/hudson/tasks/junit/pipeline/JUnitResultsStep/help-checksName.html

timja · 2020-11-18T08:55:51Z

src/main/java/hudson/tasks/junit/JUnitResultArchiver.java

+                // If we haven't been provided with a checks name, and we have pipeline test details, set the checks name
+                // to be a ' / '-joined string of the enclosing blocks names. JUnitChecksPublisher will handle defaults if
+                // checksName ends up being empty or null
+                String checksName = task.getChecksName() != null || pipelineTestDetails == null ? task.getChecksName() :


example PR where this has been tested? both custom checks name and no override

I'm going to spend some quality time with mockito to see if I can get the checks tests testing more of the implementation of parseAndSummarize.

Co-authored-by: Tim Jacomb <t.jacomb@kainos.com>

src/test/java/io/jenkins/plugins/junit/checks/JUnitChecksPublisherTest.java

jglick · 2020-11-18T12:39:40Z

src/main/java/hudson/tasks/junit/JUnitResultArchiver.java

+                // to be a ' / '-joined string of the enclosing blocks names. JUnitChecksPublisher will handle defaults if
+                // checksName ends up being empty or null
+                String checksName = task.getChecksName() != null || pipelineTestDetails == null ? task.getChecksName() :
+                      StringUtils.join(new ReverseListIterator(pipelineTestDetails.getEnclosingBlockNames()), " / ");


Are the spaces around the slash conventional?

I took my inspiration from

junit-plugin/src/main/java/hudson/tasks/junit/CaseResult.java

Line 319 in a39b176

return StringUtils.join(new ReverseListIterator(getEnclosingFlowNodeNames()), " / ") + " / " + rawName;

Right…this is a display name for rendering in an HTML GUI, though, whereas a check name is in a somewhat different category because it is used as a key in the REST API, requested in branch protection settings, etc.

I've been using slashes in my own custom checks without issue; nothing seems to mind.

used as a key in the REST API

As in in Githubs REST API?

I've been using slashes in my own custom checks without issue

Yes of course, I am just asking if there is a convention for using strings that look more like display names (Tests / Branch / Stage) or more like file paths or URIs (tests/branch/stage).

As in in Githubs REST API?

Yes.

Yes.

I'm not sure I see where.

file paths or URIs (tests/branch/stage).

ahh, to look more like this?

I prefer display name. something like Tests / Unit

src/main/java/hudson/tasks/junit/JUnitResultArchiver.java

src/main/resources/hudson/tasks/junit/JUnitResultArchiver/help-checksName.html

src/test/java/io/jenkins/plugins/junit/checks/JUnitChecksPublisherTest.java

jglick

Looking good!

jglick · 2020-11-18T14:14:20Z

src/main/java/hudson/tasks/junit/JUnitResultArchiver.java

-                String checksName = task.getChecksName() != null || pipelineTestDetails == null ? task.getChecksName() :
-                      StringUtils.join(new ReverseListIterator(pipelineTestDetails.getEnclosingBlockNames()), " / ");
+                // to be a ' / '-joined string of the enclosing blocks names. If all of that ends up being empty or null,
+                // set a default of 'Test'


Suggested change

// set a default of 'Test'

// set a default of 'Tests'

jglick · 2020-11-18T14:16:33Z

src/test/java/io/jenkins/plugins/junit/checks/JUnitChecksPublisherTest.java

+    @After
+    public void clearDetails() {
+        InterceptingChecksPublisherFactory.publisher.details.clear();
+    }


Assuming @Rule JenkinsRule (rather than @ClassRule), every test case gets its own Jenkins session, thus a new copy of every @TestExtension, so this would be unnecessary if you just make publisher an instance field rather than static. To find the current InterceptingChecksPublisherFactory instance, use ExtensionList.lookupSingleton.

Ahh, thanks! I figured there must be something like that.

timja · 2020-12-03T09:05:28Z

src/main/java/hudson/tasks/junit/JUnitResultArchiver.java

+                // If we haven't been provided with a checks name, and we have pipeline test details, set the checks name
+                // to be a ' / '-joined string of the enclosing blocks names. If all of that ends up being empty or null,
+                // set a default of 'Tests'
+                String checksName = task.getChecksName();


shouldn't we still include tests in the check ?

Hmm, yes. For context I'm working mostly on python/js environments, where our stages are dedicated to test generation...

stage('JS Tests') { sh 'yarn test' junit 'js_tests.xml' } stage('Python Tests') { sh 'pytest tests/' junit 'python_tests.xml' }

...as opposed to maven where tests are a bit more of a 'side effect' of the build

stage('Build') { sh 'mvn install' junit 'results.xml' archiveArticfcats(...) recordIssues(...) // etc... }

So we'd end up with JS Tests / Tests etc. OTOH, I'm already setting a custom name (and will be using withChecks when it's ready) so I think meeting the 'mvn' use case possibly makes more sense?

Yeah I think so

I made the change locally,

+ + private static final String DEFAULT_CHECKS_NAME = "Tests"; /** * {@link FileSet} "includes" string, like "foo/bar/*.xml" @@ -290,10 +294,12 @@ public class JUnitResultArchiver extends Recorder implements SimpleBuildStep, JU // set a default of 'Tests' String checksName = task.getChecksName(); if (checksName == null && pipelineTestDetails != null) { - checksName = StringUtils.join(new ReverseListIterator(pipelineTestDetails.getEnclosingBlockNames()), " / "); + List<String> checksComponents = new ArrayList<>(pipelineTestDetails.getEnclosingBlockNames()); + checksComponents.add(DEFAULT_CHECKS_NAME); + checksName = StringUtils.join(new ReverseListIterator(checksComponents), " / "); } if (Util.fixEmpty(checksName) == null) { - checksName = "Tests"; + checksName = DEFAULT_CHECKS_NAME; }

Requires tests changes but that gives what I expected

OK, will get that done

Ugh, sorry, you meant Tests at the beginning...

timja · 2020-12-04T08:23:17Z

@mrginglymus

🎉 jenkinsci/bom#374

timja · 2020-12-04T13:33:06Z

#221 May be related, I didn’t do any complex testing in my setup

Default checks name to be ' / '-joined enclosing blocks names

08c73a6

timja reviewed Nov 18, 2020

View reviewed changes

src/main/resources/hudson/tasks/junit/JUnitResultArchiver/help-checksName.html Outdated Show resolved Hide resolved

timja reviewed Nov 18, 2020

View reviewed changes

src/main/resources/hudson/tasks/junit/pipeline/JUnitResultsStep/help-checksName.html Outdated Show resolved Hide resolved

timja reviewed Nov 18, 2020

View reviewed changes

mrginglymus and others added 4 commits November 18, 2020 09:00

Apply suggestions from code review

0e1fa14

Co-authored-by: Tim Jacomb <t.jacomb@kainos.com>

Switch to testing arguments to ChecksDetails

b4dd8a4

Add new tests for new naming functionality

2b08bc2

Fix dependency coflict, hopefully

c70421e

jglick reviewed Nov 18, 2020

View reviewed changes

mrginglymus marked this pull request as ready for review November 18, 2020 12:49

mrginglymus requested review from timja and jglick November 18, 2020 12:49

jglick reviewed Nov 18, 2020

View reviewed changes

src/main/resources/hudson/tasks/junit/JUnitResultArchiver/help-checksName.html Outdated Show resolved Hide resolved

src/test/java/io/jenkins/plugins/junit/checks/JUnitChecksPublisherTest.java Outdated Show resolved Hide resolved

jglick linked an issue Nov 18, 2020 that may be closed by this pull request

Remove/deprecate checksName parameter to junit step #210

Closed

mrginglymus and others added 3 commits November 18, 2020 13:32

Revert helptext for traditional job types

d53c50d

Consolidate calculation of checksname

341ccb1

Don't use mockito, use TestExtension

5205fee

jglick reviewed Nov 18, 2020

View reviewed changes

Review feedback

d1274bc

mrginglymus requested a review from jglick November 18, 2020 14:32

timja reviewed Dec 3, 2020

View reviewed changes

mrginglymus force-pushed the use-enclosing-blocks-for-default-checks-name branch from a2e7710 to c227039 Compare December 3, 2020 10:12

Add 'Tests' to all status checks

db58881

mrginglymus force-pushed the use-enclosing-blocks-for-default-checks-name branch from c227039 to db58881 Compare December 3, 2020 11:38

mrginglymus requested a review from timja December 3, 2020 11:59

timja added the enhancement label Dec 3, 2020

timja merged commit 3c18200 into jenkinsci:master Dec 3, 2020

mrginglymus deleted the use-enclosing-blocks-for-default-checks-name branch December 3, 2020 12:23

jglick mentioned this pull request Dec 4, 2020

Missing summary in check when no test results recorded #221

Closed

Jacob-Steele mentioned this pull request Nov 12, 2022

[#204] - Add SCM checksName parameter jenkinsci/code-coverage-api-plugin#518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default checks name to be ' / '-joined enclosing blocks names #211

Default checks name to be ' / '-joined enclosing blocks names #211

mrginglymus commented Nov 18, 2020 •

edited

Loading

timja Nov 18, 2020 •

edited

Loading

mrginglymus Nov 18, 2020

jglick Nov 18, 2020

mrginglymus Nov 18, 2020

jglick Nov 18, 2020

mrginglymus Nov 18, 2020

jglick Nov 18, 2020

mrginglymus Nov 18, 2020 •

edited

Loading

timja Nov 18, 2020

jglick left a comment

jglick Nov 18, 2020

jglick Nov 18, 2020

mrginglymus Nov 18, 2020

timja Dec 3, 2020

mrginglymus Dec 3, 2020

timja Dec 3, 2020

timja Dec 3, 2020

mrginglymus Dec 3, 2020

mrginglymus Dec 3, 2020

timja commented Dec 4, 2020

timja commented Dec 4, 2020

Default checks name to be ' / '-joined enclosing blocks names #211

Default checks name to be ' / '-joined enclosing blocks names #211

Conversation

mrginglymus commented Nov 18, 2020 • edited Loading

timja Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrginglymus Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja commented Dec 4, 2020

timja commented Dec 4, 2020

mrginglymus commented Nov 18, 2020 •

edited

Loading

timja Nov 18, 2020 •

edited

Loading

mrginglymus Nov 18, 2020 •

edited

Loading