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 test failures #3770

Merged
merged 2 commits into from
Jun 9, 2017
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
24 changes: 0 additions & 24 deletions integration_tests/__tests__/__snapshots__/failures-test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -31,54 +31,30 @@ exports[`not throwing Error objects 3`] = `
exports[`not throwing Error objects 4`] = `
" FAIL __tests__/assertion-count-test.js
● .assertions() › throws

expect(received).toBeTruthy()

Expected value to be truthy, instead received
false

at __tests__/assertion-count-test.js:14:17

● .assertions() › throws

expect.assertions(2)

Expected two assertions to be called but only received one assertion call.

at ../../packages/jest-jasmine2/build/setup-jest-globals.js:68:21

● .assertions() › throws on redeclare of assertion count

expect(received).toBeTruthy()

Expected value to be truthy, instead received
false

at __tests__/assertion-count-test.js:18:17

● .assertions() › throws on assertion

expect.assertions(0)

Expected zero assertions to be called but only received one assertion call.

at ../../packages/jest-jasmine2/build/setup-jest-globals.js:68:21

● .hasAssertions() › throws when there are not assertions

expect.hasAssertions()

Expected at least one assertion to be called but received none.

at ../../packages/jest-jasmine2/build/setup-jest-globals.js:88:21

.assertions()
✕ throws
✕ throws on redeclare of assertion count
✕ throws on assertion
.hasAssertions()
✕ throws when there are not assertions

"
`;

Expand Down
26 changes: 25 additions & 1 deletion integration_tests/__tests__/failures-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,30 @@ const normalizeDots = text => text.replace(/\.{1,}$/gm, '.');

skipOnWindows.suite();

const cleanupStackTrace = stderr => {
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 make sure this function verifies that there is a stack trace for both and fails if one of them or both are missing? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can't run both versions yet (jasmine and circus). And this is mostly for me. when i'm done with circus this will be removed, and the error is there only to make sure i don't forget to remove it after i'm done :)

const STACK_REGEXP = /^.*at.*(setup-jest-globals|extractExpectedAssertionsErrors).*\n/gm;
if (!STACK_REGEXP.test(stderr)) {
throw new Error(
`
This function is used to remove inconsistent stack traces between
jest-jasmine2 and jest-circus. If you see this error, that means the
stack traces are no longer inconsistent and this function can be
safely removed.

output:
${stderr}
`,
);
}

return (
stderr
.replace(STACK_REGEXP, '')
// Also remove trailing whitespace.
.replace(/\s+$/gm, '')
);
};

test('not throwing Error objects', () => {
let stderr;
stderr = runJest(dir, ['throw-number-test.js']).stderr;
Expand All @@ -26,7 +50,7 @@ test('not throwing Error objects', () => {
stderr = runJest(dir, ['throw-object-test.js']).stderr;
expect(extractSummary(stderr).rest).toMatchSnapshot();
stderr = runJest(dir, ['assertion-count-test.js']).stderr;
expect(extractSummary(stderr).rest).toMatchSnapshot();
expect(extractSummary(cleanupStackTrace(stderr)).rest).toMatchSnapshot();
});

test('works with node assert', () => {
Expand Down
1 change: 1 addition & 0 deletions integration_tests/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ const cleanupStackTrace = (output: string) => {
.replace(/\n.*at.*next_tick\.js.*$/gm, '')
.replace(/\n.*at Promise \(<anonymous>\).*$/gm, '')
.replace(/\n.*at <anonymous>.*$/gm, '')
.replace(/\n.*at Generator.next \(<anonymous>\).*$/gm, '')
.replace(/^.*at.*[\s][\(]?(\S*\:\d*\:\d*).*$/gm, ' at $1');
};

Expand Down
3 changes: 2 additions & 1 deletion packages/jest-circus/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
"dependencies": {
"jest-snapshot": "^20.0.3",
"jest-matchers": "^20.0.3",
"jest-message-util": "^20.0.3"
"jest-message-util": "^20.0.3",
"jest-diff": "^20.0.3"
},
"devDependencies": {
"jest-runtime": "^20.0.3"
Expand Down
149 changes: 149 additions & 0 deletions packages/jest-circus/src/formatNodeAssertErrors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/

import type {DiffOptions} from 'jest-diff/src/diffStrings';
import type {Event, State} from '../types';

const {printReceived, printExpected} = require('jest-matcher-utils');
const chalk = require('chalk');
const diff = require('jest-diff');

type AssertionError = {|
actual: ?string,
expected: ?string,
generatedMessage: boolean,
message: string,
name: string,
operator: ?string,
stack: string,
|};

const assertOperatorsMap = {
'!=': 'notEqual',
'!==': 'notStrictEqual',
'==': 'equal',
'===': 'strictEqual',
};

const humanReadableOperators = {
deepEqual: 'to deeply equal',
deepStrictEqual: 'to deeply and strictly equal',
notDeepEqual: 'not to deeply equal',
notDeepStrictEqual: 'not to deeply and strictly equal',
};

module.exports = (event: Event, state: State) => {
switch (event.name) {
case 'test_failure':
case 'test_success': {
event.test.errors = event.test.errors.map(error => {
return error instanceof require('assert').AssertionError
? assertionErrorMessage(error, {expand: state.expand})
: error;
});
break;
}
}
};

const getOperatorName = (operator: ?string, stack: string) => {
if (typeof operator === 'string') {
return assertOperatorsMap[operator] || operator;
}
if (stack.match('.doesNotThrow')) {
return 'doesNotThrow';
}
if (stack.match('.throws')) {
return 'throws';
}
return '';
};

const operatorMessage = (operator: ?string, negator: boolean) =>
typeof operator === 'string'
? operator.startsWith('!') || operator.startsWith('=')
? `${negator ? 'not ' : ''}to be (operator: ${operator}):\n`
: `${humanReadableOperators[operator] || operator} to:\n`
: '';

const assertThrowingMatcherHint = (operatorName: string) => {
return (
chalk.dim('assert') +
chalk.dim('.' + operatorName + '(') +
chalk.red('function') +
chalk.dim(')')
);
};

const assertMatcherHint = (operator: ?string, operatorName: string) => {
let message =
chalk.dim('assert') +
chalk.dim('.' + operatorName + '(') +
chalk.red('received') +
chalk.dim(', ') +
chalk.green('expected') +
chalk.dim(')');

if (operator === '==') {
message +=
' or ' +
chalk.dim('assert') +
chalk.dim('(') +
chalk.red('received') +
chalk.dim(') ');
}

return message;
};

function assertionErrorMessage(error: AssertionError, options: DiffOptions) {
const {expected, actual, message, operator, stack} = error;
const diffString = diff(expected, actual, options);
const negator =
typeof operator === 'string' &&
(operator.startsWith('!') || operator.startsWith('not'));
const hasCustomMessage = !error.generatedMessage;
const operatorName = getOperatorName(operator, stack);

if (operatorName === 'doesNotThrow') {
return (
assertThrowingMatcherHint(operatorName) +
'\n\n' +
chalk.reset(`Expected the function not to throw an error.\n`) +
chalk.reset(`Instead, it threw:\n`) +
` ${printReceived(actual)}` +
chalk.reset(hasCustomMessage ? '\n\nMessage:\n ' + message : '') +
stack.replace(/AssertionError(.*)/g, '')
);
}

if (operatorName === 'throws') {
return (
assertThrowingMatcherHint(operatorName) +
'\n\n' +
chalk.reset(`Expected the function to throw an error.\n`) +
chalk.reset(`But it didn't throw anything.`) +
chalk.reset(hasCustomMessage ? '\n\nMessage:\n ' + message : '') +
stack.replace(/AssertionError(.*)/g, '')
);
}

return (
assertMatcherHint(operator, operatorName) +
'\n\n' +
chalk.reset(`Expected value ${operatorMessage(operator, negator)}`) +
` ${printExpected(expected)}\n` +
chalk.reset(`Received:\n`) +
` ${printReceived(actual)}` +
chalk.reset(hasCustomMessage ? '\n\nMessage:\n ' + message : '') +
(diffString ? `\n\nDifference:\n\n${diffString}` : '') +
stack.replace(/AssertionError(.*)/g, '')
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ import type {TestResult, Status} from 'types/TestResult';
import type {GlobalConfig, Path, ProjectConfig} from 'types/Config';
import type {Event, TestEntry} from '../../types';

import {getState, setState} from 'jest-matchers';
import {
extractExpectedAssertionsErrors,
getState,
setState,
} from 'jest-matchers';
import {formatResultsErrors} from 'jest-message-util';
import {SnapshotState, addSerializer} from 'jest-snapshot';
import {addEventHandler, ROOT_DESCRIBE_BLOCK_NAME} from '../state';
Expand Down Expand Up @@ -157,11 +161,18 @@ const eventHandler = (event: Event) => {
case 'test_success':
case 'test_failure': {
_addSuppressedErrors(event.test);
_addExpectedAssertionErrors(event.test);
break;
}
}
};

const _addExpectedAssertionErrors = (test: TestEntry) => {
const errors = extractExpectedAssertionsErrors();
errors.length && (test.status = 'fail');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it is shorter, but I think it would be more readble like this, no?

if (errors.length) {
  test.status = 'fail'
}

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 would disagree with readability, recently i find it very hard to navigate through 100 lines functions that do just 2 things, but i agree that this line messes up semantics pretty bad by using a logical operator as an if condition :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, as long as we don't have pattern matching :D

test.errors = test.errors.concat(errors);
};

// Get suppressed errors from ``jest-matchers`` that weren't throw during
// test execution and add them to the test result, potentially failing
// a passing test.
Expand Down
7 changes: 6 additions & 1 deletion packages/jest-circus/src/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,20 @@ import type {Event, State, EventHandler} from '../types';

import {makeDescribe} from './utils';
import eventHandler from './eventHandler';
import formatNodeAssertErrors from './formatNodeAssertErrors';

const eventHandlers: Array<EventHandler> = [eventHandler];
const eventHandlers: Array<EventHandler> = [
eventHandler,
formatNodeAssertErrors,
];

const ROOT_DESCRIBE_BLOCK_NAME = 'ROOT_DESCRIBE_BLOCK';
const STATE_SYM = Symbol('JEST_STATE_SYMBOL');

const ROOT_DESCRIBE_BLOCK = makeDescribe(ROOT_DESCRIBE_BLOCK_NAME);
const INITIAL_STATE: State = {
currentDescribeBlock: ROOT_DESCRIBE_BLOCK,
expand: undefined,
hasFocusedTests: false,
rootDescribeBlock: ROOT_DESCRIBE_BLOCK,
testTimeout: 5000,
Expand Down
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 @@ const _formatError = (error: ?Exception): string => {
} else if (error.message) {
return error.message;
} else {
return String(error);
return `${String(error)} thrown`;
Copy link
Member

Choose a reason for hiding this comment

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

If you are turning it into a sentence, it needs to end with a ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the way jasmine throws errors now, i just change it to this to be compatible with current snapshots

}
};

Expand Down
1 change: 1 addition & 0 deletions packages/jest-circus/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export type State = {|
hasFocusedTests: boolean, // that are defined using test.only
rootDescribeBlock: DescribeBlock,
testTimeout: number,
expand?: boolean, // expand error messages
|};

export type DescribeBlock = {|
Expand Down
64 changes: 64 additions & 0 deletions packages/jest-matchers/src/extractExpectedAssertionsErrors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* Copyright (c) 2014, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/

import {
EXPECTED_COLOR,
RECEIVED_COLOR,
matcherHint,
pluralize,
} from 'jest-matcher-utils';

import {getState, setState} from './jest-matchers-object';

// Create and format all errors related to the mismatched number of `expect`
// calls and reset the matchers state.
const extractExpectedAssertionsErrors = () => {
const result = [];
const {
assertionCalls,
expectedAssertionsNumber,
isExpectingAssertions,
} = getState();
setState({assertionCalls: 0, expectedAssertionsNumber: null});
if (
typeof expectedAssertionsNumber === 'number' &&
assertionCalls !== expectedAssertionsNumber
) {
const numOfAssertionsExpected = EXPECTED_COLOR(
pluralize('assertion', expectedAssertionsNumber),
);
const error = new Error(
matcherHint('.assertions', '', String(expectedAssertionsNumber), {
isDirectExpectCall: true,
}) +
'\n\n' +
`Expected ${numOfAssertionsExpected} to be called but only received ` +
RECEIVED_COLOR(pluralize('assertion call', assertionCalls || 0)) +
'.',
);
result.push(error);
}
if (isExpectingAssertions && assertionCalls === 0) {
const expected = EXPECTED_COLOR('at least one assertion');
const received = RECEIVED_COLOR('received none');
const error = new Error(
matcherHint('.hasAssertions', '', '', {
isDirectExpectCall: true,
}) +
'\n\n' +
`Expected ${expected} to be called but ${received}.`,
);
result.push(error);
}

return result;
};

module.exports = extractExpectedAssertionsErrors;
Loading