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

fix: display test duration even if time is mocked out #7264

Merged
merged 5 commits into from
Oct 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
- `[jest-changed-files]` Return correctly the changed files when using `lastCommit=true` on Mercurial repositories ([#7228](https://github.com/facebook/jest/pull/7228))
- `[babel-jest]` Cache includes babel environment variables ([#7239](https://github.com/facebook/jest/pull/7239))
- `[jest-config]` Use strings instead of `RegExp` instances in normalized configuration ([#7251](https://github.com/facebook/jest/pull/7251))
- `[jest-circus]` Make sure to display real duration even if time is mocked ([#7264](https://github.com/facebook/jest/pull/7264))

### Chore & Maintenance

Expand Down
11 changes: 11 additions & 0 deletions e2e/__tests__/override-globals.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,14 @@ test('overriding native promise does not freeze Jest', () => {
const run = runJest('override-globals');
expect(run.stderr).toMatch(/PASS __tests__(\/|\\)index.js/);
});

test('has a duration even if time is faked', () => {
const regex = /works well \((\d+)ms\)/;
const {stderr} = runJest('override-globals', ['--verbose']);

expect(stderr).toMatch(regex);

const [, duration] = stderr.match(regex);

expect(Number(duration)).toBeGreaterThan(0);
});
5 changes: 4 additions & 1 deletion e2e/override-globals/package.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
{
"jest": {
"testEnvironment": "node"
"testEnvironment": "node",
"setupFiles": [
"<rootDir>/setup.js"
]
}
}
2 changes: 2 additions & 0 deletions e2e/override-globals/setup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Date.now = () => 0;
process.hrtime = () => [0, 5000];
2 changes: 1 addition & 1 deletion packages/jest-circus/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ export const callAsyncCircusFn = (

export const getTestDuration = (test: TestEntry): ?number => {
const {startedAt} = test;
return startedAt ? Date.now() - startedAt : null;
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously it was missing completely when time was fakes as startedAt === 0 which was falsy. using hrtime it changed to always say 0ms. The babel plugin ensures we reference the real process.hrtime now

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the millisecond resolution from Date.now enough or is there any other reason to use process.hrtime?

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory it's more accurate, although I don't think that matters in practice.
Any reason to prefer Date?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't need a more accurate precision than milliseconds we can stick to Date. I'd prefer it for simplicity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed back, but fixed the startedAt === 0 case

return typeof startedAt === 'number' ? Date.now() - startedAt : null;
};

export const makeRunResult = (
Expand Down
8 changes: 6 additions & 2 deletions packages/jest-util/src/install_common_globals.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@ const DTRACE = Object.keys(global).filter(key => key.startsWith('DTRACE'));
export default function(globalObject: Global, globals: ConfigGlobals) {
globalObject.process = createProcessObject();

// Keep a reference to "Promise", since "jasmine_light.js" needs it.
globalObject[globalObject.Symbol.for('jest-native-promise')] = Promise;
const symbol = globalObject.Symbol;
// Keep a reference to some globals that Jest needs
globalObject[symbol.for('jest-native-promise')] = Promise;
globalObject[symbol.for('jest-native-now')] = globalObject.Date.now.bind(
globalObject.Date,
);

// Forward some APIs.
DTRACE.forEach(dtrace => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,34 @@ module.exports = ({template}) => {
const promiseDeclaration = template(`
var Promise = global[Symbol.for('jest-native-promise')] || global.Promise;
`);
const nowDeclaration = template(`
var jestNow = global[Symbol.for('jest-native-now')] || global.Date.now;
`);

return {
name: 'jest-native-promise',
name: 'jest-native-globals',
visitor: {
ReferencedIdentifier(path, state) {
if (path.node.name === 'Promise' && !state.injectedPromise) {
state.injectedPromise = true;
if (path.node.name === 'Promise' && !state.jestInjectedPromise) {
state.jestInjectedPromise = true;
path
.findParent(p => p.isProgram())
.unshiftContainer('body', promiseDeclaration());
}
if (
path.node.name === 'Date' &&
path.parent.property &&
path.parent.property.name === 'now'
) {
if (!state.jestInjectedNow) {
state.jestInjectedNow = true;
path
.findParent(p => p.isProgram())
.unshiftContainer('body', nowDeclaration());
}

path.parentPath.replaceWithSourceString('jestNow');
}
},
},
};
Expand Down
4 changes: 2 additions & 2 deletions scripts/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,9 @@ function buildFile(file, silent) {

if (INLINE_REQUIRE_BLACKLIST.test(file)) {
// The modules in the blacklist are injected into the user's sandbox
// We need to guard `Promise` there.
// We need to guard some globals there.
options.plugins.push(
require.resolve('./babel-plugin-jest-native-promise')
require.resolve('./babel-plugin-jest-native-globals')
);
} else {
// Remove normal plugin.
Expand Down