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.

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

feat: warn when there are multiple configs #11922

Merged
merged 41 commits into from Oct 6, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
b85b26b
chore: throw when there are multiple configs
jankaifer Oct 2, 2021
4dff740
fix: allowed package.json without 'jest' as valid config
jankaifer Oct 2, 2021
227f7d4
test: unit test for multiple configs error in resolver
jankaifer Oct 2, 2021
a678d28
Apply suggestions from code review
jankaifer Oct 4, 2021
0f81fe4
test: fixed integration test
jankaifer Oct 4, 2021
b6ff8b8
tests: fixed config issues in old tests
jankaifer Oct 4, 2021
27fdaff
feat: multiple jest.config.ext files are not allowed
jankaifer Oct 4, 2021
0d15a1f
test: added e2e test for --config override
jankaifer Oct 4, 2021
eccf953
fix: more explicit expression
jankaifer Oct 5, 2021
3d71585
fix: added more real-life package.json example
jankaifer Oct 5, 2021
4cca511
Merge branch 'jankaifer/warn-multiple-configs-found' of github.com:Ja…
jankaifer Oct 5, 2021
752b1f9
test: added snapshot tests for multiple errors
jankaifer Oct 5, 2021
02b8ef2
test: moved snapshots from unit tests to e2e tests
jankaifer Oct 5, 2021
cd7c9c9
test: fixed rootDir replacements in snapshots
jankaifer Oct 5, 2021
9e13ff2
Apply suggestions from code review
jankaifer Oct 5, 2021
f30ab95
added missing import
jankaifer Oct 5, 2021
b931029
Change from CR
jankaifer Oct 5, 2021
f9c515e
improved error message
jankaifer Oct 5, 2021
3b1c5b8
added test for --config error surpresion
jankaifer Oct 5, 2021
02338fd
added more real life package.json
jankaifer Oct 5, 2021
2ce0401
added more assertions for multiple config errors
jankaifer Oct 5, 2021
ac00510
improved formatting of error message
jankaifer Oct 5, 2021
82d3e44
fixed sample jest configs in e2e tests
jankaifer Oct 5, 2021
db8c90c
wip: changed from error to warning
jankaifer Oct 5, 2021
ab05377
fixed unit tests for warning
jankaifer Oct 5, 2021
df0f3a5
updated snapshots
jankaifer Oct 5, 2021
1f17acc
Update CHANGELOG.md
SimenB Oct 6, 2021
2d6efff
Update packages/jest-config/src/resolveConfigPath.ts
jankaifer Oct 6, 2021
3682947
Merge branch 'main' into jankaifer/warn-multiple-configs-found
SimenB Oct 6, 2021
7727e74
add slash import
SimenB Oct 6, 2021
9926edf
no flat in node 10
SimenB Oct 6, 2021
91655ce
rename tests
SimenB Oct 6, 2021
d60356f
slash path in e2e test as well
SimenB Oct 6, 2021
6a52634
skip duplicate warning
SimenB Oct 6, 2021
3505a2c
add copyright header
SimenB Oct 6, 2021
80e52de
make warning yellow
SimenB Oct 6, 2021
8abb1c1
specify key in pkg.json
SimenB Oct 6, 2021
746f353
use wrap
SimenB Oct 6, 2021
cae826c
removed hardcoded filename
jankaifer Oct 6, 2021
1de582b
made skipMultipleConfigWarning optional
jankaifer Oct 6, 2021
b57cbf5
Update packages/jest-config/src/resolveConfigPath.ts
SimenB Oct 6, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions e2e/Utils.ts
Expand Up @@ -176,20 +176,23 @@ interface JestPackageJson extends PackageJson {
}

const DEFAULT_PACKAGE_JSON: JestPackageJson = {
description: 'THIS IS AN AUTOGENERATED FILE AND SHOULD NOT BE ADDED TO GIT',
jest: {
testEnvironment: 'node',
},
};

export const createEmptyPackage = (
directory: Config.Path,
packageJson = DEFAULT_PACKAGE_JSON,
packageJson: PackageJson = DEFAULT_PACKAGE_JSON,
) => {
const packageJsonWithDefaults = {
...packageJson,
description: 'THIS IS AN AUTOGENERATED FILE AND SHOULD NOT BE ADDED TO GIT',
};
fs.mkdirSync(directory, {recursive: true});
fs.writeFileSync(
path.resolve(directory, 'package.json'),
JSON.stringify(packageJson, null, 2),
JSON.stringify(packageJsonWithDefaults, null, 2),
);
};

Expand Down
24 changes: 24 additions & 0 deletions e2e/__tests__/configOverride.test.ts
@@ -0,0 +1,24 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import {getConfig} from '../runJest';

test('reads config from cjs file', () => {
SimenB marked this conversation as resolved.
Show resolved Hide resolved
const {configs} = getConfig(
'config-override',
['--config', 'different-config.json'],
{
skipPkgJsonCheck: true,
},
);

expect(configs).toHaveLength(1);
expect(configs[0].displayName).toEqual({
color: 'white',
name: 'Config from different-config.json file',
});
});
2 changes: 1 addition & 1 deletion e2e/__tests__/dependencyClash.test.ts
Expand Up @@ -18,7 +18,7 @@ const hasteImplModulePath = path

beforeEach(() => {
cleanup(tempDir);
createEmptyPackage(tempDir);
createEmptyPackage(tempDir, {});
SimenB marked this conversation as resolved.
Show resolved Hide resolved
});

// This test case is checking that when having both
Expand Down
10 changes: 10 additions & 0 deletions e2e/config-override/__tests__/test.js
@@ -0,0 +1,10 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

test('dummy test', () => {
expect(1).toBe(1);
});
3 changes: 3 additions & 0 deletions e2e/config-override/different-config.json
@@ -0,0 +1,3 @@
{
"displayName": "Config from different-config.json file"
}
3 changes: 3 additions & 0 deletions e2e/config-override/jest.config.json
@@ -0,0 +1,3 @@
{
"displayName": "Config from json file"
}
1 change: 1 addition & 0 deletions e2e/config-override/package.json
@@ -0,0 +1 @@
{}
4 changes: 1 addition & 3 deletions e2e/esm-config/cjs/package.json
@@ -1,3 +1 @@
{
"jest": {}
SimenB marked this conversation as resolved.
Show resolved Hide resolved
}
{}
4 changes: 1 addition & 3 deletions e2e/esm-config/mjs/package.json
@@ -1,3 +1 @@
{
"jest": {}
}
{}
SimenB marked this conversation as resolved.
Show resolved Hide resolved
38 changes: 38 additions & 0 deletions packages/jest-config/src/__tests__/resolveConfigPath.test.ts
Expand Up @@ -14,6 +14,7 @@ import resolveConfigPath from '../resolveConfigPath';
const DIR = path.resolve(tmpdir(), 'resolve_config_path_test');
const ERROR_PATTERN = /Could not find a config file based on provided values/;
const NO_ROOT_DIR_ERROR_PATTERN = /Can't find a root directory/;
const MULTIPLE_CONFIGS_ERROR_PATTERN = /Multiple configurations found/;

beforeEach(() => cleanup(DIR));
afterEach(() => cleanup(DIR));
Expand Down Expand Up @@ -92,12 +93,49 @@ describe.each(JEST_CONFIG_EXT_ORDER.slice(0))(
resolveConfigPath(path.dirname(relativePackageJsonPath), DIR),
).toBe(absoluteJestConfigPath);

// jest.config.js and package.json with 'jest' cannot be used together

writeFiles(DIR, {[relativePackageJsonPath]: JSON.stringify({jest: {}})});

// absolute
expect(() =>
resolveConfigPath(path.dirname(absolutePackageJsonPath), DIR),
).toThrowError(MULTIPLE_CONFIGS_ERROR_PATTERN);

// relative
expect(() =>
resolveConfigPath(path.dirname(relativePackageJsonPath), DIR),
).toThrowError(MULTIPLE_CONFIGS_ERROR_PATTERN);

expect(() => {
resolveConfigPath(
path.join(path.dirname(relativePackageJsonPath), 'j/x/b/m/'),
DIR,
);
}).toThrowError(NO_ROOT_DIR_ERROR_PATTERN);
});

describe.each(JEST_CONFIG_EXT_ORDER.slice(0))(
`jest.config.%s and jest.config${extension}`,
extension2 => {
if (extension === extension2) return;

const relativeJestConfigPaths = [
`a/b/c/jest.config${extension}`,
`a/b/c/jest.config${extension2}`,
];

writeFiles(DIR, {
[relativeJestConfigPaths[0]]: '',
[relativeJestConfigPaths[1]]: '',
});

// multiple configs here, should throw

expect(() =>
resolveConfigPath(path.dirname(relativeJestConfigPaths[0]), DIR),
).toThrowError(MULTIPLE_CONFIGS_ERROR_PATTERN);
},
);
},
);
62 changes: 55 additions & 7 deletions packages/jest-config/src/resolveConfigPath.ts
Expand Up @@ -57,16 +57,21 @@ const resolveConfigPathByTraversing = (
initialPath: Config.Path,
cwd: Config.Path,
): Config.Path => {
const jestConfig = JEST_CONFIG_EXT_ORDER.map(ext =>
const configFiles = JEST_CONFIG_EXT_ORDER.map(ext =>
path.resolve(pathToResolve, getConfigFilename(ext)),
).find(isFile);
if (jestConfig) {
return jestConfig;
).filter(isFile);
SimenB marked this conversation as resolved.
Show resolved Hide resolved

const packageJson = findPackageJson(pathToResolve);
if (packageJson && hasPackageJsonJestKey(packageJson)) {
configFiles.push(packageJson);
}

if (configFiles.length > 1) {
throw new Error(makeMultipleConfigsError(configFiles));
Copy link
Member

Choose a reason for hiding this comment

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

if we throw it's a breaking change, and we need to wait for Jest 28. Thoughts on making it a warning for now so we can land it, then throw later?

Copy link
Contributor Author

@jankaifer jankaifer Oct 5, 2021

Choose a reason for hiding this comment

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

Sounds reasonable.

Should I hide it behind a flag? Or just delete it and rewrite it for warnings?
This question is related to tests, the code itself is just one line, but tests are completely different.

Copy link
Member

Choose a reason for hiding this comment

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

I'd do console.warn instead of throwing, make the unit tests mock out the console, while the e2e test is essentially the same except for the exit code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolveConfigPath has to be a pure function (no console.warn there), because it is run multiple times.

There is hasDeprecationWarnings: boolean flag somewhere. I will look into it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hasDeprecationWarnings has no text associated with it.

The best solution I found was to propagate shouldLogWarnings. But I'm not sure it is good enough.

Copy link
Contributor Author

@jankaifer jankaifer Oct 5, 2021

Choose a reason for hiding this comment

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

The "second" usage is here: https://github.com/facebook/jest/blob/a22ed6592f772df6b19314d9f06a7285c3b0968b/packages/jest-config/src/index.ts#L287

It's reading jest config to determine if there are any additional projects defined.

And then it is called again for each project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB ^ this one is still not solved.

Copy link
Member

Choose a reason for hiding this comment

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

gave that a whack with 6a52634 (#11922)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so you went with shouldLogWarnings. 👍

Copy link
Member

Choose a reason for hiding this comment

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

yeah, it was either that or store some list of the exact warning we had logged (a Set to dedupe) then only log after all configs were resolved. This seemed cleaner

}

const packageJson = path.resolve(pathToResolve, PACKAGE_JSON);
if (isFile(packageJson)) {
return packageJson;
if (configFiles.length || packageJson) {
jankaifer marked this conversation as resolved.
Show resolved Hide resolved
return configFiles[0] ?? packageJson;
}

// This is the system root.
Expand All @@ -83,6 +88,25 @@ const resolveConfigPathByTraversing = (
);
};

const findPackageJson = (pathToResolve: Config.Path) => {
const packagePath = path.resolve(pathToResolve, PACKAGE_JSON);
if (isFile(packagePath)) {
return packagePath;
}

return undefined;
};

const hasPackageJsonJestKey = (packagePath: Config.Path) => {
const content = fs.readFileSync(packagePath, 'utf8');
try {
return 'jest' in JSON.parse(content);
} catch {
// If package is not a valid JSON
return false;
}
};

const makeResolutionErrorMessage = (
initialPath: Config.Path,
cwd: Config.Path,
Expand All @@ -95,3 +119,27 @@ const makeResolutionErrorMessage = (
`traverse directory tree up, until it finds one of those files in exact order: ${JEST_CONFIG_EXT_ORDER.map(
ext => `"${getConfigFilename(ext)}"`,
).join(' or ')}.`;

const makeMultipleConfigsError = (configPaths: Array<Config.Path>) =>
'● Multiple configurations found:\n\n' +
`Jest found configuration in ${concatenateWords(
configPaths.map(wrapInBackticks),
)}.\n` +
jankaifer marked this conversation as resolved.
Show resolved Hide resolved
'This is not allowed because it is probably a mistake.\n\n' +
'Configuration Documentation:\n' +
'https://jestjs.io/docs/configuration.html\n';

const wrapInBackticks = (word: string) => `\`${word}\``;

const concatenateWords = (words: Array<string>): string => {
if (words.length === 0) {
throw new Error('Cannot concatenate an empty array.');
} else if (words.length <= 2) {
return words.join(' and ');
} else {
return concatenateWords([
words.slice(0, -1).join(', '),
words[words.length - 1],
]);
}
};