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

Jump to bottom

Exclude setup files from coverage #7790

Merged
merged 10 commits into from
Feb 4, 2019
Merged

Exclude setup files from coverage #7790

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8211398
Exclude setup files from coverage
lekterable Feb 3, 2019
233aa27
Update CHANGELOG.md
lekterable Feb 3, 2019
c3c2c39
Merge branch 'master' into setup-files-coverage
lekterable Feb 3, 2019
23de604
Exclude globalSetup and globalTeardown files from coverage report
lekterable Feb 3, 2019
14da76c
Add types to should_instrument.test.js
lekterable Feb 3, 2019
e86a02f
Use normalized config in 'should_instrument.test.js'
lekterable Feb 3, 2019
5f5b1e2
Update CHANGELOG.md
lekterable Feb 3, 2019
2ef296b
Update e2e tests and snapshots
lekterable Feb 3, 2019
4d8ae5c
Fix linting and typing
lekterable Feb 3, 2019
a3e6782
Add TODO comment
lekterable Feb 4, 2019
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
- `[jest-config]` Allow `moduleFileExtensions` without 'js' for custom runners ([#7751](https://github.com/facebook/jest/pull/7751))
- `[jest-cli]` Load transformers before installing require hooks ([#7752](https://github.com/facebook/jest/pull/7752)
- `[jest-cli]` Handle missing `numTodoTests` in test results ([#7779](https://github.com/facebook/jest/pull/7779))
- `[jest-runtime]` Exclude setup files from coverage report ([#7790](https://github.com/facebook/jest/pull/7790)
lekterable marked this conversation as resolved.
Show resolved Hide resolved

### Chore & Maintenance

Expand Down
46 changes: 41 additions & 5 deletions packages/jest-runtime/src/__tests__/should_instrument.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,26 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import type {Path, ProjectConfig} from 'types/Config';
import type {Options} from '../ScriptTransformer';

import {normalize} from 'jest-config';
import shouldInstrument from '../shouldInstrument';

describe('shouldInstrument', () => {
const defaultFilename = 'source_file.test.js';
const defaultOptions = {
const defaultFilename: Path = 'source_file.test.js';
const defaultOptions: Options = {
collectCoverage: true,
};
const defaultConfig = {
rootDir: '/',
};
const defaultConfig = normalize(
{
rootDir: '/',
},
{},
).options;

describe('should return true', () => {
const testShouldInstrument = (
Expand Down Expand Up @@ -198,5 +206,33 @@ describe('shouldInstrument', () => {

testShouldInstrument(filename, defaultOptions, defaultConfig);
});

it('if file is a globalSetup file', () => {
testShouldInstrument('globalSetup.js', defaultOptions, {
rootDir: '/',
globalSetup: 'globalSetup.js',
});
});

it('if file is globalTeardown file', () => {
testShouldInstrument('globalTeardown.js', defaultOptions, {
rootDir: '/',
globalTeardown: 'globalTeardown.js',
});
});

it('if file is in setupFiles', () => {
testShouldInstrument('setupTest.js', defaultOptions, {
rootDir: '/',
setupFiles: ['setupTest.js'],
});
});

it('if file is in setupFilesAfterEnv', () => {
testShouldInstrument('setupTest.js', defaultOptions, {
rootDir: '/',
setupFilesAfterEnv: ['setupTest.js'],
});
});
});
});
22 changes: 22 additions & 0 deletions packages/jest-runtime/src/shouldInstrument.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,28 @@ export default function shouldInstrument(
return false;
}

if (config.globalSetup === filename) {
return false;
}

if (config.globalTeardown === filename) {
return false;
}

if (
config.setupFiles &&
config.setupFiles.some(setupFile => setupFile === filename)
) {
return false;
}

if (
config.setupFilesAfterEnv &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

setupFilesAfterEnv and setupFiles are normalized to return empty array, so this extra check is not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thymikee I think they are not normalized for unit tests, just checked and got TypeError: Cannot read property 'some' of undefined, do you want me to leave the check here or provide a default value somewhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, that's because shouldInstrument test file is not typed. Wanna try to add @flow pragma to it and make it use normalized config as it actually expects? If that's too tedious, just pass default values for these specific tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah sure, I'll try to take care of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thymikee I've added the pragma and types, how would I make it use the normalized config tho?

Copy link
Contributor Author

@lekterable lekterable Feb 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The thing is that currently, test cases overwrite the default config when they pass their own config object to testShouldInstrument so it still throws a Cannot read property 'some' of undefined error. How about making the testShouldInstrument in should_instrument.test.js merge the default config and the one from args instead of overwriting it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldInstrument expects every config to be normalized. As I said, it's probably to much of a hassle and not in scope of this PR, so I'm good with leaving this as is (although it seems like an area for some small speedup, so definitely worth following up 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.

agreed, I will try to take care of this in a new PR, thanks for the help!

Copy link
Member

Choose a reason for hiding this comment

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

Could you add TODO comments so we don't forget?

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 comment added

config.setupFilesAfterEnv.some(setupFile => setupFile === filename)
) {
return false;
}

if (MOCKS_PATTERN.test(filename)) {
return false;
}
Expand Down