test: fix line numbers in stack traces#5664
test: fix line numbers in stack traces#5664jedwards1211 wants to merge 6 commits intomochajs:mainfrom
Conversation
|
👋 Hi @jedwards1211, thanks for the pull request! A scan flagged a concern with it. Could you please take a look? [pr-task-completion] This PR's body is missing
Repositories often provide a set of tasks that pull request authors are expected to complete. Those tasks should be marked as completed with a
|
|
This seems helpful :) I don't have much capacity right now but will check this out when I do. Thanks for your continued contributions, I appreciate the backing issue as well :) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5664 +/- ##
==========================================
+ Coverage 89.71% 89.76% +0.05%
==========================================
Files 64 65 +1
Lines 4695 4699 +4
Branches 978 978
==========================================
+ Hits 4212 4218 +6
+ Misses 483 481 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
okay no rush! This failed because I forgot to use |
to make it work on windows
|
any chance we can get a unit test for this change? I know it's super meta, just want to make sure we're covering our bases. If it seems infeasible, that's fine |
|
Yeah that wouldn't be difficult |
NOTE: the nyc config to unexclude failing-sync.fixture.js from instrumentation is essential, without it the test would fail to catch any issues with stack traces.
jedwards1211
left a comment
There was a problem hiding this comment.
@mark-wiemer I had to un-exclude this fixture from instrumentation in the nyc config for the sake of the test:
"!test/integration/fixtures/failing-sync.fixture.js"
I wish I could put comments in that file because if someone thinks "what is this for??" and removes this line,
then the test will no longer be able to catch issues with the stack traces. I put an explanation in the commit
message at least
|
Appreciate it. I think it should be fine, we're very careful about removing anything, and we'd definitely reference this PR and any other mentions of the file before removing it. Are you able to add comments to the fixture.js file explaining what it's for? |
|
Actually that's an existing fixture for another test that I repurposed. Maybe I should make one just for this test? |
|
Making a new fixture is probably best, yes :) |
|
👋 @jedwards1211 are you waiting on us for anything still? |
|
No, do y'all need anything from me? |
|
Oh sorry, looks like i forgot to make some minor changes |
PR Checklist
status: accepting prsOverview
Changes the test scripts to run
nycwithNODE_OPTIONS=--enable-source-mapsso that Node will apply source maps to stack traces.