Skip to content

Commit

Permalink
fix(cli): fix bug with parsing --fooBar=baz type CLI flags
Browse files Browse the repository at this point in the history
This fixes a bug with how these flags were being parsed which was
messing with Jest. Basically, the issue related to how the `knownArgs`
array on the `ConfigFlags` object was being populated. For CLI key-value
args of the format `--argName=value` the whole string
`"--argName=value"` was being added _as well as_ the value string
`"value"`, which had the effect of passing duplicate args to Jest.

This refactors how such arguments are handled to always parse apart the
argument name and the value, adding only the argument name and then the
argument value, so that if you pass, for instance,
`--outputFile=output.json`, the args passed to Jest will look like

```ts
['--outputFile', 'output.json']
```

See #3471 and #3481 for more details
  • Loading branch information
alicewriteswrongs committed Jul 20, 2022
1 parent 835e00f commit a924353
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 13 deletions.
59 changes: 47 additions & 12 deletions src/cli/parse-flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,13 @@ export const parseFlags = (args: string[], sys?: CompilerSystem): ConfigFlags =>
}
}

flags.unknownArgs = flags.args.filter((arg: string) => {
return !flags.knownArgs.includes(arg);
});
// to find unknown / unrecognized arguments we filter `args`, including only
// arguments whose normalized form is not found in `knownArgs`. `knownArgs`
// is populated during the call to `parseArgs` above. For arguments like
// `--foobar` the string `"--foobar"` will be added, while for more
// complicated arguments like `--bizBoz=bop` or `--bizBoz bop` just the
// string `"--bizBoz"` will be added.
flags.unknownArgs = flags.args.filter((arg: string) => !flags.knownArgs.includes(parseEqualsArg(arg)[0]));

return flags;
};
Expand Down Expand Up @@ -259,17 +263,18 @@ const getValue = (

let value: string | undefined;
let matchingArg: string | undefined;

args.forEach((arg, i) => {
if (arg.startsWith(`--${dashCaseName}=`) || arg.startsWith(`--${configCaseName}=`)) {
value = getEqualsValue(arg);
matchingArg = arg;
// our argument was passed at the command-line in the format --argName=arg-value
[matchingArg, value] = parseEqualsArg(arg);
} else if (arg === `--${dashCaseName}` || arg === `--${configCaseName}`) {
// the next value in the array is assumed to be a value for this argument
value = args[i + 1];
matchingArg = arg;
} else if (alias) {
if (arg.startsWith(`-${alias}=`)) {
value = getEqualsValue(arg);
matchingArg = arg;
[matchingArg, value] = parseEqualsArg(arg);
} else if (arg === `-${alias}`) {
value = args[i + 1];
matchingArg = arg;
Expand All @@ -287,13 +292,43 @@ interface CLIArgValue {
}

/**
* When a parameter is set in the format `--foobar=12` at the CLI (as opposed to
* `--foobar 12`) we want to get the value after the `=` sign
* Parse an 'equals' argument, which is a CLI argument-value pair in the
* format `--foobar=12` (as opposed to the space-separated format `--foobar
* 12`).
*
* To parse this we split on the `=`, returning the first part as the argument
* name and the second part as the value. We join the value on `"="` in case
* there is another `"="` in the argument.
*
* This function is safe to call with any arg, and can therefore be used as
* an argument 'normalizer'. If CLI argument is not an 'equals' argument then
* the return value will be a tuple of the original argument and an empty
* string `""` for the value.
*
* In code terms, if you do:
*
* @param commandArgument the arg in question
* @returns the value after the `=`
* ```ts
* const [arg, value] = parseEqualsArg("--myArgument")
* ```
*
* Then `arg` will be `"--myArgument"` and `value` will be `""`, whereas if
* you do:
*
*
* ```ts
* const [arg, value] = parseEqualsArg("--myArgument=myValue")
* ```
*
* Then `arg` will be `"--myArgument"` and `value` will be `"myValue"`.
*
* @param arg the arg in question
* @returns a tuple containing the arg name and the value (if present)
*/
const getEqualsValue = (commandArgument: string) => commandArgument.split('=').slice(1).join('=');
export const parseEqualsArg = (arg: string): [string, string] => {
const [originalArg, ...value] = arg.split('=');

return [originalArg, value.join('=')];
};

/**
* Small helper for getting type-system-level assurance that a `string` can be
Expand Down
26 changes: 25 additions & 1 deletion src/cli/test/parse-flags.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type * as d from '../../declarations';
import { LogLevel } from '../../declarations';
import { BOOLEAN_CLI_ARGS, STRING_CLI_ARGS, NUMBER_CLI_ARGS } from '../config-flags';
import { parseFlags } from '../parse-flags';
import { parseEqualsArg, parseFlags } from '../parse-flags';

describe('parseFlags', () => {
let args: string[] = [];
Expand Down Expand Up @@ -190,19 +190,29 @@ describe('parseFlags', () => {
it.each(STRING_CLI_ARGS)('should parse string flag %s', (cliArg) => {
const flags = parseFlags([`--${cliArg}`, 'test-value'], sys);
expect(flags.knownArgs).toEqual([`--${cliArg}`, 'test-value']);
expect(flags.unknownArgs).toEqual([])
expect(flags[cliArg]).toBe('test-value');
});

it.each(STRING_CLI_ARGS)('should parse string flag --%s=value', (cliArg) => {
const flags = parseFlags([`--${cliArg}=path/to/file.js`], sys);
expect(flags.knownArgs).toEqual([`--${cliArg}`, 'path/to/file.js']);
expect(flags.unknownArgs).toEqual([])
expect(flags[cliArg]).toBe('path/to/file.js');
});

it.each(NUMBER_CLI_ARGS)('should parse number flag %s', (cliArg) => {
const flags = parseFlags([`--${cliArg}`, '42'], sys);
expect(flags.knownArgs).toEqual([`--${cliArg}`, '42']);
expect(flags.unknownArgs).toEqual([])
expect(flags[cliArg]).toBe(42);
});

it('should parse --compare', () => {
args[0] = '--compare';
const flags = parseFlags(args, sys);
expect(flags.knownArgs).toEqual(['--compare']);
expect(flags.unknownArgs).toEqual([])
expect(flags.compare).toBe(true);
});

Expand Down Expand Up @@ -574,4 +584,18 @@ describe('parseFlags', () => {
expect(flags.help).toBe(true);
expect(flags.config).toBe('./myconfig.json');
});

describe('parseEqualsArg', () => {
it.each([
['--fooBar=baz', '--fooBar', 'baz'],
['--foo-bar=4', '--foo-bar', '4'],
['--fooBar=twenty=3*4', '--fooBar', 'twenty=3*4'],
['--fooBar', '--fooBar', ''],
['--foo-bar', '--foo-bar', ''],
])('should parse %s correctly', (testArg, expectedArg, expectedValue) => {
const [arg, value] = parseEqualsArg(testArg);
expect(arg).toBe(expectedArg);
expect(value).toBe(expectedValue);
});
});
});
11 changes: 11 additions & 0 deletions src/testing/jest/test/jest-config.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,17 @@ describe('jest-config', () => {
expect(jestArgv.maxWorkers).toBe(2);
});

it('should pass --outputFile=path/to/file', () => {
const args = ['test', '--ci', '--outputFile=path/to/my-file'];
const config = mockValidatedConfig();
config.flags = parseFlags(args, config.sys);
expect(config.flags.args).toEqual(['--ci', '--outputFile=path/to/my-file']);
expect(config.flags.unknownArgs).toEqual([]);
config.testing = {};
const jestArgv = buildJestArgv(config);
expect(jestArgv.outputFile).toBe('path/to/my-file');
});

it('pass --maxWorkers=2 arg when e2e test and --ci', () => {
const args = ['test', '--ci', '--e2e', '--max-workers=2'];
const config = mockValidatedConfig();
Expand Down

0 comments on commit a924353

Please sign in to comment.