Improve our approach for testing auth

[chris48s]

Our auth tests tend to fall into one of two categories

Either we implement auth on a base class, then test the auth is handed correctly in the base class using a dummy
https://github.com/badges/shields/blob/master/services/stackexchange/stackexchange-base.spec.js

Or we pick one concrete implementation and test that
https://github.com/badges/shields/blob/master/services/gitlab/gitlab-tag.spec.js

This is not water-tight.

As @calebcartwright suggests in #9387 (comment)
it would be good if we could be a bit more robust about this and make sure we cover all the concrete service classes that are supposed to use auth.

I've not tried it yet, but one possible solution might be to encapsulate the test boilerplate once in a function, then loop over all the relevant classes and call it

something like

function runTheTests(ServiceClass) {
  const service = new ServiceClass();
  // setup
  // make assertions
}

for (const serviceClass of [ServiceClass1, ServiceClass2]) {
  runTheTests(serviceClass)
}

[calebcartwright]

@jNullj wanted to flag your attention in case this was something you'd be interested in working on (definitely no pressure though!)

[jNullj]

I gave this a quick look and i think i am up to the task.
Just to make sure i got the gist of it, the idea is to test every service (and not just base service) while using the same base function.
Would that mean i should probebly loop over services for each of the base classes the require auth?
Should I add those tests to each base class spec file?

[chris48s]

Just to make sure i got the gist of it, the idea is to test every service (and not just base service) while using the same base function.

Bingo

Would that mean i should probebly loop over services for each of the base classes the require auth?
Should I add those tests to each base class spec file?

First off: I think probably the best way to tackle this will be to pick one service. Lets say stackexchange for the sake of argument - just so we can use concrete examples in the next bit - but we could pick another one first. Work on a PR for that one first to establish the pattern. That will mean we don't have to modify too much code if there is a bit of back-and-forth on the PR. Then once we've reviewed that and decided on an approach we like we can do another PR (or a series of them) applying that approach to the other services.

Beyond that, I think that's a couple of ways to approach it:

One option would be the example I gave in the top post. So that one would probably look like a single test file (in this case stackexchange.spec.js) which imports StackExchangeMonthlyQuestions, StackExchangeReputation, StackExchangeQuestions and tests all of them.

Another way would be to define a re-usable helper in a stackexchange-test-helpers.js that can then be imported into individual test modules for stackexchange-monthlyquestions.spec.js, stackexchange-reputation.spec.js and stackexchange-taginfo.spec.js.

I think either of those could work. I think we probably want to optimise for:

• reducing boilerplate and also
• making it clear what failed if something failed

[chris48s]

It seems like #9681 is pretty close to merging which gives us a good starting point for a shared helper and pattern we can apply.

Once we merge that, the next steps of this will be to make use of those and make a series of follow up PRs gradually bringing other services into line with the convention.

The following services have got some kind of tests in place. Some of them are testing just the base class, some test one concrete implementation or all concrete implementations. They're a bit of a mixed bag:

• bitbucket: Basic Auth
• discord: Authorization header
• drone: Authorization header
• gitea: Authorization header
• gitlab: Authorization header
• jenkins: Basic Auth
• jira: Basic Auth
• librariesio: Query String
• nexus: Basic Auth
• npm: Authorization header
• obs: Basic Auth
• opencollective: Query String
• sonar: Basic Auth
• symfony: Basic Auth
• teamcity: Basic Auth
• wheelmap: Query String

These ones all have no tests at all covering the auth:

• azure-devops: Basic Auth
• bower: Query String, shared with librariesio
• curseforge: API Key header
• pepy: API Key header
• youtube: Query String
• weblate: Authorization header

..and then these three are complicated and messy. I'm going to suggest we ignore these as they are unlikely to fit into any generic pattern.

• github
• twitch
• docker

One thing we could do next is pick off all the services that use query string auth first.

Another one could be to concentrate on making the test helper more generic. If we've got a test helper (or 3 helpers) that work for services that implement auth using query string, basic auth, and authorization header, that would cover most cases.

[chris48s]

#9681 implements the necessary test helpers an applies the pattern to:

• services/discord/discord.spec.js
• services/docker/docker-automated.spec.js
• services/obs/obs.spec.js
• services/pepy/pepy-downloads.spec.js
• services/stackexchange/stackexchange-monthlyquestions.spec.js
• services/stackexchange/stackexchange-reputation.spec.js
• services/stackexchange/stackexchange-taginfo.spec.js

[jNullj]

Will add other services today or tomorrow

[chris48s]

Awesome 👍 Thanks

When you do this, can you split them down into chunks somehow. Doesn't necessarily have to be as fine grained as one PR per service, but maybe one per auth method: All the Basic Auth services in one PR, all the Query String services in another. Something like that.

If I've got a bunch of small PRs to review, its easier to make time for them. One huge PR will end up sitting around for ages because it is harder to find the time to review it. Ta