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

chore: update source-map-support #9147

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Nov 7, 2019

Summary

This breaks breaks stack traces, not sure why. Seems to come from evanw/node-source-map-support#253, but I don't know anything more than that

Test plan

Eventually green CI

@JLHwung
Copy link
Contributor

JLHwung commented Nov 7, 2019

The snapshot should be updated, and this is exactly what has been fixed in source-map 0.5.14.

Let's take this snapshot for an example
https://github.com/facebook/jest/blob/bbfef531f436a08698e2c4b289659d48265142c6/e2e/__tests__/__snapshots__/customMatcherStackTrace.test.ts.snap#L22-L47
It is produced by
https://github.com/facebook/jest/blob/bbfef531f436a08698e2c4b289659d48265142c6/e2e/custom-matcher-stack-trace/__tests__/sync.test.js#L41-L54

In the code above, the error is thrown in the function baz, according to the stack trace format, it should print on the first line the name of the function called, which is baz, instead of Error.

source-map v0.5.16 fixed this bug and one can see the snapshot difference on the CI log.

    - Snapshot  - 6
    + Received  + 6

    @@ -14,11 +14,11 @@
               |             ^
            46 |     };
            47 | 
            48 |     // This expecation fails due to an error we throw (intentionally)

    -       at Error (__tests__/sync.test.js:45:13)
    -       at baz (__tests__/sync.test.js:43:23)
    -       at bar (__tests__/sync.test.js:42:23)
    -       at foo (__tests__/sync.test.js:52:7)
    -       at Object.callback (__tests__/sync.test.js:11:18)
    -       at Object.toCustomMatch (__tests__/sync.test.js:53:8)
    +       at baz (__tests__/sync.test.js:45:13)
    +       at bar (__tests__/sync.test.js:43:23)
    +       at foo (__tests__/sync.test.js:42:23)
    +       at callback (__tests__/sync.test.js:52:7)
    +       at Object.toCustomMatch (__tests__/sync.test.js:11:18)
    +       at Object.<anonymous> (__tests__/sync.test.js:53:8)

You can also verify the correctness by running the following JavaScript program copy pasted from __tests__/sync.test.js

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

foo();

It will print the following stack trace.

/Users/jh/code/babel/test.js:4
     throw Error('qux');
     ^

Error: qux
    at baz (/Users/jh/code/babel/test.js:4:12)
    at bar (/Users/jh/code/babel/test.js:2:22)
    at foo (/Users/jh/code/babel/test.js:1:19)
    at Object.<anonymous> (/Users/jh/code/babel/test.js:7:1)
    at Module._compile (internal/modules/cjs/loader.js:956:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:973:10)
    at Module.load (internal/modules/cjs/loader.js:812:32)
    at Function.Module._load (internal/modules/cjs/loader.js:724:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1025:10)
    at internal/main/run_main_module.js:17:11

@SimenB
Copy link
Member Author

SimenB commented Nov 8, 2019

Thanks for chiming in @JLHwung, and providing some details!

It seems to me it's now more correct, but provides a worse UX since the name of the assertion is not in the errors. We might have to do some juggling in order to restore the previous errors, as I think seeing Object.toBe when you called expect().toBe is nicer than Object.<anonymous>.test even though the latter is technically correct.


So, doing some digging. If I stop filtering the stack trace, I currently get the following stack:

      at Object.<anonymous>.test (baaah.test.js:2:17)
      at Object.asyncJestTest (packages/jest-jasmine2/build/jasmineAsyncInstall.js:100:37)
      at resolve (packages/jest-jasmine2/build/queueRunner.js:43:12)
          at new Promise (<anonymous>)
      at mapper (packages/jest-jasmine2/build/queueRunner.js:26:19)
      at promise.then (packages/jest-jasmine2/build/queueRunner.js:73:41)
      at process._tickCallback (internal/process/next_tick.js:68:7)

If I remove https://github.com/facebook/jest/blob/85789b5d050f483fc9b8f31876de30f965da3ba4/packages/expect/src/index.ts#L284

I get

      at processResult (packages/expect/build/index.js:337:19)
      at Object.toBe (packages/expect/build/index.js:387:16)
      at Object.<anonymous>.test (baaah.test.js:2:17)
      at Object.asyncJestTest (packages/jest-jasmine2/build/jasmineAsyncInstall.js:100:37)
      at resolve (packages/jest-jasmine2/build/queueRunner.js:43:12)
          at new Promise (<anonymous>)
      at mapper (packages/jest-jasmine2/build/queueRunner.js:26:19)
      at promise.then (packages/jest-jasmine2/build/queueRunner.js:73:41)
      at process._tickCallback (internal/process/next_tick.js:68:7)

If I pass processResult into captureStackTrace instead of throwingMatcher, I get this

      at Object.toBe (packages/expect/build/index.js:387:16)
      at Object.<anonymous>.test (baaah.test.js:2:17)
      at Object.asyncJestTest (packages/jest-jasmine2/build/jasmineAsyncInstall.js:100:37)
      at resolve (packages/jest-jasmine2/build/queueRunner.js:43:12)
          at new Promise (<anonymous>)
      at mapper (packages/jest-jasmine2/build/queueRunner.js:26:19)
      at promise.then (packages/jest-jasmine2/build/queueRunner.js:73:41)
      at process._tickCallback (internal/process/next_tick.js:68:7)

So what I essentially want is to set the name of the function at the top frame as the name of at Object.<anonymous>.test. I'm not sure how easy that is, especially when we get to custom and (even harder) async errors. Might need to use callsites or something similar rather than actual error objects and construct stack traces manually.

Thoughts?

@SimenB
Copy link
Member Author

SimenB commented May 2, 2022

We cannot land this until it does not mess up #10633

@SimenB SimenB force-pushed the bump-source-map-support branch 2 times, most recently from ee52f15 to 76b4d07 Compare May 6, 2022 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants