Detect from commit history which service tests to run

[paulmelnikow]

This reworking of the service test detection decouples it from pull requests. It automatically detects which services to run by examining the git working tree and commit history. This is a really nice dev experience. You run npm run test:services and it does the right thing. In the rare case where you don't want that, you can pass -- --all or -- --only=.

CircleCI seems to clone the entire history and all its branches by default, so I think this will work there, too. 🤞

Service tests can no longer be tagged in the PR title. However, they can be tagged in a commit message. Any tagged services are tested in addition to the auto-detected services. There's a minor tradeoff here in the manual tagging: services can be tagged prior to opening the PR and the PR titles are less cluttered; however, it's necessary to push a commit if you want to test more services.

This approach is way more reliable and much simpler: a net deletion of 2 scripts, a temporary file, and 100 lines of code.

[shields-ci]

	Warnings
⚠️	This PR added helper modules in lib/ but not accompanying tests. Generally helper modules should have their own tests.

	Messages
📖	✨ Thanks for your contribution to Shields, @paulmelnikow!
📖	Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

[PyvesB]

Apart from the minor observations regarding the logs, two points come to mind:

• if my understanding is correct, affected-services.js is not calling identifyTesters, therefore if only the tests are modified, they won't be run? Is this really what we want?
• "The test runner examines the commit history to automatically detect which services to test. If for any reason it's not detecting the correct services [...]". What about changes made to server.js? Most of our services are currently in there and it's still the way of doing things recommended by the tutorial. I feel like we would be confusing contributors if we're basically saying that something is expected to happen but it won't actually happen in most cases, at least in the current state of things.

Let me know whether this makes sense!

[PyvesB]

Why is this logged as an error?

[PyvesB]

Isn't this similar to the previous log line? Also, we're logging about the found services in cli.js, so do we need these logs here at all?

[paulmelnikow]

Thanks for the review comments! What you're saying makes perfect sense. I'll respond soon in more detail.

[chris48s]

Good points made by @PyvesB - agree with both points. Also thanks for getting into the more detailed stuff. I haven't done a properly detailed read of this code, but I've had a think about the top-level concept and I've also checked out the code to try a few things and I do have some thoughts on this:

1. Sometimes it is useful to be able to manually specify some services to run tests against even if we haven't modified any service code. For example, we sometimes test dependency upgrades or base class modifications against some sample of services that rely on that dependency. For example: Bump semver from 5.5.0 to 5.5.1 ; test with [luarocks gem npm] #1952 , Bump nock from 9.5.0 to 9.6.1; test with [codeclimate] #1913 , Bump request from 2.87.0 to 2.88.0; test on [travis chocolatey waffle] #1911 - it would be useful to retain this ability.

2. I see where you're going with this for CI purposes, but for local dev, I think the most common case is that you want to run tests on services you're currently changing (i.e: your unstaged changes) as opposed to other stuff recently committed on the branch you're working on. I think this at least needs some clarification in the docs that you need to use only= to test as you work. Maybe another thing we could possibly think about as an enhancement is using environment CI=true to configure behaviour?

3. This seems to break the --only= flag. For example, I checked this out and tried to run the tests for a couple of services and no tests were run:

$ npm run test:services -- --only=gem

> gh-badges@1.3.0 test:services /home/chris/repositories/github_chris48s/shields
> HANDLE_INTERNAL_ERRORS=false mocha --delay lib/service-test-runner/cli.js "--only=gem"

Running tests for 1 services: gem.


$ npm run test:services -- --only=travis

> gh-badges@1.3.0 test:services /home/chris/repositories/github_chris48s/shields
> HANDLE_INTERNAL_ERRORS=false mocha --delay lib/service-test-runner/cli.js "--only=travis"

Running tests for 1 services: travis.

[chris48s]

We should add a note to the docs here that we can also run npm run test:services -- --all to run the full service test suite.

[paulmelnikow]

Thanks for the additional comments! I pretty much agree with you. In my workflow I'm typically refactoring heavily until I finish, so I alternate between troubleshooting an individual service and making sure I broke nothing else.

Troubleshooting an individual service – is that similar to the mode you're describing? I could see testing on my uncommitted changes covering that case pretty well.

Are these the four cases we want to support?

• Changed in this branch
• Changed since last commit
  • Or only unstaged? For my workflow, uncommited would be more helpful
• Only a given list of services
• All services

Sometimes it is useful to be able to manually specify some services to run tests against even if we haven't modified any service code.

This is possible with this code by putting the customary bracketed names into a commit message. Want to give that a shot and see how it feels?

[paulmelnikow]

I meant to add: sorry my responses have been a little short this week! I've had a busy several days and am trying to squeeze in as much Shields work as possible into short sprints here and there.

[chris48s]

Or only unstaged? For my workflow, uncommited would be more helpful

Yeah, sorry. "uncommitted" would be a better word than "unstaged" 👍

This is possible with this code by putting the customary bracketed names into a commit message

Is the PR title supported, or only the commit message?. I think its still important to be able to use the PR title: If you think about a dependency, having to check out dependabot's branch, edit the commit message and force-push it back up is additional friction especially when we'll probably lose it again when dependabot auto-rebases.

[paulmelnikow]

A few options to think about…

Is the PR title supported, or only the commit message?. I think its still important to be able to use the PR title: If you think about a dependency, having to check out dependabot's branch, edit the commit message and force-push it back up is additional friction especially when we'll probably lose it again when dependabot auto-rebases.

Only the commit message. I think you've seen it happen (and commented recently) that CircleCI builds don't correctly detect that they are on a PR. There is no way to do that reliably with CircleCI.

Actually, that's not completely true. There is one way to do it: configure CircleCI to run only on PRs.

That would necessitate either:

• A separate CI service – or simply a separate repo – to run the daily tests
• A bot that triggers the nightly Circle build programmatically

Alternatively, we could experiment with another way to handle Dependabot, which is to check out the branch and push a second commit. Or to edit something in the UI (we could designate a dummy file to edit, like a .gitkeep somewhere) and add a second commit. I believe once a new commit is pushed, Dependabot will not auto-rebase. We could experiment to see what happens if you manually trigger dependabot to rebase. Also, often, merging master through the UI works fine on dependabot PRs.

Finally, we could consider running service tests through a different CI service that is more tightly bound to PRs. Rather than editing the PR, we could have a bot that listened for comments like @shields-ci test appveyor service and triggered additional builds. Finally, we could combine these techniques. If Dependabot plays well enough with additional commits on the branch, we could use PR comments to trigger a bot that pushed a commit.

[chris48s]

Alternatively, we could experiment with another way to handle Dependabot, which is to check out the branch and push a second commit.
...
I believe once a new commit is pushed, Dependabot will not auto-rebase. We could experiment to see what happens if you manually trigger dependabot to rebase

Having done a quick test: #1949 (comment) we lose that as soon as we push another commit. I think not having that feature will become annoying.

we could have a bot that listened for comments like @shields-ci test appveyor service and triggered additional builds

Personally I think this is the nicest suggestion as it fits in quite nicely with the dependabot @command workflow. Simultaneously it also probably represents the biggest yak-shave and introduces some additional complexity when we need to configure/test it.

Another option, of course, is we can run the relevant tests locally (I probably prefer that to losing dependabot auto-rebase). If we can solve everything else, that's a viable interim solution because we can use the commit message text in the case where we're making a modification to base classes/error handling/etc which doesn't directly touch any service but has impact on multiple services.

[paulmelnikow]

Personally I think this is the nicest suggestion as it fits in quite nicely with the dependabot @command workflow. Simultaneously it also probably represents the biggest yak-shave …

Totally agreed.

Merging master through the UI (using the Update branch button) is not much harder than manually rebasing; I wouldn't mind doing that in the interim.

[paulmelnikow]

I'm not happy with the current state of affairs, and while there are aspects of this approach I really prefer, it's clear it will need to be rethought through.

Maybe we should use a bot. Could be in conjunction with CircleCI, another on-demand system, or something like Drone. I'd prefer something on-demand so it doesn't require sysadmin effort or create a knowledge or access bottleneck.