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

Remove/deprecate checksName parameter to junit step #210

Closed
jglick opened this issue Nov 16, 2020 · 7 comments · Fixed by #211
Closed

Remove/deprecate checksName parameter to junit step #210

jglick opened this issue Nov 16, 2020 · 7 comments · Fixed by #211

Comments

@jglick
Copy link
Member

jglick commented Nov 16, 2020

checksName was introduced in #189 as an attempt to address #183, namely

we have multiple invocations of junit (across multiple different pipelines jobs) per PR.
This means that currently, only the last reported test appears.

It suffices to use SuiteResult.getEnclosingBlockNames, which will be set automatically according to stage or parallel branch as of #76, without modifications to the Jenkinsfile.

@mrginglymus
Copy link
Contributor

Posted on the PR initially, reposting here:


While I don't disagree that there may be a better way of detecting a default value that isn't Tests, I would be incredibly disappointed to see the ability for a user to set their own checks name at the point of junit invocation removed.

Providing it via the program that produces the xml to be consumed would require both substantial changes to our Jenkinsfiles and junit producing programs, compared to the one line we are able to achieve the behaviour with currently.

We already have an issue with the warningsng plugin where the name of the report on jenkins has to be the exact same as the check reported to github.

For context, our use case here is that we have a number of parent jobs that may run on a single PR, each with a large number of inidividual checks - e.g. our smoke run has > 10 checks reported, our long run about 5. They are a mix of test results, warnings, and coverage, plus some custom checks. They are also duplicated between parent jobs, for instance our build stage runs on both the long run and the deploy run.

Therefore, to help keep some kind of order to the large number of checks, we namespace them by the parent job, so we end up with:

Deploy  # github status, links to jenkins
Deploy / Build  # github check, links to github
Deploy / Deploy
Smoke
Smoke / Linting
Smoke / Unit Tests
Long
Long / Build
Long / Long Tests

etc.

The easiest way to achieve this, by far, is for every plugin that publishes a check to allow the user to provide the name of that check.

I'm also hoping that at some point the checks API will support updating checks, so that we can have a pattern that looks like:

withCheck('My Check Name') { check ->
    sh 'test --output output.xml'
    junit testResults: 'output.xml', check: check
}

ie, a wrapper that creates a check in progress, catches and reports exceptions.


Having now seen the proposal - this would be totally unacceptable as a replacement. We sometimes invoke junit multiple times per stage, so would have to totally rearchitect our jenkinsfiles to meet this reduced flexibility.

Again, I think this would make a lot of sense as a default, but the ability to override must be kept.

@mrginglymus
Copy link
Contributor

Another thought arises - having the checks name have to come from the stage name will make using this check as a required check on github hard, as any refactoring of your jenkinsfile could result in a change to the computed name, requiring changes to configuration of your branch protection rules.

Again to make absolutely clear, I think this would be a sensible default (and I'm happy to raise a PR for it), but it can't be the only option.

@timja
Copy link
Member

timja commented Nov 17, 2020

could you create the feature request on the checks-api plugin?

withCheck('My Check Name') { check ->
    sh 'test --output output.xml'
    junit testResults: 'output.xml', check: check
}

https://github.com/jenkinsci/checks-api-plugin/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc

it makes sense to me

@mrginglymus
Copy link
Contributor

@jglick
Copy link
Member Author

jglick commented Nov 17, 2020

We sometimes invoke junit multiple times per stage

The design of the junit Pipeline step is that it should be run at most once per stage/branch. That is why it collects the “enclosing stage names” the way it does; and these are also preserved in URLs and the GUI—it is essentially part of the data model for the test report. You can certainly argue that it would be a better design to make this “result name” be an explicit argument to the step (which I did), but that is not how the decision went, and having two competing idioms for doing the same thing is undesirable.

At this point checksName has been released and cannot be undone, so at best it can be an override of the name implied by stage structure and used for other purposes in the plugin.

By the way I stumbled across this issue by noticing that jenkinsci/bom builds only displayed a single test report, despite running the junit step numerous times. In that context I would not particularly care about differentiating test results per branch, so long as the total count and failure details were aggregated.

@timja
Copy link
Member

timja commented Nov 17, 2020

In that context I would not particularly care about differentiating test results per branch, so long as the total count and failure details were aggregated.

FTR they aren't aggregated after:
#199

@mrginglymus
Copy link
Contributor

and having two competing idioms for doing the same thing is undesirable.

As far as what name is used for a check on Github, it's seems to be open season as far as the four places I'm currently it:

  1. In the github branch plugin - the top level job either reports 'Jenkins' or a custom name
  2. In junit - it reports either 'Tests' or a custom name (or, per your suggestion if implemented, the enclosing blocks)
  3. In warnings, it reports the name of a the report (which defaults to the name of the tool, but is overridable)
  4. In coverage, I'm not sure it can be set. I think it's just 'Coverage'

I would argue that as far as how jenkins interacts with external services, users should at least be given the option to chose names that suit them. Otherwise in order to have consistency on the receiving end on github, we must force the same idiom used within junit on all other plugins, and indeed on anything consuming such checks (like branch protection rules)

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

Successfully merging a pull request may close this issue.

3 participants