bcoe / c8

output coverage reports using Node.js' built in coverage
ISC License
2k stars 91 forks source link

c8 upgrade from 8.0.0 to 8.0.1 seems to break --exclude on Windows when value is single-quoted in npm script #488

Open DavidAnson opened 1 year ago

DavidAnson commented 1 year ago

c8 --check-coverage --branches 100 --functions 100 --lines 100 --statements 100 --exclude 'test/**' --exclude 'micromark/**' npm test

Failing CI run for this change due to lack of 100% coverage for excluded test files:

https://github.com/DavidAnson/markdownlint/actions/runs/5664137721

Passing CI run with 8.0.0 for comparison:

https://github.com/DavidAnson/markdownlint/actions/runs/5664138479

bcoe commented 1 year ago

There weren't any significant changes to logic between 8.0.0 and 8.0.1, just a transitive dependency update:

https://github.com/istanbuljs/istanbuljs/commit/eab82a9aeff140a8fd2981c7f872830c985d479f

Could you use npm overrides to experiment with the older version of make-dir, and see if this is the culprit.

DavidAnson commented 1 year ago

Overriding make-dir to 3.1.0 (previous version) does NOT fix the problem: https://github.com/DavidAnson/markdownlint/actions/runs/5676526442

Overriding yargs and yargs-parser to their c8@8.0.0 versions according to https://github.com/bcoe/c8/compare/v8.0.0...v8.0.1 DOES fix the problem: https://github.com/DavidAnson/markdownlint/actions/runs/5676565863

Both yargs updates applied in c8@8.0.1 are server-breaking, so this is allowed.

Nothing relevant stands out for me in the release notes:

It would be nice for c8 to add a test to cover this scenario.

DavidAnson commented 1 year ago

Oh, you're a yargs maintainer! That should make this easier. :)

DavidAnson commented 1 year ago

FYI, overriding ONLY yargs-parser (to ^20.2.9) also produces a successful run: https://github.com/DavidAnson/markdownlint/actions/runs/5676622211

bcoe commented 1 year ago

@DavidAnson sorry to keep giving you work, but out of curiosity does it break with yargs pinned to 21.0.0. I'm suspicious of https://github.com/yargs/yargs-parser/pull/407 and quoting being the issue.

It might also be worth trying:

c8 --check-coverage --branches 100 --functions 100 --lines 100 --statements 100 --exclude test/** --exclude micromark/** npm test

I'm wondering if, in Windows only, perhaps the tokens are being parsed as 'test/**' rather than test/**, and then not properly handled by globbing.

DavidAnson commented 1 year ago

I think you meant to override and force yargs-parser to 21.0.0? That fails:

https://github.com/DavidAnson/markdownlint/actions/runs/5697905729

Overriding it to 20.2.9, the previous version works:

https://github.com/DavidAnson/markdownlint/actions/runs/5697924507

Here’s the diff of those two releases; it includes the commit you called out:

https://github.com/yargs/yargs-parser/compare/yargs-parser-v20.2.9...yargs-parser-v21.0.0

Removing the single quotes around the glob paths on 21.0.0 converts it to working:

https://github.com/DavidAnson/markdownlint/actions/runs/5697947307

However, my understanding is that single-quoting glob paths as I do is a best practice because without the quotes, the user’s shell expands each glob and running on different shells/platforms can lead to unpredictable behavior. By single-quoting the path, it’s passed to the tool as-is and the tool can apply consistent globbing logic.

DavidAnson commented 1 year ago

@bcoe, I think you're right about that commit. Previously, yargs-parser would remove quotes from each val in processValue but now it only does so if the top-level input was a string (see inputIsString). The c8 scenario passes argv as an array (via process.argv), so quotes are no longer removed (on all platforms).

On macOS (and presumably Linux), the npm script command line with single quotes comes in via process.argv with the quotes already removed (and the glob string untouched). What I suspect is that on Windows (sorry, can't try it easily), the quotes are not removed from process.argv and are now passed through as-is by yargs-parser to the glob engine which fails to match any paths with those quotes in the glob.

DavidAnson commented 1 year ago

Ironically, it turns out I don't need either --exclude in this scenario, so I can simplify the command to c8 --100 and avoid the issue entirely. (The default exclusion includes test/** which I discovered during the course of looking into this.)

bcoe commented 1 year ago

@DavidAnson I think we're perhaps thinking the same thing, which is that the input still has quotes when passed to the glob engine? I think we'd probably be safe to do some preprocessing in c8 and remove wrapping quotes, what do you think?

DavidAnson commented 1 year ago

This kind of platform-specific stuff scares me. I maintain a CLI as well and include some pretty intricate directions for getting consistent behavior for arguments, but this issue makes me worry I missed something. https://github.com/DavidAnson/markdownlint-cli2#command-line

I don't use a command-line parsing library in that project, but I thought this was one of the big reasons to do so. I'm bummed to find using something as prominent as yargs isn't an easy solution.

DavidAnson commented 1 year ago

Ooookay. This was bothering me, so I did more experimentation. Here's receipts: https://github.com/DavidAnson/markdownlint-cli2/actions/runs/5734878989/job/15541764773

I'd summarize the key points as follows (as observed in GitHub's Actions environment):

It's this last point that led to the failure behind this issue as my scenario invokes c8 via an npm script on Windows and single-quotes its glob arguments.

Do with this as you wish. :) My recommendation for markdownlint-cli2 was already to use double quotes and I think that's the most consistent pattern, so it stands as-is.

fasttime commented 1 year ago

I think that the best way to deal with this kind of inconsistencies (both glob patterns and quotes in the command line) would be passing { shell: true } to foreground-child, I mean here:

https://github.com/bcoe/c8/blob/a13584d5be5259ebb6a00455d352c3e8b16006de/bin/c8.js#L37-L45

That's basically the reason why shell exists as a Node.js spawn option. But I doubt that this solution would be desirable in all situations.

bcoe commented 11 months ago

We need to turn https://github.com/bcoe/c8/issues/488#issuecomment-1661505776 into integration tests IMO, and then fix.