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

Cleanup stack trace in expect errors #78

Closed
wants to merge 3 commits into from

Conversation

Zikoat
Copy link
Contributor

@Zikoat Zikoat commented Aug 17, 2023

There is a lot of unnecessary noise in the error report, which makes it difficult to mentally parse the error message, and there is a lot of scrolling. The whole stack trace and the test result object is written to the console for each error.

This change removes the test result object, because it is already printed properly first, and the last 3 lines of the stack trace, which are usually jest-light-runner internals.

I think it is helpful to keep the stack trace, because you can click on it in vscode to go directly where the error occured.

 FAIL  apps/finexa-api/src/debtor/debtors-with-approved-claims/testSum.unit.test.js
  ✕ sum (3 ms)

sum
    JestAssertionError: expect(received).toMatchInlineSnapshot(snapshot)
    
    Snapshot name: `sum 1`
    
    - Snapshot  - 1
    + Received  + 3
    
    - 3
    + {
    +   "result": 2,
    + }
        at /home/zikoat/myrepo/apps/finexa-api/src/debtor/debtors-with-approved-claims/testSum.unit.test.ts:2:29
        at runTest (file:///home/zikoat/myrepo/node_modules/jest-light-runner/src/worker-runner.js:177:3)
-        at runTestBlock (file:///home/zikoat/myrepo/node_modules/jest-light-runner/src/worker-runner.js:141:7)
-        at run (file:///home/zikoat/myrepo/node_modules/jest-light-runner/src/worker-runner.js:86:3)
-        at /home/zikoat/myrepo/node_modules/piscina/src/worker.ts:147:20
-     {
-      matcherResult: {
-        actual: '{\n  "result": 2,\n}',
-        expected: '3',
-        message: '\x1B[2mexpect(\x1B[22m\x1B[38;2;0;95;95m\x1B[48;2;215;255;255mreceived\x1B[49m\x1B[39m\x1B[2m).\x1B[22mtoMatchInlineSnapshot\x1B[2m(\x1B[22m\x1B[38;2;128;0;128m\x1B[48;2;255;215;255msnapshot\x1B[49m\x1B[39m\x1B[2m)\x1B[22m\n' +
-          '\n' +
-          'Snapshot name: `sum 1`\n' +
-          '\n' +
-          '\x1B[38;2;128;0;128m\x1B[48;2;255;215;255m- Snapshot  - 1\x1B[49m\x1B[39m\n' +
-          '\x1B[38;2;0;95;95m\x1B[48;2;215;255;255m+ Received  + 3\x1B[49m\x1B[39m\n' +
-          '\n' +
-          '\x1B[38;2;128;0;128m\x1B[48;2;255;215;255m- 3\x1B[49m\x1B[39m\n' +
-          '\x1B[38;2;0;95;95m\x1B[48;2;215;255;255m+ {\x1B[49m\x1B[39m\n' +
-          '\x1B[38;2;0;95;95m\x1B[48;2;215;255;255m+   "result": 2,\x1B[49m\x1B[39m\n' +
-          '\x1B[38;2;0;95;95m\x1B[48;2;215;255;255m+ }\x1B[49m\x1B[39m',
-        name: 'toMatchInlineSnapshot',
-        pass: false
-      }
-    }

 › 1 snapshot failed.
Snapshot Summary
 › 1 snapshot failed from 1 test suite. Inspect your code changes or run `npm run npx -- -u` to update them.

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   1 failed, 1 total
Time:        0.589 s, estimated 1 s
Ran all test suites matching /testsum/i.

@Zikoat
Copy link
Contributor Author

Zikoat commented Aug 17, 2023

Yeah, it's probably a good idea to use the same formatting as the original runner, but integrating jest-message-util seems to be slightly more challenging than this single-line change, and I'm not sure if the parameters "just fit". I haven't really set up my dev environment properly to develop this library.

@fisker Could you take a look, and see if we can use that library here? Maybe don't merge this PR while we are investigating, or close this PR and create a new issue?

@Zikoat
Copy link
Contributor Author

Zikoat commented Aug 17, 2023

I don't think there is any automatic tests to test that failures are reported correctly. I have tested manually in my repo, but you'll have to test that failures are reported correctly if you do some changes.

@nicolo-ribaudo
Copy link
Owner

I agree that we should get rid of that object in error message, as it's mostly noise.

Yeah, it's probably a good idea to use the same formatting as the original runner, but integrating jest-message-util seems to be slightly more challenging than this single-line change, and I'm not sure if the parameters "just fit". I haven't really set up my dev environment properly to develop this library.

I can take a look at this

I don't think there is any automatic tests to test that failures are reported correctly. I have tested manually in my repo, but you'll have to test that failures are reported correctly if you do some changes.

Ugh... PRs welcome 😅 Right our testing strategy is "make sure that it doesn't break Babel's and Prettier's tests".

@nicolo-ribaudo
Copy link
Owner

formatResultsErrors is to merge multiple errors into a single one, and it expects a list of errors that already have their own failure message.

I had to make some tweaks to this PR and I didn't have push access to your branch so I merged it manually: you can see the final result at bfb47d5.

@nicolo-ribaudo nicolo-ribaudo changed the title Hide JSON object below stack trace in error Cleanup stack trace in expect errors Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants