Split codecov into separate job, combine coverage of all matrix jobs

[tsibley]

Combining coverage from all matrix jobs is beneficial for any
version-dependent code paths.

Resolves #845.

Co-authored-by: Victor Lin [email protected]

Related issue(s)

The original patch from my comment in #899 (comment) was applied unchanged by @victorlin in #948. In this PR, I correct a small omission in my original patch that didn't provide the Augur source code needed by coverage xml. New PR as #948 was against Victor's personal fork not a shared branch.

Testing

• CI passes
• Coverage works

[codecov]

Codecov Report

Merging #968 (0780990) into master (5916362) will increase coverage by 2.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
+ Coverage   59.34%   61.43%   +2.09%     
==========================================
  Files          42       44       +2     
  Lines        6011     6623     +612     
  Branches     1539     1769     +230     
==========================================
+ Hits         3567     4069     +502     
- Misses       2185     2286     +101     
- Partials      259      268       +9     

Impacted Files	Coverage Δ
augur/util_support/node_data_file.py	100.00% <0.00%> (ø)
augur/errors.py	100.00% <0.00%> (ø)
augur/measurements.py	31.25% <0.00%> (ø)
augur/filter.py	97.01% <0.00%> (+0.78%)	⬆️
augur/validate.py	71.17% <0.00%> (+3.84%)	⬆️
augur/utils.py	73.63% <0.00%> (+8.98%)	⬆️
augur/__init__.py	88.23% <0.00%> (+9.20%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5916362...0780990. Read the comment docs.

[victorlin]

Looks good! Of course, No source for code is pretty descriptive looking back at it now.

It's interesting that there are coverage increases from this change. I thought coverage would only increase if we have version-dependent code paths, but the lines that have changed coverage aren't version-dependent. Do you have an idea what else could explain this?

[victorlin]

Also, I found out you can edit PRs from forks using gh pr checkout. I was able to do that in nextstrain/nextstrain.org#520. Checkout is trivial, a ref for the PR is already available as mentioned in a related Slack comment. But I'm curious how the push auth works, since there isn't a way to edit files on a fork PR base branch via the GitHub website.

[tsibley]

It's interesting that there are coverage increases from this change. … Do you have an idea what else could explain this?

I noticed that too and made a cursory look into it but didn't come to a conclusion. I suspect noise. It'd be good to understand this, but I also think it's most likely benign.

But I'm curious how the push auth works, since there isn't a way to edit files on a fork PR base branch via the GitHub website.

Hmm. Unless gh pr checkout sets up branch-specific auth (which I believe is possible with enough git shenanigans), I suspect GitHub selectively authorizes pushes to the refs/pull/N/head refs based on if the person opening the PR selected "Allow edits from maintainers".

[tsibley]

Hmm, looks like maybe not based on trying it with #948.

To github.com:nextstrain/augur
 ! [remote rejected]   HEAD -> refs/pull/948/head (deny updating a hidden ref)

[victorlin]

@tsibley oh I hadn't enabled Allow edits and access to secrets by maintainers, it's on now if you'd like to try again.

[tsibley]

@victorlin Same issue. The deny updating a hidden ref part implies to me that it's not specifically related to that PR setting but some other thing.

Earlier I tried to test out how gh pr checkout configured the branch for pushes, but I couldn't get it actually working right away because of other issues with how I have gh installed (via snap, which has confinement meaning it didn't have access to my SSH credentials). So I decided it wasn't worth spending a bunch of time fixing that first just to satisfy my curiosity, since I don't otherwise use gh for managing remotes/clones.

If you still have the branch around for nextstrain/nextstrain.org#520, I'd be curious to see the output of this from your local repo:

git config --get-regexp '^branch[.]patch-1[.]'

(If the local branch isn't named patch-1, then adjust.)

[victorlin]

@tsibley I had removed it after making changes on that PR, but just did the following:

% gh pr checkout 520
From https://github.com/nextstrain/nextstrain.org
 * [new ref]         refs/pull/520/head -> patch-1
Switched to branch 'patch-1'

% git config --get-regexp '^branch[.]patch-1[.]'
branch.patch-1.remote origin
branch.patch-1.pushremote origin
branch.patch-1.merge refs/pull/520/head

% git remote show origin
* remote origin
  Fetch URL: https://github.com/nextstrain/nextstrain.org
  Push  URL: https://github.com/nextstrain/nextstrain.org
  HEAD branch: master
...
  Local branches configured for 'git pull':
    master  merges with remote master
    patch-1 merges with remote refs/pull/520/head
  Local ref configured for 'git push':
    master pushes to master (local out of date)

% git commit -m 'testing' --allow-empty

% git push
fatal: The upstream branch of your current branch does not match
the name of your current branch.  To push to the upstream branch
on the remote, use

    git push origin HEAD:refs/pull/520/head

To push to the branch of the same name on the remote, use

    git push origin HEAD

% git push origin HEAD:refs/pull/520/head
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 201 bytes | 201.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
To https://github.com/nextstrain/nextstrain.org
 ! [remote rejected] HEAD -> refs/pull/520/head (deny updating a hidden ref)
error: failed to push some refs to 'https://github.com/nextstrain/nextstrain.org'

Maybe something is different now that the PR is closed. If you're still curious like me, can you open a PR from a personal fork and I can try using gh pr checkout with that?

[tsibley]

@victorlin #971

[victorlin]

@tsibley

% gh pr checkout 971
? Which should be the base repository (used for e.g. querying issues) for this directory? nextstrain/augur
remote: Enumerating objects: 1, done.
remote: Counting objects: 100% (1/1), done.
remote: Total 1 (delta 0), reused 1 (delta 0), pack-reused 0
Unpacking objects: 100% (1/1), 189 bytes | 189.00 KiB/s, done.
From https://github.com/nextstrain/augur
 * [new ref]           refs/pull/971/head -> test-pr-from-fork
Switched to branch 'test-pr-from-fork'

% git config --get-regexp '^branch[.]test-pr-from-fork[.]'
branch.test-pr-from-fork.remote https://github.com/tsibley/augur.git
branch.test-pr-from-fork.pushremote https://github.com/tsibley/augur.git
branch.test-pr-from-fork.merge refs/heads/test-pr-from-fork

% git remote show origin
... nothing with "test-pr-from-fork"

% git commit -m 'testing' --allow-empty
[test-pr-from-fork fd1c0e39] testing

% git push
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 204 bytes | 204.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
To https://github.com/tsibley/augur.git
   242d2879..fd1c0e39  test-pr-from-fork -> test-pr-from-fork

[victorlin]

Also, noting that I can see the README edit button on https://github.com/tsibley/augur/tree/test-pr-from-fork now, that branch only. So it looks like selective push access to the branch (as long as Allow edits and access to secrets by maintainers is enabled and the PR is open?).

EDIT: yeah, I took a closer look and checking that option in #948 makes it seem like it's set:

but it's unchecked upon refreshing the page. Sounds like a GitHub bug, that option should not be shown on closed PRs.

[tsibley]

Yep, so selective authz against the branch in the originating repo. gh pr checkout is initially creating the local branch from the nextstrain/augur refs/pull/971/head ref, but then updating the local branch's config to point at (track) the tsibley/augur refs/heads/test-pr-from-fork ref instead. Neat to know!

(A possibly related thing you might be interested in knowing is that (roughly) GitHub physically stores forks in the same physical repo as the base, which is why you can view commits from a fork in the base repo's context.)