From eb440602d79396eebbf3f8a509f60f3e03417440 Mon Sep 17 00:00:00 2001 From: Alice Date: Thu, 17 Nov 2022 13:41:18 -0500 Subject: [PATCH] fix(cli): ensure that argument order is correct for Jest (#3827) This ensures that CLI arguments are ordered correctly when we pass them to Jest. In particular, we want to ensure that all flags of the type `--maxWorkers=0` and the like are _before_ any flags which we don't know about, like `my-spec-file.ts` (the latter being actually a match pattern for which spec files to run). In other words, if the user passes arguments like this: ``` --e2e foo.spec.ts ``` or ``` --e2e -- foo.spec.ts ``` We need to ensure we use something like ``` ["--e2e", "foo.spec.ts"] ``` to generate the Jest config. The wrinkle is that after we've parsed our known CLI arguments (in the `cli` module) we do some addition modification of the arguments, based on values set in the `Config`, in the `buildJestArgv` function. In particular, we'll look to see if a `maxWorkers` flag is already passed and, if not, we add one to the args before they're used to build the Jest configuration. Before this change that would result in an array like this being passed to Jest: ``` ["--e2e", "foo.spec.ts", "--maxWorkers=0"] ``` because we simply stuck the new `--maxWorkers` arg right on the end of the already-combined array (combined because it's basically `knownArgs + unknownArgs`). No good! Why no good? Well, basically because Jest works best if the filename matches are at the end. It's behavior if they're _not_ is, unfortunately, inconsistent and it will work sometimes, but it (as far as I have tested it!) _always_ works if the pattern is at the end. So in order to provide a guarantee the pattern is at the end, we modify a copy of `knownArgs` with flags like this and then produce a new, combined array to pass to Jest when we're all done, so it looks like this instead: ``` ["--e2e", "--maxWorkers=0", "foo.spec.ts"] ``` Much better! --- src/testing/jest/jest-config.ts | 22 ++++++++++++++----- src/testing/jest/test/jest-config.spec.ts | 26 ++++++++++++++++++++++- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/testing/jest/jest-config.ts b/src/testing/jest/jest-config.ts index 2e3d31256c8..e586cc2eb31 100644 --- a/src/testing/jest/jest-config.ts +++ b/src/testing/jest/jest-config.ts @@ -46,15 +46,27 @@ function getLegacyJestOptions(): Record { export function buildJestArgv(config: d.ValidatedConfig): Config.Argv { const yargs = require('yargs'); - const args = [...config.flags.knownArgs.slice(), ...config.flags.unknownArgs.slice()]; + const knownArgs = config.flags.knownArgs.slice(); - if (!args.some((a) => a.startsWith('--max-workers') || a.startsWith('--maxWorkers'))) { - args.push(`--max-workers=${config.maxConcurrentWorkers}`); + if (!knownArgs.some((a) => a.startsWith('--max-workers') || a.startsWith('--maxWorkers'))) { + knownArgs.push(`--max-workers=${config.maxConcurrentWorkers}`); } if (config.flags.devtools) { - args.push('--runInBand'); - } + knownArgs.push('--runInBand'); + } + + // we combine the modified args and the unknown args here and declare the + // result read only, providing some typesystem-level assurance that we won't + // mutate it after this point. + // + // We want that assurance because Jest likes to have any filepath match + // patterns at the end of the args it receives. Those args are going to be + // found in our `unknownArgs`, so while we want to do some stuff in this + // function that adds to `knownArgs` we need a guarantee that all of the + // `unknownArgs` are _after_ all the `knownArgs` in the array we end up + // generating the Jest configuration from. + const args: ReadonlyArray = [...knownArgs, ...config.flags.unknownArgs]; config.logger.info(config.logger.magenta(`jest args: ${args.join(' ')}`)); diff --git a/src/testing/jest/test/jest-config.spec.ts b/src/testing/jest/test/jest-config.spec.ts index b4b5799486c..5b553ef7f61 100644 --- a/src/testing/jest/test/jest-config.spec.ts +++ b/src/testing/jest/test/jest-config.spec.ts @@ -42,7 +42,7 @@ describe('jest-config', () => { expect(jestArgv.maxWorkers).toBe(2); }); - it('forces --maxWorkers=4 arg when e2e test and --ci', () => { + it('passes default --maxWorkers=0 arg when e2e test and --ci', () => { const args = ['test', '--ci', '--e2e']; const config = mockValidatedConfig(); config.flags = parseFlags(args); @@ -55,6 +55,20 @@ describe('jest-config', () => { expect(jestArgv.maxWorkers).toBe(0); }); + it('passed default --maxWorkers=0 arg when e2e test and --ci with filepath', () => { + const args = ['test', '--ci', '--e2e', '--', 'my-specfile.spec.ts']; + const config = mockValidatedConfig(); + config.flags = parseFlags(args); + + expect(config.flags.args).toEqual(['--ci', '--e2e', '--', 'my-specfile.spec.ts']); + expect(config.flags.unknownArgs).toEqual(['--', 'my-specfile.spec.ts']); + + const jestArgv = buildJestArgv(config); + expect(jestArgv.ci).toBe(true); + expect(jestArgv.maxWorkers).toBe(0); + expect(jestArgv._).toEqual(['my-specfile.spec.ts']); + }); + it('pass --maxWorkers=2 arg to jest', () => { const args = ['test', '--maxWorkers=2']; const config = mockValidatedConfig(); @@ -200,5 +214,15 @@ describe('jest-config', () => { const jestArgv = buildJestArgv(config); expect(jestArgv.json).toBe(true); + // the `_` field holds any filename pattern matches + expect(jestArgv._).toEqual(['my-component.spec.ts']); + }); + + it('should parse multiple file patterns', () => { + const args = ['test', '--spec', '--json', '--', 'foobar/*', 'my-spec.ts']; + const jestArgv = buildJestArgv(mockValidatedConfig({ flags: parseFlags(args) })); + expect(jestArgv.json).toBe(true); + // the `_` field holds any filename pattern matches + expect(jestArgv._).toEqual(['foobar/*', 'my-spec.ts']); }); });