build: adds GitHub Actions for test/integration/lint

[bcoe]

Description of proposed changes

Follow on from #1057, adds GitHub actions for lint, test, integration. If #1057 is landed, it might be worth combining it into the single ci.yaml.

Related issue(s)

Related to #1057

Testing

I've tested the configuration here.

Notes

Relationship between integration tests and #1057:

I noticed when wiring up integration-test, that it has many similarities with #1057 (in that, it requires the datasets, the server is running, asserts against text) -- the advantage is that folks can simply enumerate a variety of URLs in urls.txt (it's a nice little abstraction)...

I'd argue it might be worth dropping integration-test, in favor of the approach in #1057, and the E2E work happening in #917 around E2E visual testing.

Requiring tests on PRs:

I recommend enforcing branch protection if this lands:

[bcoe]

used the same trick as #1058, which has been working nicely (I've had the smoke tests pass in actions 10 or so times now, on my testing repo).

[bcoe]

gets lint actually passing.

[jameshadfield]

Think this (and the lint job) should use ${{ matrix.node }}. Eventually I can see us testing on multiple node versions

[bcoe]

I added a matrix for both lint and integration 👍

[jameshadfield]

This is really cool. I pushed up a copy of this branch as this temporary PR to test the CI. It is looking really good, but I have a couple of questions:

1. The workflow doesn't actually run on the PR itself, rather it runs on a merge of the PR into master (see this part of the actions/checkout@v2 docs). This creates a confusing failure in the above PR, where linting fails on a recent introduction in master which isn't in this PR (https://github.com/nextstrain/auspice/pull/1064/checks?check_run_id=588632308#step:5:10).

Is this normal practice? I imagine that after implementing this CI we avoid linting errors so this shouldn't be a problem, and in more complicated situations it favors a rebase-onto-master approach.

2. The unit tests fail CI with ReferenceError: page is not define which I think is addressed by a comment of yours on test: text-based dataset smoke test #1057. Would you mind adding that to this PR?

I recommend enforcing branch protection if this lands

👍 will do

[bcoe]

The workflow doesn't actually run on the PR itself, rather it runs on a merge of the PR into master

I'm using a similar configuration to what I use on yargs (seen here). I believe it will run on pull requests once merged, and part of the repos main workflow:

on:
  push:
    branches:
      - master
  pull_request:

☝️ indicates, run on all pull_request, only run on push to master. This is so, for pull_request, you don't get a suite of tests for both the push and the PR changes.

[jameshadfield]

Looking back at it my comment didn't quite explain what I was trying to get at:

My understanding is that the CI will run on each PR, and so you'll get a "checks" tab like you see in #1064. But the actual code the CI tests are being run on (i.e. the GitHub runner set-up, which has the auspice code available via actions/checkout@v2) is a merge of the PR into master, i.e. you're testing the world that would exist were the PR to me merged. This is why you get failures such as 👇 where the line which fails has been introduced in master upstream of where this PR branched off.

P.S. Do you know why the CI is running on this PR? I thought they didn't run for PRs from forks. Still trying to wrap my head around actions.

[jameshadfield]

I'm happy with this direction. If you could address the following we can get this merged ASAP:

• add ${{ matrix.node }} to node-version
• rename the fix npm rule
• rebase onto master, which now has 0 lint errors, so the lint job should pass
• modify the test rule to "test": "jest test/*.js", so that only unit tests are run and thus the job passes. An alternate solution would be to modify the job description in the workflow.
• Confirm that the tests for a PR should be run on a checked-out codebase which is itself a merge of the PR into master, as it is currently doing.

[bcoe]

i.e. you're testing the world that would exist were the PR to me merged

I think this is good default behavior, because you need to know that your PR doesn't fail tests if it's landed on top of master.

In practice, on my team's repos, we require that you're up to date with master before you can merge:

So at any given time, it's equivalent to the PR having being rebased with master prior to testing.

---

I think I've addressed the other review 👌

[bcoe]

P.S. Do you know why the CI is running on this PR

I'm not 100% sure, I could have sworn it needed to be blessed by the maintainer by being merged first (maybe the fact that you forked the branch had an effect?).

[jameshadfield]

Fantastic. Going to merge now & then we can add in further tests etc in later PRs. I'll also move us away from TravisCI by creating a workflow which only runs when the release branch changes, but that's not pressing. Thanks again!