From a9243533b3784f3041601248a590ec14268d7c7a Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Wed, 20 Jul 2022 11:44:15 -0400 Subject: [PATCH] fix(cli): fix bug with parsing --fooBar=baz type CLI flags 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 --- src/cli/parse-flags.ts | 59 ++++++++++++++++++----- src/cli/test/parse-flags.spec.ts | 26 +++++++++- src/testing/jest/test/jest-config.spec.ts | 11 +++++ 3 files changed, 83 insertions(+), 13 deletions(-) diff --git a/src/cli/parse-flags.ts b/src/cli/parse-flags.ts index c2db1af329b..cfcc6b89b28 100644 --- a/src/cli/parse-flags.ts +++ b/src/cli/parse-flags.ts @@ -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; }; @@ -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; @@ -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 diff --git a/src/cli/test/parse-flags.spec.ts b/src/cli/test/parse-flags.spec.ts index f63018b4805..df44abda4ee 100644 --- a/src/cli/test/parse-flags.spec.ts +++ b/src/cli/test/parse-flags.spec.ts @@ -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[] = []; @@ -190,12 +190,21 @@ 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); }); @@ -203,6 +212,7 @@ describe('parseFlags', () => { args[0] = '--compare'; const flags = parseFlags(args, sys); expect(flags.knownArgs).toEqual(['--compare']); + expect(flags.unknownArgs).toEqual([]) expect(flags.compare).toBe(true); }); @@ -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); + }); + }); }); diff --git a/src/testing/jest/test/jest-config.spec.ts b/src/testing/jest/test/jest-config.spec.ts index ed4672fe95a..71c7e722fa7 100644 --- a/src/testing/jest/test/jest-config.spec.ts +++ b/src/testing/jest/test/jest-config.spec.ts @@ -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();