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

[DavidAnson]

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]

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

istanbuljs/istanbuljs@eab82a9

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

[DavidAnson]

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 [email protected] versions according to v8.0.0...v8.0.1 DOES fix the problem: https://github.com/DavidAnson/markdownlint/actions/runs/5676565863

Both yargs updates applied in [email protected] are server-breaking, so this is allowed.

Nothing relevant stands out for me in the release notes:

• https://github.com/yargs/yargs/blob/main/CHANGELOG.md
• https://github.com/yargs/yargs-parser/blob/main/CHANGELOG.md

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

[DavidAnson]

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

[DavidAnson]

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

[bcoe]

@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 yargs/yargs-parser#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]

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:

yargs/yargs-parser@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]

@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]

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]

@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]

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]

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):

• On Ubuntu and macOS, unquoted glob arguments are expanded by the shell
• On Windows, glob arguments are never expanded by the shell
• On ALL platforms, single- and double- quoted arguments on the command-line show up unquoted in a Node app's process.argv
• When quoted arguments are provided to a Node app via an npm script, they show up unquoted in process.argv EXCEPT for single-quoted arguments on Windows which show up single-quoted in process.argv

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]

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:

c8/bin/c8.js

Lines 37 to 45 in a13584d

	foreground(hideInstrumenterArgs(argv), async (done) => {
	try {
	await outputReport(argv)
	} catch (err) {
	console.error(err.stack)
	process.exitCode = 1
	}
	done()
	})

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]

We need to turn #488 (comment) into integration tests IMO, and then fix.