enable coverage for PRs

[tgummerer]

Codecov numbers should be stable enough now to enable them for PRs as well.

[justinvp]

Definitely want to do this!

Aren't we still running a different set of tests between PRs and merges? If so, this may end up destabilizing the coverage numbers (PRs will report a smaller coverage number than merges).

I think we need to make sure we're running the full set of tests on PRs. Maybe that means we run the full tests on the current language runtime version, and report coverage for that for both PRs and merges. And then have merges also run on minimum language runtime version as well, but don't report coverage for those tests? Something like that? Or open to other ideas.

[tgummerer]

I managed to set the base of the PR correctly here now, but the results are still somewhat sad. Codecov seems to get confused that we are building the merge commit, and attributes wrong coverage because of that. Looking at the codecov result, it looks like it's attributing missing coverage to comments, or even lines that can't even be uncovered, e.g.

So I think our options here I think are:

• make sure that the PR is always based on the latest commit on master (that's pretty annoying, and will lead to a lot of rebases on master during PR reviews, which is extra annoying)
• build the PR checks based on the the HEAD commit, instead of the merge commit. I think this is less annoying, but might lead to some surprises at merge time. Not ideal, but better than the option above.
• Don't use the coverage numbers as PR checks

There's also the issue Justin points out above regarding running different tests. We would have to run the macos tests during PRs as well, and either start running the Windows tests during merge (making the merge queue more annoying again), or ignore coverage numbers for windows (that's probably fine?). But there's also things like sdk/go/common/util/ciutil/github_actions.go, which runs different code based on whether we're running during a pull request or in the merge queue, so the coverage numbers for that will always be different.

[mikhailshilkov]

I have a hard time imagining the experience in my head. Is it safe to ship the check (the least bad option, without blocking PRs on it) and live with it for a couple of days?

[tgummerer]

I was planning on discussing this in ideation tomorrow before merging it.

[tgummerer]

I think I got the merge commit thing right now, with there only being small diffs in the code coverage from running a different set of tests in the merge queue and on PR still. However now the status checks don't show up anymore 🤦

I opened an upstream issue to ask about this: codecov/codecov-action#1581

[blampe]

https://app.codecov.io/gh/pulumi/pulumi/pull/17204/indirect-changes

Your GitHub PR is based on ebf838e (older) but coverage is getting computed using d30f4b5 (newer) as its base.

You might try computing a value for commit_parent as something like git merge-base {{ github.base_ref }} {{ github.head_ref }} to find the specific commit you branched from. I would have expected it to do this automatically 🤷

git merge-base origin/master origin/tg/enable-codecov-checks
ebf838e02e36c2549206275c3a40c37f95e1e6c4

[tgummerer]

https://app.codecov.io/gh/pulumi/pulumi/pull/17204/indirect-changes

Your GitHub PR is based on ebf838e (older) but coverage is getting computed using d30f4b5 (newer) as its base.

You might try computing a value for commit_parent as something like git merge-base {{ github.base_ref }} {{ github.head_ref }} to find the specific commit you branched from. I would have expected it to do this automatically 🤷

git merge-base origin/master origin/tg/enable-codecov-checks
ebf838e02e36c2549206275c3a40c37f95e1e6c4

Hrm I gave this a try now, and it's showing the base correctly as 6e9ec22 now, but the coverage data still seems to be broken :( I don't feel like I understand what codecov is doing here at all.

[Zaid-Ajaj]

is this comment outdated now?

[tgummerer]

Indeed, good catch! I've updated it now.