Reduce boilerplate for creating new testers [npm appveyor gem uptimerobot clojars]

[paulmelnikow]

This is a bit of sugar that reduces the boilerplate for creating testers in what I expect will become the standard case: a service in foo/foo-thing.service.js with its tests in foo/foo-thing.tester.js.

This makes a small stylistic change, which is to name the service by its CamelCase class name rather than an invented snake-case ID. Whereas before the name was specified in class FooThing extends Base[Json]Service and a second time in the tester, it now only needs to be specified once.

This plays well with #1933 and #1931.

[shields-ci]

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

Generated by 🚫 dangerJS

[chris48s]

Really like this idea 👍

Is there a reason for only applying this to Libraries.io, NPM and uptimerobot here? Is there a reason it can't work for the other stuff we've refactored - Clojars, Appveyor, Gem, etc?

Also could we update the service test docs to mention this.

[chris48s]

It seems what we're trying to do here is create a ServiceTester object based on some non-trivial criteria. I think a more common approach here would be to use the factory pattern and have something like a ServiceTesterFactory which is responsible for producing the ServiceTester object. Having a static method on the ServiceTester class which creates instances of itself seems like a more unusual structure.

What do you think?

[paulmelnikow]

It's a subtle point you're making, though I agree, that given how specific this logic is it's reasonable to move this out of the class. I don't think we need a factory class, though we could move the magical factory function into its own module and do something like:

const createServiceTester = require('../create-service-tester')

or

const { automagicallyCreateServiceTester } = require('../create-service-tester')

[paulmelnikow]

Is there a reason it can't work for the other stuff we've refactored - Clojars, Appveyor, Gem, etc?

I guess I was just trying not to invest beyond being confident it would work, until I knew folks liked the idea. 😀Have updated those three.

[paulmelnikow]

Also could we update the service test docs to mention this.

It only works with new-style services so I think we should wait and update in that pass.

[chris48s]

It only works with new-style services so I think we should wait and update in that pass.

Aah yes good point

Looks there are now some failing service tests to fix, but I think separating the object creation concerns into a createServiceTester() function makes sense 👍

[paulmelnikow]

The Librariesio 404 failures are not related to this change. I think they are flaky; that whole API is flaky. The other failing tests should be fixed now.

[chris48s]

Yeah I can see that most of those are timeouts.

I can also see that the 'not found' tests failed in the same way on last night's daily build, so this isn't breaking anything worse than it was already. I'm confused about why the not found tests are failing with the wrong not found message:

      -  "value": "not found"
      +  "value": "package not found"

Maybe worth having another look at that once the valid requests are coming back in sensible time.

[chris48s]

There's a couple of other services that could use this syntax as well (APM, cdnjs.. maybe others) but we can do those as another job

[paulmelnikow]

I’m not sure how best to deal with the service tests in relation to merging. I don’t like the idea of having such flaky tests. Usually on pull requests with timeouts I just re-run and re-run and re-run until they do pass. But we have another not found test, which isn’t related.

Maybe the thing to do here is to investigate and fix that not found test first, in a separate PR, and then circle back to this, re-running and re-running the flaky tests until they do pass.

The other alternative is to make service tests not required, though I’m concerned that they could inadvertently be skipped in that case…

[chris48s]

Given this feature isn't fixing a serious bug or anything, we could leave this open for a bit. If libraries.io is responding tomorrow, we've not really lost anything..

If you're keen to get this work in, another option would be to revert the changes to the libraries.io tests so they're still defined with the old syntax i.e:

const t = new ServiceTester({
  id: 'librariesio-dependent-repos',
  title: 'Libraries.io dependent repos',
  pathPrefix: '/librariesio/dependent-repos',
})

and deal with both using the new syntax and working out what's up with the 'not found' case as another job for another day. The main change here is introducing the syntax to core. As I've noted, there are other classes where we're still using the old syntax.

Alternatively, you could just cheat and edit the PR title to "Reduce boilerplate for creating new testers [npm appveyor gem uptimerobot clojars]" then all the tests will pass and you can merge guilt free ;) Of course, #1936 would remove this option in future.

[paulmelnikow]

Haha I think I will cheat for today. I'm eager to get this in, in part because it touches a lot of files, and in part because I think our typical inventory level of open PRs feels a bit high. The longer things are open, the more chance of conflicts there is, which means people put off changes they want to make, or have to do extra work to deal with the changes.

[shields-deployment]

This pull request was merged to master branch. Now this change is waiting for deployment.
Deploys usually happen every few weeks. After deployment changes are copied to gh-pages branch.

This badge displays deployment status: