Improve our approach for testing auth

chris48s commented 1 year ago

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 https://github.com/badges/shields/pull/9387#discussion_r1299106960 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 commented 1 year ago

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

jNullj commented 1 year ago

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 commented 1 year ago

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 commented 9 months ago

It seems like https://github.com/badges/shields/pull/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 commented 8 months ago

https://github.com/badges/shields/pull/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 commented 8 months ago

Will add other services today or tomorrow

chris48s commented 8 months ago

Awesome :+1: 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

badges / shields

Improve our approach for testing auth #9493