Skip to content
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.

Sign up for GitHub

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(cli): ensure that argument order is correct for Jest #3827

Merged
merged 1 commit into from
Nov 17, 2022
Merged

fix(cli): ensure that argument order is correct for Jest #3827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions src/testing/jest/jest-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,27 @@ function getLegacyJestOptions(): Record<string, boolean | number | string> {
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<string> = [...knownArgs, ...config.flags.unknownArgs];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnknownArgs doesn't get mutated anywhere, correct? I just noticed we created a copy before with slice but not doing that here. Probably okay, just erring on the side of caution

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik the .slice call that was there before was redundant, which is why I removed it.

If I do

let xs = [1,2,3]

and then

let ys = [...xs]

all the things in xs (which is string[]) are primitive types, so they will be copied out by value when the spread operator is applied to them. so calling .slice() or .concat() on xs as it's being spread into ys I think will only create an extra intermediary array which will be dropped later on. In other words I think:

let xs = [1,2,3]
let ys = [...xs.concat()]

is basically equivalent to

let xs = [1,2,3]
let tmp = xs.concat()
let ys = [...tmp]

We don't need that tmp array, and all it will do is increase GC pressure (by a likely negligible amount haha).

Does that make sense / are there some bits of JS array semantics I'm not grokking correctly? always possible in JS land, haha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope you're good. Just making sure it was an intentional change!


config.logger.info(config.logger.magenta(`jest args: ${args.join(' ')}`));

Expand Down
26 changes: 25 additions & 1 deletion src/testing/jest/test/jest-config.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
Expand Down Expand Up @@ -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']);
});
});