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

bvaughn · 2017-12-22T19:07:17Z

Proof of concept for Christoph's suggestion from PR #5138.

Resolves #5136

Note: I don't have much context for the Jest project so this might be an unacceptable hack. 😄

codecov-io · 2017-12-22T19:16:45Z

Codecov Report

Merging #5162 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5162   +/-   ##
=======================================
  Coverage   61.18%   61.18%           
=======================================
  Files         202      202           
  Lines        6765     6765           
  Branches        3        4    +1     
=======================================
  Hits         4139     4139           
  Misses       2625     2625           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cbc26b...b8c6bfe. Read the comment docs.

thymikee · 2017-12-22T19:44:13Z

packages/expect/src/jest_matchers_object.js

@@ -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) => {
+  for (const key in matchers) {


In the spirit of babel/babel#6748 can we change it to forEach?

Interesting. Sure thing!

SimenB

Can you include the integration test from the other PR as well? If we regress, it's easier to identify than if we just get a different number i the trace in the tests in this PR

SimenB · 2017-12-23T13:04:54Z

packages/expect/src/index.js

@@ -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;


Is this still needed in this case?

Nope! Removed. 😄

bvaughn · 2018-01-02T19:43:22Z

Can you include the integration test from the other PR as well?

I'm not clear on how that integration test improves clarity in this case 😄 but I've re-added it!

SimenB · 2018-01-03T10:58:52Z

integration_tests/__tests__/__snapshots__/custom_matcher.test.js.snap

+
+exports[`Custom matcher preserves error stack 1`] = `
+"Error: qux
+    at baz (/Users/bvaughn/Documents/git/jest/integration_tests/__tests__/custom_matcher.test.js:49:13)


this shouldn't happen, right? we should still clean it up to be relative paths (especially for this integration test, but in general as well)

Or do we really not want prettified stack trace at all?

😆 Sorry. This was super silly of me. I'm embarrassed. I noticed that CI failed but hadn't had a chance yet to circle back and see why.

I'm not sure if I fixed this in the best way 😄 but it now tests the prettified, relative stack.

SimenB · 2018-01-03T21:12:11Z

@bvaughn I added a commit with what I meant, and it seems like it doesn't work properly - the stack is lost. I haven't dug into why yet, ideas?

SimenB · 2018-01-03T21:14:45Z

I'm not clear on how that integration test improves clarity in this case

My thinking was that then we can see from the outside that the stack is preserved when running all of jest, instead of just expect in isolation. And unless I'm missing something, the integration test shows that this doesn't work

SimenB · 2018-01-03T21:15:14Z

integration_tests/__tests__/__snapshots__/stack_trace.test.js.snap

+      53 | });
+      54 | 
+
+      at __tests__/custom_matcher.test.js:51:8


this is wrong, we have lost the trace

Yes, as of your commit.

Indeed. should be fixed now

bvaughn · 2018-01-03T21:16:08Z

I haven't dug into why yet, ideas?

I don't really know Jest's infrastructure well enough to understand the implications of your commit.

And unless I'm missing something, the integration test shows that this doesn't work

It did work until it was moved. 😄 I'm not sure if the fact that it doesn't work now indicates a problem with the test, or the way it's now being run.

It looks like the test has been changed slightly to verify the output of console.error rather than the error.stack (as before). Am I reading that wrong?

SimenB · 2018-01-03T21:18:44Z

No, I'm being stupid. My watch process had crashed, so I didn't have your code changes compiled when running jest. It does indeed work :D Sorry about that

SimenB

I still think it's sorta weird how we mess with the stacks, but this is a move in the right direction feature wise I think

bvaughn · 2018-01-03T21:25:19Z

😆 No worries at all. I was pretty confused.

bvaughn · 2018-01-03T21:26:04Z

Thank you for shepherding me through my first commit to Jest, @SimenB 🙇

SimenB · 2018-01-03T21:27:36Z

Gah, Windows... 🙁 I'll try to skip it, but might have to move the test to a separate file so we kan sip the whole suite (because of the snapshot)

SimenB · 2018-01-03T21:32:48Z

integration_tests/stack_trace/__tests__/custom_matcher.test.js

+    expect(() => 'foo').toCustomMatch('foo');
+  });
+
+  // This test is expected to fail


it passes, is that an issue?

Ah, my comment is perhaps confusing.

The "test" isn't expected to fail. The custom matcher assertion is expected to fail, which is why it's wrapped in an expect-to-throw check.

I'll update the comment. 😄

bvaughn · 2018-01-03T21:48:11Z

I've updated the inline comments for (hopefully) better clarity.

…t on windows

This reverts commit f902b4b.

cpojer · 2018-01-04T08:50:35Z

packages/expect/src/jest_matchers_object.js

+export const setMatchers = (matchers: MatchersObject, isInternal: boolean) => {
+  Object.keys(matchers).forEach(key => {
+    const matcher = matchers[key];
+    Object.defineProperty(matcher, '__jestInternal', {


Can we not use a Symbol instead to make this actually secret?

cpojer · 2018-01-04T08:51:46Z

This is looking good, thanks for sticking with us. My only two concerns are:

Can we use a Symbol instead to annotate internal matchers?
Any way we could avoid the added loop on adding all the matchers? It's gonna make Jest's vm context startup unnecessarily slower by calling Object.defineProperty on each matcher.

…chers

also format all markdown files

…chers

…branch

bvaughn · 2018-01-04T18:33:00Z

Thanks @cpojer!

Can we use a Symbol instead to annotate internal matchers?

Yes. Good suggestion.

Any way we could avoid the added loop on adding all the matchers? It's gonna make Jest's vm context startup unnecessarily slower by calling Object.defineProperty on each matcher.

I chose this approach for a couple of reasons:

Minimum footprint, rather than copy-pasting a new attribute to every matcher in source. (This is not a strong argument.)
Guards against accidental omission. (Maybe I could ensure this in some other way, with Flow? The RawMatcherFn type is a function, so I'm not sure how to attribute it with Flow. Maybe there's a way to do this with an intersection type?)
I didn't think the small loop would make a noticeable impact on performance. That being said, if you're concerned that it would then I'm happy to change it. Any suggested alternative approaches?

cpojer

This works for me. Let's make CI green and then feel free to merge it :)

…chers

bvaughn · 2018-01-05T17:22:28Z

Ah! The test that was failing CI (integration_tests/__tests__/failures.test.js) passed for me when run locally, but that's because I hadn't run yarn build recently enough and there was an off-by-one line number in the snapshot. Should be fixed with the latest push.

SimenB · 2018-01-05T17:38:52Z

integration_tests/custom_matcher_stack_trace/__tests__/custom_matcher.test.js

+    const foo = () => bar();
+    const bar = () => baz();
+    const baz = () => {
+      throw Error('qux');


Should this be throw new Error? I'm not sure if there's a semantical difference...

No difference that I'm aware of.

15.11.1 The Error Constructor Called as a Function

When Error is called as a function rather than as a constructor, it creates and initialises a new Error object. Thus the function call Error(…) is equivalent to the object creation expression new Error(…) with the same arguments.

source: http://ecma-international.org/ecma-262/5.1/#sec-15.11.1

cpojer · 2018-01-05T18:58:09Z

Awesome, thanks for working on this!

bvaughn · 2018-01-05T19:16:42Z

Thank you and @SimenB for your help!

github-actions · 2021-05-13T00:10:58Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the CLA Signed ✔️ label Dec 22, 2017

bvaughn mentioned this pull request Dec 22, 2017

Expose JestAssertionError to custom matchers #5138

Closed

bvaughn force-pushed the override-error-stack-only-for-internal-matchers branch from f438ca8 to fe5ca1f Compare December 22, 2017 19:08

Expect only overrides error stack for built-in matchers

f39d750

bvaughn force-pushed the override-error-stack-only-for-internal-matchers branch from fe5ca1f to f39d750 Compare December 22, 2017 19:12

thymikee reviewed Dec 22, 2017

View reviewed changes

bvaughn added 2 commits December 22, 2017 13:01

Fixed naggy lint issue

e7d9bbd

Replaced for...of with Object.keys().forEach()

9c10921

SimenB reviewed Dec 23, 2017

View reviewed changes

bvaughn added 3 commits January 2, 2018 11:09

Merged master

eb5d29c

Removed JestAssertionError export (as it is no longer needed)

ea088cf

Added custom matcher test

f2e7335

SimenB reviewed Jan 3, 2018

View reviewed changes

bvaughn and others added 2 commits January 3, 2018 08:57

Adjusted custom_matcher test to use prettified, non-absolute stack

3e00753

test: refactor integration test

9e0cfae

SimenB reviewed Jan 3, 2018

View reviewed changes

update with correct snapshot

31f8e58

SimenB approved these changes Jan 3, 2018

View reviewed changes

test: skip on windows

c388a49

SimenB reviewed Jan 3, 2018

View reviewed changes

Clarified comment

0c5c78c

Further clarified comment

5843ab5

SimenB added 4 commits January 3, 2018 23:16

test: fix test for node 6

f902b4b

test: move custom matcher stack to separate file to be able to skip i…

bd21922

…t on windows

Revert "test: fix test for node 6"

3ef09ea

This reverts commit f902b4b.

test: really fix node 6

f88bf07

cpojer reviewed Jan 4, 2018

View reviewed changes

SimenB and others added 5 commits January 4, 2018 11:12

Merge branch 'master' into override-error-stack-only-for-internal-mat…

ff50df6

…chers

docs: add package prefix to changelog

2da0eee

also format all markdown files

Merge branch 'master' into override-error-stack-only-for-internal-mat…

6f6dd91

…chers

Reverted unrelated Markdown changes that had made their way into the …

e8ab539

…branch

Replaced '__jestInternal' string attribute with a Symbol

7dad466

cpojer approved these changes Jan 4, 2018

View reviewed changes

bvaughn added 2 commits January 5, 2018 09:18

Merge branch 'master' into override-error-stack-only-for-internal-mat…

179093c

…chers

Updated snapshot

b8c6bfe

SimenB reviewed Jan 5, 2018

View reviewed changes

cpojer merged commit 84abf60 into jestjs:master Jan 5, 2018

bvaughn deleted the override-error-stack-only-for-internal-matchers branch January 5, 2018 19:16

renovate-tests mentioned this pull request Feb 6, 2018

Update dependency jest to v22 - autoclosed renovate-tests/changelog-md#3

Closed

renovate bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

chet-manley mentioned this pull request Apr 18, 2020

Unintended consequence of internal-only stack trace override #9837

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

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

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

Conversation

bvaughn commented Dec 22, 2017 • edited Loading

codecov-io commented Dec 22, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

SimenB commented Jan 3, 2018

SimenB commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jan 3, 2018 • edited Loading

SimenB commented Jan 3, 2018 • edited Loading

SimenB left a comment

Choose a reason for hiding this comment

bvaughn commented Jan 3, 2018 • edited Loading

bvaughn commented Jan 3, 2018

SimenB commented Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Jan 4, 2018

bvaughn commented Jan 4, 2018

cpojer left a comment

Choose a reason for hiding this comment

bvaughn commented Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

bvaughn Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

15.11.1 The Error Constructor Called as a Function

cpojer commented Jan 5, 2018

bvaughn commented Jan 5, 2018

github-actions bot commented May 13, 2021

bvaughn commented Dec 22, 2017 •

edited

Loading

codecov-io commented Dec 22, 2017 •

edited

Loading

bvaughn Jan 3, 2018 •

edited

Loading

bvaughn commented Jan 3, 2018 •

edited

Loading

SimenB commented Jan 3, 2018 •

edited

Loading

bvaughn commented Jan 3, 2018 •

edited

Loading

SimenB commented Jan 3, 2018 •

edited

Loading

bvaughn commented Jan 5, 2018 •

edited

Loading

bvaughn Jan 5, 2018 •

edited

Loading