[JenkinsCoverage] Update Jenkins Code Coverage API for new plugin version

[mincrmatt12]

Recently, the Jenkins Code Coverage API plugin re-added its external API, but changed the format. This updates the Jenkins service to accommodate the new API.

Unfortunately, this breaks the current test case and I can't find any public instances that have updated to the new plugin version. I've replaced it with my own instance that is up to date, but it's not hosted on a particularly reliable server. I imagine the server jenkins.library.illinois.edu which used to be used will update at some point, so in the future the URL could probably be reverted to what it was before commit 1f104b2.

[github-actions]

	Messages
📖	✨ Thanks for your contribution to Shields, @mincrmatt12!

Generated by 🚫 dangerJS against 692164d

[chris48s]

Unfortunately, this breaks the current test case and I can't find any public instances that have updated to the new plugin

More importantly than just the test cases: From what you're saying there are going to be people out there using this whose badges will break if we deploy this change because they have not updated to the same version of jenkins you are using yet. i.e: if I deploy this change tomorrow, we fix the badge for you (and some other users) but also break it for a bunch of other people.

I think given that, we need to be able to maintain compatibility with both versions.

I think my preferred way to do this would be to define 2 schemas: One for each version. Then we allow the response to match either using Joi.alternatives() and make the badge logic conditional on the response shape. I don't know if v1 and v2 is the right terminology, but we'd do something like

const v1Schema = Joi.object({
  results: Joi.object({
    elements: Joi.array()
      .items(
        Joi.object({
          name: Joi.string().required(),
          ratio: Joi.number().min(0).max(100).required(),
        })
      )
      .has(Joi.object({ name: 'Line' }))
      .min(1)
      .required(),
  }).required(),
}).required()

const v2Schema = Joi.object({
  projectStatistics: Joi.object({
    line: Joi.string()
      .pattern(/\d+\.\d+%/)
      .required(),
  }).required(),
}).required()

...

api: {
  schema: Joi.alternatives.try(v1Schema, v2Schema),

  transform: function(json) {
    if ('results' in json) {
      // do this thing
    }
    if ('projectStatistics' in json) {
      // do that thing
    }
  }
}

I've not tested that code, but hopefully the idea makes sense.

[calebcartwright]

What about making this an input vector via the query parameters? I feel like we've got somewhat a similar precedent with the Sonar badges which require the version to be provided

[mincrmatt12]

The "determine based on response" approach won't work since the URL changed between versions. I think using an extra query parameter if we want to support the old version of the plugin would work though.

The situation with the plugin is this:

• the first version of the plugin had a remote API that the current badge uses
• since the 2.0 release (around Oct 2021) it's been nonfunctional
• the recent 4.0 release re-added it with the new URL/schema that this PR uses

so I'm not sure whether or not the badge should default to using the new version. Maybe the query param should default to the old plugin if not provided but the frontend builder should default to the new one? (although I haven't looked at how the frontend works so I'm not sure if that's easily done)

[chris48s]

OK thanks.

In general, we prioritise backwards-compatibility for existing users so I think the current behaviour should remain the default for badges using the existing route.

Some possible options:

1. Make the new route /apiv4, so the format choices become /jacoco, /cobertura, /api (which is the v1 format) and /apiv4 (the new one)
2. Make the new one /apiv4, rename the old one to /apiv1 and provide a redirect from /api to /apiv1, so the documented format choices are /jacoco, /cobertura, /apiv1, /apiv4 but existing badges using /api redirect to /apiv1. This is basically the same as 1 but it makes it a bit clearer what's going on in the frontend
3. Leave the format choices as /jacoco, /cobertura, /api but add a ?version= query param, which defaults to 1 if not supplied (again for backwards-compatibility)
4. Leave the route as /api. Try requesting /coverage/result and /coverage in parallel and roll with whichever one returns a 200, validating against the appropriate schema.

I think option 2 is probably the least-worst of those IMO.

[mincrmatt12]

Alright, I've had a go at implementing the "expose as apiv1/apiv4 and redirect old badges". It looks like the previous test case for the V1 api has mysteriously stopped working, so I've had to remove that test case. Should it be mocked instead?

[chris48s]

Nice work. I've had a look over this and it looks in good shape. If you could replace the v1 test with a mocked test, that would be great. There are lots of examples of service tests using mocks https://github.com/search?q=repo%3Abadges%2Fshields%20.intercept(nock&type=code I don't know of any other public instances we can use.

side note: For centralised services, it is good to have tests that call the live service (even if they can be a bit flaky in some cases) because it means if something changes upstream (e.g: a package registry make a change to their API, or an endpoint gets deprecated, or whatever) then we know about it because our tests start failing. With services like Jenkins which are self-host only and there is no central service, I wonder if it would make more sense to get rid of the live tests and just test against mocks because we don't really get the payoff we normally exchange for the live tests being flaky.

Anyway, that isn't something we need to change in this PR. Lets just mock the apiv1 test and then I'll get this merged. Thanks

[mincrmatt12]

Ok I've added a mock based on data from an older build from the previously used test instance.

[chris48s]

Thanks. I've pushed one more commit to this before merging, just to change the formatting.
We can pass nock a js object rather than a JSON string for the response, which allows us to lay it out in a more readable way.
Cheers

[JulienElkaim]

It does not seem to work?
For example, my Jenkins build here use Code Coverage API Plugin and returns 94%
http://load-balancer-jenkins-master-1462245991.ap-northeast-1.elb.amazonaws.com:8080/job/PRs%20-%20CI%20Build/job/babar/3/

However, when I try https://shields.io/badges/jenkins-coverage with apiv4 nothing shows :/
Is there an update to do to support this coverage plugin?

I use Code Coverage API Plugin 4.7.0

[mincrmatt12]

The current version of the badge expects coverage results to be published with the id coverage, so it'll probably work if you change that in your Jenkinsfile. There should probably be an issue for adding an extra parameter for this though.

[JulienElkaim]

Thank you @mincrmatt12 you were right the issue was not a lack of support of the new API. The specificity of the id value was key, my badge now works. Thanks!