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 2 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(/works well \((\d+)ms\)/);
SimenB marked this conversation as resolved.
Show resolved Hide resolved

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];
18 changes: 18 additions & 0 deletions flow-typed/convert-hrtime.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. 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.
*
* @flow
*/

declare module 'convert-hrtime' {
declare export default (
hrtime: [number, number],

This comment was marked as outdated.

) => {
seconds: number,
milliseconds: number,
nanoseconds: number,
};
}
1 change: 1 addition & 0 deletions packages/jest-circus/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"jest-snapshot": "^23.6.0",
"jest-util": "^23.4.0",
"pretty-format": "^23.6.0",
"convert-hrtime": "^2.0.0",
"stack-utils": "^1.0.1"
},
"devDependencies": {
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-circus/src/event_handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ const handler: EventHandler = (event, state): void => {
}
case 'test_start': {
state.currentlyRunningTest = event.test;
event.test.startedAt = Date.now();
event.test.startedAt = process.hrtime();
event.test.invocations += 1;
break;
}
Expand Down
8 changes: 7 additions & 1 deletion packages/jest-circus/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import type {
import {convertDescriptorToString} from 'jest-util';
import isGeneratorFn from 'is-generator-fn';
import co from 'co';
import convertHrtime from 'convert-hrtime';

import StackUtils from 'stack-utils';

Expand Down Expand Up @@ -240,7 +241,12 @@ 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


if (!startedAt) {
return null;
}

return convertHrtime(process.hrtime(startedAt)).milliseconds;
};

export const makeRunResult = (
Expand Down
5 changes: 4 additions & 1 deletion packages/jest-util/src/install_common_globals.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ 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.
// Keep a reference to some globals that Jest needs
globalObject[globalObject.Symbol.for('jest-native-promise')] = Promise;
globalObject[
globalObject.Symbol.for('jest-hrtime')
] = globalObject.process.hrtime.bind(globalObject.process);

// Forward some APIs.
DTRACE.forEach(dtrace => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@ module.exports = ({template}) => {
const promiseDeclaration = template(`
var Promise = global[Symbol.for('jest-native-promise')] || global.Promise;
`);
const hrtimeDeclaration = template(`
var hrtime = global[Symbol.for('jest-hrtime')] || global.process.hrtime;
SimenB marked this conversation as resolved.
Show resolved Hide resolved
`);

return {
name: 'jest-native-promise',
name: 'jest-native-globals',
visitor: {
ReferencedIdentifier(path, state) {
if (path.node.name === 'Promise' && !state.injectedPromise) {
Expand All @@ -25,6 +28,20 @@ module.exports = ({template}) => {
.findParent(p => p.isProgram())
.unshiftContainer('body', promiseDeclaration());
}
if (
path.node.name === 'process' &&
path.parent.property &&
path.parent.property.name === 'hrtime'
) {
if (!state.injectedHrtime) {
state.injectedHrtime = true;
SimenB marked this conversation as resolved.
Show resolved Hide resolved
path
.findParent(p => p.isProgram())
.unshiftContainer('body', hrtimeDeclaration());
}

path.parentPath.replaceWithSourceString('hrtime');
}
},
},
};
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
2 changes: 1 addition & 1 deletion types/Circus.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ export type TestEntry = {|
mode: TestMode,
name: TestName,
parent: DescribeBlock,
startedAt: ?number,
startedAt: ?[number, number],
duration: ?number,
status: ?TestStatus, // whether the test has been skipped or run already
timeout: ?number,
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4152,6 +4152,11 @@ conventional-recommended-bump@^2.0.6:
meow "^4.0.0"
q "^1.5.1"

convert-hrtime@^2.0.0:
version "2.0.0"
resolved "https://registry.yarnpkg.com/convert-hrtime/-/convert-hrtime-2.0.0.tgz#19bfb2c9162f9e11c2f04c2c79de2b7e8095c627"
integrity sha1-Gb+yyRYvnhHC8Ewsed4rfoCVxic=

convert-source-map@^1.1.0, convert-source-map@^1.1.1, convert-source-map@^1.1.3, convert-source-map@^1.4.0, convert-source-map@^1.5.1:
version "1.6.0"
resolved "https://registry.yarnpkg.com/convert-source-map/-/convert-source-map-1.6.0.tgz#51b537a8c43e0f04dec1993bffcdd504e758ac20"
Expand Down