7.1.1-7.1.2 seems to break coverage reporting on macos/ubuntu/windows with Node 10 (only)

[DavidAnson]

• Version: 7.1.2
• Platform: ubuntu-latest/Node 10

https://github.com/DavidAnson/markdownlint runs c8 against master every day. After 10 successful runs with no changes to that branch, it started failing yesterday: https://github.com/DavidAnson/markdownlint/actions?query=workflow%3ACI

I created a fork of master with c8 locked to 7.1.1: https://github.com/DavidAnson/markdownlint/tree/c8711

It passes: https://github.com/DavidAnson/markdownlint/actions/runs/98587182

I created a fork of master with c8 locked to 7.1.2: https://github.com/DavidAnson/markdownlint/tree/c8712

It fails: https://github.com/DavidAnson/markdownlint/actions/runs/98587661

To be clear, that is the only difference between the branches:

pi@claw:~/markdownlint $ git diff c8711..c8712
diff --git a/package.json b/package.json
index 1edc8d0..d16356a 100644
--- a/package.json
+++ b/package.json
@@ -35,7 +35,7 @@
   "devDependencies": {
     "@types/node": "~13.11.1",
     "browserify": "~16.5.1",
-    "c8": "7.1.1",
+    "c8": "7.1.2",
     "cpy-cli": "~3.1.0",
     "eslint": "~6.8.0",
     "eslint-plugin-jsdoc": "~22.1.0",

GitHub aggressively terminates Actions with a failure, but this run demonstrates that the same 7.1.2 that fails on ubuntu-latest/Node 10 passes on ubuntu-latest/Node 14: https://github.com/DavidAnson/markdownlint/actions/runs/98145382

The relevant command is: c8 --check-coverage --branches 100 --functions 100 --lines 100 --statements 100 node test/markdownlint-test.js. In the failure case, it reports 4 lines uncovered: helpers.js | 99.4 | 96.35 | 100 | 99.4 | 156,194,197,554

The corresponding file: https://github.com/DavidAnson/markdownlint/blob/c8712/helpers/helpers.js

The first of the corresponding lines:
https://github.com/DavidAnson/markdownlint/blob/34e2fd057648fda7559185e9aeaa68284f0d359b/helpers/helpers.js#L156

Please let me know if you need anything else from me.

[bcoe]

@DavidAnson I'm confused, I would expect this to exit with 1, because your coverage is below the threshold of 100.

[DavidAnson]

Sorry, I wasn't clear.

Coverage as measured by c8@7.1.1 is 100% across ubuntu/windows/mac and Node 10/12/14. Coverage as measured by c8@7.1.2 is <100% on ubuntu/Node 10 yet 100% on ubuntu/Node 14 per this run: https://github.com/DavidAnson/markdownlint/actions/runs/98145382

Locally (ubuntu/Node 12), it also reports 100% with c8@7.1.2. So why is c8@7.1.2 reporting <100% coverage on ubuntu/Node 10? And if c8 is correctly reporting <100% on ubuntu/Node 10, then why are the 4 lines it says are uncovered all "}" or a comment?

My assumption was that the inconsistent coverage reporting indicated a problem since the code is the same in all cases and I don't think there's any OS/platform dependence. If this is something on my side, please let me know.

[DavidAnson]

I've pushed a commit to the c8712 branch to remove ubuntu-latest from the test matrix. That shows <100% coverage on macos/Node 10 as well. https://github.com/DavidAnson/markdownlint/actions/runs/100359241

I've pushed another commit to that branch to restore ubuntu-latest and remove Node 10. That shows 100% coverage on macos/ubuntu/windows across Node 12/14. https://github.com/DavidAnson/markdownlint/actions/runs/100359827

So why is coverage <100% only under Node 10 with c8@7.1.2 where it was 100% with c8@7.1.1?

[DavidAnson]

For completeness, I pushed another commit to the c8712 branch to get a run of c8@7.1.2 on windows/Node 10. It shows <100% coverage. https://github.com/DavidAnson/markdownlint/actions/runs/100361363

Summarizing: For the same code, c8@7.1.2 shows:

• 100% coverage on macos/ubuntu/windows with Node 12/14
• <100% coverage on macos/ubuntu/windows with Node 10

c8@7.1.1 shows 100% coverage for all of those configurations.

[DavidAnson]

There appears to be only one runtime change to c8 between 7.1.1 and 7.1.2, the addition of the await keyword to lib/commands/report.js: v7.1.1...v7.1.2

I don't see why that would have this kind of effect, but the behavior difference between 7.1.1 and 7.1.2 is 100% reproducible.

[bcoe]

Your coverage is below 100%, for Node 10 in the old (passing) build too:

All files         |    99.9 |    99.24 |     100 |    99.9 |                   

This will have been addressed in newer versions of Node.js, due to fixes in V8 (specifically this addressed edge cases like a single missing line after a throw).

With this await missing, I believe what was happening was that the process was exiting with 0 before --check-coverage set the exit code to 1.

[bcoe]

What I might be tempted to do would be to split the coverage check into its own workflow, like this:

https://github.com/yargs/yargs/blob/master/.github/workflows/coverage.yaml

... aside:

Then only enforce coverage thresholds for the latest Node version (that way as we address bugs in V8, you can enforce thresholds based on the latest and greatest Node.js version). If you use a .nycrc file to set your thresholds, or a nyc stanzain yourpackage.json`, there's actually a badge you can add to your README too.

[DavidAnson]

Ugh, I hadn’t noticed c8 was silently failing on Node 10 all along. :(

Well, if this is a Node 10 bug, then it’s not a c8 bug and we can close this issue.

I hope they fix it as Node 10 is still supported, but I will make other plans w.r.t. CI, perhaps splitting the Workflow as you suggest.

Thanks for looking into this!

[bcoe]

@DavidAnson the V8 patches probably won't make there way all the way up to Node 10 (unfortunately), the tldr; is that the V8 in Node 10 is different enough from the V8 in newer Node versions that it's hard to float these patches... The nice thing about c8, is that it uses logic built right in to the V8 JavaScript engine, the crumby thing about c8 is that it uses logic built right in to the V8 JavaScript engine, so the turn around in patches can be a bit painful.

I hope you find a reasonable workaround 👍 please don't be shy about opening issues on this repo

[DavidAnson]

Just FYI, my thinking is to update the Workflow thusly:

- name: Run All Validations
  if: ${{ matrix.node-version != 10.x }}
  run: npm run ci
- name: Run Tests Only
  if: ${{ matrix.node-version == 10.x }}
  run: npm run test

[bcoe]

@DavidAnson I think it would be good to add a section to the README that covers this topic, it's definitely confusing that the engine matters so much.