-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(vitest): Define nested CAC options #3983
Conversation
|
✅ Deploy Preview for fastidious-cascaron-4ded94 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a better way to do these using cac
directly. Could it be configured to understand the object structures?
Regarding the tests, I wasn't sure how to proceed since currently there aren't any tests to differentiate between CLI and config file arguments so I'm not sure it's something the Vitest team wants.
We have some CLI flag tests at test/config
but I'm not sure how this could be implemented there. We would need a way to print the resolved values of the test case and compare those depending on what CLI/config values were passed.
thresholdAutoUpdate
CLI argument to boolean (fix #3962)
If I'm not mistaken, I think just doing If |
@AriPerkkio Ok so I found out what the problem actually was. I haven't actually confirmed this but after digging through the source it seems Because of this, it seems this also affects any other property that uses dot notation and is not explicitly defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks better and seems to work. Should we do this to all configuration options, @sheremet-va?
Here's test case that you can use to validate these. You might need to adjust some other files under test/config
as well:
// vitest/test/config/test/options.test.ts
import { expect, test } from 'vitest'
import * as testUtils from '../../test-utils'
function runVitestCli(...cliArgs: string[]) {
return testUtils.runVitestCli('run', 'fixtures/test/log-config.test.ts', ...cliArgs)
}
test('--coverage', async () => {
const { stdout } = await runVitestCli('--coverage')
expect(stdout).toMatch('coverage.enabled true')
})
test('--coverage.thresholdAutoUpdate', async () => {
const { stdout } = await runVitestCli('--coverage.thresholdAutoUpdate=true')
expect(stdout).toMatch('coverage.thresholdAutoUpdate true')
})
test('--coverage.thresholdAutoUpdate=false', async () => {
const { stdout } = await runVitestCli('--coverage.thresholdAutoUpdate=false')
expect(stdout).toMatch('coverage.thresholdAutoUpdate false')
})
// vitest/test/config/fixtures/test/log-output.test.ts
import { test } from 'vitest'
import type { UserConfig } from 'vitest/config'
/* eslint-disable no-console, unused-imports/no-unused-vars */
test('logs resolved configuration', async () => {
// @ts-expect-error -- internal
const { snapshotOptions, ...config }: UserConfig['test'] = globalThis.__vitest_worker__.config
// Log options that are tested
console.log('coverage.enabled', config.coverage?.enabled)
console.log('coverage.thresholdAutoUpdate', config.coverage?.thresholdAutoUpdate)
})
But how does it look in the help menu? If it's displayed correctly, I'm fine with adding all options to CLI. |
It's displayed correctly.
If listing all possible options makes |
So should I filter dot-notation options? |
Shouldn't this be a concern for |
Sure, we can then check if enabling them doesn't cause too verbose
Feel free to implement test cases as you see best. The example shown above is printing Vitest's resolved configuration values, which means it will read CLI and configuration files and resolve final configuration object. |
I think I did all the nested options, I changed the description on some of them where it makes sense to have a more compact description or a description with a link since CLIs don't have anchor tags. I also thought about what you said regarding the verbose output so I went ahead and omitted the nested options by default but enabled them when the Let me know once you guys think this looks good so I can start writing the tests and updating the docs. |
Filtering options from For the implementation I think we should structure the cli
.version(version)
// Options shown in --help
.option('-r, --root <path>', 'Root path')
.option('-c, --config <path>', 'Path to config file')
.option('--coverage', 'Enable coverage report')
...
// Options hidden from --help. These are defined for type resolving.
.option('--coverage.provider [name]', 'hide-from-help')
.option('--coverage.enabled', 'hide-from-help')
... Then in the |
I disagree, when using the verbose option in any program, the user expects the output to be exactly that; verbose. I think the combination of the links and disabling the verbose output by default, such that it won't ever get in the way of the user unless they explicitly choose to enable it, makes for very sensible defaults.
I agree since it's probably easier to visually scan for the option you're looking for.
This is a good idea if we do decide to go with the route of completely disabling the nested options from What do you think? |
Is Instead of adding a verbose mode for |
Yeah I've never run into a CLI where
This is why I used an environment variable, from what I could gather on the docs and my own testing, I think your idea of using |
Here's minimal example for filtering import { cac } from "cac";
const cli = cac("repro");
cli
.version("1.2.3")
.option("-r, --root <path>", "Root path")
.option("-u, --update", "Update snapshot")
.option(
"--coverage",
"Enable coverage report. Use --help --coverage to see more options"
)
.option("--coverage.reporter [name]", "Choose coverage reporter")
.help((info) => {
const options = info.find((current) => current.title === "Options");
if (typeof options !== "object") return info;
const helpIndex = process.argv.findIndex((arg) => arg === "--help");
const subCommand = process.argv[helpIndex + 1];
// Filter out options that are not related to the sub command
if (subCommand && subCommand.startsWith("--")) {
options.body = options.body
.split("\n")
.filter((line) => line.trim().startsWith(subCommand))
.join("\n");
}
// No sub-command, filter out options with dot-notation
else {
options.body = options.body
.split("\n")
.filter((line) => /--\S+\./.test(line) === false)
.join("\n");
}
return info;
});
cli.parse();
|
Thanks for the great example! And while it's unfortunate we have to directly use I built on your example and added a few things (I already pushed them so you can check it out):
I may have gone a little overboard haha. Anyway, let me know your thoughts! |
I don't like using |
What do you suggest? Maybe |
Maybe |
f3e10ac
to
2bd8bbc
Compare
Done! Sorry for the delay and the force push, I messed up the git history and had to force push otherwise I would commit like 700 different changes! Do we just need the tests now? |
Ideally, yes. And mention this in docs. I also see that browser tests are failing with this change - are you sure all CLI arguments are correct? |
Done.
I forgot to put |
thresholdAutoUpdate
CLI argument to boolean (fix #3962)
packages/vitest/src/node/cli.ts
Outdated
return info | ||
} | ||
|
||
if (subcommands.length === 1 && subcommands[0] === '--expand-help') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually documented anywhere? I can't find how users should know about this command
…lean when using the CLI
…RBOSE environment variable
0dee39b
to
3c5acff
Compare
I thought of different ways of addressing this but after realizing
cac
doesn't provide a native solution to transform option values with dot notation I thought the best way was to convert it to a boolean only if the string is either"true"
or"false"
and just ignore it if it's something different since Vitest doesn't seem to do runtime type checking.I also thought about implementing this at the provider level but it'd mean the code would have to be duplicated for both v8 and istanbul.
It also doesn't seem like something the provider should be concerned with.
Regarding the tests, I wasn't sure how to proceed since currently there aren't any tests to differentiate between CLI and config file arguments so I'm not sure it's something the Vitest team wants.
I also think this is a bigger problem that includes other dot-notation based options like
coverage.enabled
and I'd like to hear your guys' opinions on this because it doesn't seem to be somethingcac
can currently solve natively.StackBlitz demonstrating that this fixes #3962 by using my own fork of this pull request: https://stackblitz.com/edit/vitest-dev-vitest-ecfukt