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

[Discussion] Expect only overrides error stack for built-in matchers #5162

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f39d750
Expect only overrides error stack for built-in matchers
bvaughn Dec 22, 2017
e7d9bbd
Fixed naggy lint issue
bvaughn Dec 22, 2017
9c10921
Replaced for...of with Object.keys().forEach()
bvaughn Dec 22, 2017
eb5d29c
Merged master
bvaughn Jan 2, 2018
ea088cf
Removed JestAssertionError export (as it is no longer needed)
bvaughn Jan 2, 2018
f2e7335
Added custom matcher test
bvaughn Jan 2, 2018
3e00753
Adjusted custom_matcher test to use prettified, non-absolute stack
bvaughn Jan 3, 2018
9e0cfae
test: refactor integration test
SimenB Jan 3, 2018
31f8e58
update with correct snapshot
SimenB Jan 3, 2018
c388a49
test: skip on windows
SimenB Jan 3, 2018
0c5c78c
Clarified comment
bvaughn Jan 3, 2018
5843ab5
Further clarified comment
bvaughn Jan 3, 2018
f902b4b
test: fix test for node 6
SimenB Jan 3, 2018
bd21922
test: move custom matcher stack to separate file to be able to skip i…
SimenB Jan 3, 2018
3ef09ea
Revert "test: fix test for node 6"
SimenB Jan 3, 2018
f88bf07
test: really fix node 6
SimenB Jan 3, 2018
ff50df6
Merge branch 'master' into override-error-stack-only-for-internal-mat…
SimenB Jan 4, 2018
2da0eee
docs: add package prefix to changelog
SimenB Jan 4, 2018
6f6dd91
Merge branch 'master' into override-error-stack-only-for-internal-mat…
bvaughn Jan 4, 2018
e8ab539
Reverted unrelated Markdown changes that had made their way into the …
bvaughn Jan 4, 2018
7dad466
Replaced '__jestInternal' string attribute with a Symbol
bvaughn Jan 4, 2018
179093c
Merge branch 'master' into override-error-stack-only-for-internal-mat…
bvaughn Jan 5, 2018
b8c6bfe
Updated snapshot
bvaughn Jan 5, 2018
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
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
## master

None for now

### Fixes

* `[babel-jest]` moduleFileExtensions not passed to babel transformer.
([#4637](https://github.com/facebook/jest/issues/4637))
* Do not override `Error` stack (with `Error.captureStackTrace`) for custom matchers.
([#5162](https://github.com/facebook/jest/pull/5162))

### Features

Expand Down
32 changes: 30 additions & 2 deletions packages/expect/src/__tests__/stacktrace.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,18 @@
const jestExpect = require('../');

jestExpect.extend({
toCustomMatch(callback, expectation) {
const actual = callback();

if (actual !== expectation) {
return {
message: () => `Expected "${expectation}" but got "${actual}"`,
pass: false,
};
}

return {pass: true};
},
toMatchPredicate(received, argument) {
argument(received);
return {
Expand All @@ -22,7 +34,7 @@ it('stack trace points to correct location when using matchers', () => {
try {
jestExpect(true).toBe(false);
} catch (error) {
expect(error.stack).toContain('stacktrace.test.js:23');
expect(error.stack).toContain('stacktrace.test.js:35');
}
});

Expand All @@ -32,6 +44,22 @@ it('stack trace points to correct location when using nested matchers', () => {
jestExpect(value).toBe(false);
});
} catch (error) {
expect(error.stack).toContain('stacktrace.test.js:32');
expect(error.stack).toContain('stacktrace.test.js:44');
}
});

it('stack trace points to correct location when throwing from a custom matcher', () => {
try {
jestExpect(() => {
const foo = () => bar();
const bar = () => baz();
const baz = () => {
throw new Error('Expected');
};

foo();
}).toCustomMatch('bar');
} catch (error) {
expect(error.stack).toContain('stacktrace.test.js:57');
}
});
14 changes: 10 additions & 4 deletions packages/expect/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ const makeThrowingMatcher = (
result = matcher.apply(matcherContext, [actual].concat(args));
} catch (error) {
if (
matcher.__jestInternal === true &&
!(error instanceof JestAssertionError) &&
error.name !== 'PrettyFormatPluginError' &&
// Guard for some environments (browsers) that do not support this feature.
Expand Down Expand Up @@ -252,7 +253,8 @@ const makeThrowingMatcher = (
};
};

expect.extend = (matchers: MatchersObject): void => setMatchers(matchers);
expect.extend = (matchers: MatchersObject): void =>
setMatchers(matchers, false);

expect.anything = anything;
expect.any = any;
Expand Down Expand Up @@ -280,9 +282,9 @@ const _validateResult = result => {
};

// add default jest matchers
expect.extend(matchers);
expect.extend(spyMatchers);
expect.extend(toThrowMatchers);
setMatchers(matchers, true);
setMatchers(spyMatchers, true);
setMatchers(toThrowMatchers, true);

expect.addSnapshotSerializer = () => void 0;
expect.assertions = (expected: number) => {
Expand All @@ -296,4 +298,8 @@ expect.getState = getState;
expect.setState = setState;
expect.extractExpectedAssertionsErrors = extractExpectedAssertionsErrors;

// Expose JestAssertionError for custom matchers
// This enables them to preserve the stack for specific errors
expect.JestAssertionError = JestAssertionError;
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! Removed. 😄


module.exports = (expect: Expect);
9 changes: 8 additions & 1 deletion packages/expect/src/jest_matchers_object.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ export const setState = (state: Object) => {

export const getMatchers = () => global[JEST_MATCHERS_OBJECT].matchers;

export const setMatchers = (matchers: MatchersObject) => {
export const setMatchers = (matchers: MatchersObject, isInternal: boolean) => {
Object.keys(matchers).forEach(key => {
const matcher = matchers[key];
Object.defineProperty(matcher, '__jestInternal', {
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 not use a Symbol instead to make this actually secret?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

value: isInternal,
});
});

Object.assign(global[JEST_MATCHERS_OBJECT].matchers, matchers);
};
1 change: 1 addition & 0 deletions types/Matchers.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export type RawMatcherFn = (
expected: any,
actual: any,
options: any,
__jestInternal?: boolean,
) => ExpectationResult;

export type ThrowingMatcherFn = (actual: any) => void;
Expand Down