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

jest-circus Timeouts #3760

Merged
merged 2 commits into from
Jun 8, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
13 changes: 0 additions & 13 deletions integration_tests/__tests__/__snapshots__/timeouts-test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,6 @@ Ran all test suites.
`;

exports[`exceeds the timeout 1`] = `
" FAIL __tests__/a-banana.js
● banana

Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

at ../../packages/jest-jasmine2/build/queueRunner.js:53:21

✕ banana

"
`;

exports[`exceeds the timeout 2`] = `
"Test Suites: 1 failed, 1 total
Tests: 1 failed, 1 total
Snapshots: 0 total
Expand Down
6 changes: 3 additions & 3 deletions integration_tests/__tests__/timeouts-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ test('exceeds the timeout', () => {

const {stderr, status} = runJest(DIR, ['-w=1', '--ci=false']);
const {rest, summary} = extractSummary(stderr);
expect(status).toBe(1);
expect(rest).toMatchSnapshot();
expect(rest).toMatch(/(jasmine\.DEFAULT_TIMEOUT_INTERVAL|Exceeded timeout)/);
expect(summary).toMatchSnapshot();
expect(status).toBe(1);
});

test('does not exceed the timeout', () => {
Expand All @@ -57,7 +57,7 @@ test('does not exceed the timeout', () => {

const {stderr, status} = runJest(DIR, ['-w=1', '--ci=false']);
const {rest, summary} = extractSummary(stderr);
expect(status).toBe(0);
expect(rest).toMatchSnapshot();
expect(summary).toMatchSnapshot();
expect(status).toBe(0);
});
31 changes: 19 additions & 12 deletions packages/jest-circus/src/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@ const _runTestsForDescribeBlock = async (describeBlock: DescribeBlock) => {

const _runTest = async (test: TestEntry): Promise<void> => {
const testContext = Object.create(null);

const isSkipped =
test.mode === 'skip' ||
(getState().hasFocusedTests && test.mode !== 'only');

if (isSkipped) {
dispatch({name: 'test_skip', test});
return;
}

const {afterEach, beforeEach} = getEachHooksForTest(test);

for (const hook of beforeEach) {
Expand All @@ -69,27 +79,24 @@ const _runTest = async (test: TestEntry): Promise<void> => {

const _callHook = (hook: Hook, testContext?: TestContext): Promise<any> => {
dispatch({hook, name: 'hook_start'});
return callAsyncFn(hook.fn, testContext, {isHook: true})
const {testTimeout: timeout} = getState();
return callAsyncFn(hook.fn, testContext, {isHook: true, timeout})
.then(() => dispatch({hook, name: 'hook_success'}))
.catch(error => dispatch({error, hook, name: 'hook_failure'}));
};

const _callTest = (test: TestEntry, testContext: TestContext): Promise<any> => {
const isSkipped =
test.mode === 'skip' ||
(getState().hasFocusedTests && test.mode !== 'only');

if (isSkipped) {
dispatch({name: 'test_skip', test});
return Promise.resolve();
}

const _callTest = async (
test: TestEntry,
testContext: TestContext,
): Promise<any> => {
dispatch({name: 'test_start', test});
const {testTimeout: timeout} = getState();

if (!test.fn) {
throw Error(`Tests with no 'fn' should have 'mode' set to 'skipped'`);
}

return callAsyncFn(test.fn, testContext)
return callAsyncFn(test.fn, testContext, {isHook: false, timeout})
.then(() => dispatch({name: 'test_success', test}))
.catch(error => dispatch({error, name: 'test_failure', test}));
};
Expand Down
65 changes: 39 additions & 26 deletions packages/jest-circus/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,46 +110,59 @@ const getEachHooksForTest = (
return result;
};

const _makeTimeoutMessage = (timeout, isHook) => {
const message = `Exceeded timeout of ${timeout}ms for a ${isHook ? 'hook' : 'test'}.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about also receiving testEntry.name as a parameter? So that it is a bit more explicit about in which test this happened

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i actually did it first and then realized that the name of the test is already printed right before the error (cause it's going to be under the failing test) 🙂

Copy link
Member

Choose a reason for hiding this comment

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

What does this look like? Currently, timeouts are the hardest thing to deal with and the signal they give is so bad.

Also, you can write this function without curly braces, return statements or new variables. I believe in you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screen shot 2017-06-08 at 9 06 43 am

timeouts are hard to deal with, but mostly because of the quality of the test.
There's not much we can do here. We can try to print the exact hook that timeouted (if it happened in a hook) or we can try to print test coverage of the test function (to see which parts weren't called), but i'm not sure how helpful it is

Copy link
Member

Choose a reason for hiding this comment

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

Can we add "Use jest.setTimeout(…) to update the timeout" maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's actually a good idea

return new Error(message);
};

const callAsyncFn = (
fn: AsyncFn,
testContext: ?TestContext,
{isHook}: {isHook?: boolean} = {isHook: false},
{
isHook,
timeout,
test,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be sorted alphabetically? It seems pretty important to @cpojer though, so I'm just stealing his thunder.

Copy link
Member

Choose a reason for hiding this comment

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

yes, it must be. It is sacred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpojer that will go into your performance review! 😃

}: {isHook?: ?boolean, timeout: number, test?: TestEntry},
): Promise<any> => {
// If this fn accepts `done` callback we return a promise that fullfills as
// soon as `done` called.
if (fn.length) {
return new Promise((resolve, reject) => {
return new Promise((resolve, reject) => {
setTimeout(() => reject(_makeTimeoutMessage(timeout, isHook)), timeout);

// If this fn accepts `done` callback we return a promise that fullfills as
// soon as `done` called.
if (fn.length) {
const done = (reason?: Error | string): void =>
reason ? reject(reason) : resolve();

fn.call(testContext, done);
});
}
return fn.call(testContext, done);
}

let returnedValue;
try {
returnedValue = fn.call(testContext);
} catch (error) {
return Promise.reject(error);
}
let returnedValue;
try {
returnedValue = fn.call(testContext);
} catch (error) {
return reject(error);
}

// If it's a Promise, return it.
if (returnedValue instanceof Promise) {
return returnedValue;
}
// If it's a Promise, return it.
if (returnedValue instanceof Promise) {
return returnedValue.then(resolve).catch(reject);
Copy link
Member

Choose a reason for hiding this comment

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

.then(resolve, reject)

}

if (!isHook && returnedValue !== void 0) {
throw new Error(
`
if (!isHook && returnedValue !== void 0) {
return reject(
new Error(
`
test functions can only return Promise or undefined.
Returned value: ${String(returnedValue)}
`,
);
}
),
);
}

// Otherwise this test is synchronous, and if it didn't throw it means
// it passed.
return Promise.resolve();
// Otherwise this test is synchronous, and if it didn't throw it means
// it passed.
return resolve();
});
};

const getTestDuration = (test: TestEntry): ?number => {
Expand Down