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

fix(lib/test): stack traces when res.name is Error, not string #532

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

petermetz
Copy link

When executing tests, the res.name property
(as constructed in the ./lib/test.js#_assert method)
is actually not a string but the error that was thrown.
This (assuming edge case) combined with
extra.error and opts.error both being falsy ends up
producing this situation where feeding res.name into
the Error constructor makes the original error's stack
trace disappear and only it's message remains with the
newly constructed Error instance holding a completely
generic stack trace that's internal to tape's source code
(and therefore not much use when a test is failing due to
some code is throwing exceptions instead of failing an
assertion).

Signed-off-by: Peter Somogyvari peter.metz@unarin.com

@petermetz
Copy link
Author

@ljharb Not sure why it's failing on older Node versions TBH. Do you have any pro tips where I could start with fixing the CI on NodeJS 7, 6, etc.?

Failing assertion here for example:
https://travis-ci.org/github/substack/tape/jobs/736511556#L1010

Thank you in advance!

When executing tests, the res.name property
(as constructed in the ./lib/test.js#_assert method)
is actually not a string but the error that was thrown.
This (assuming edge case) combined with
extra.error and opts.error both being falsy ends up
producing this situation where feeding res.name into
the Error constructor makes the original error's stack
trace disappear and only it's message remains with the
newly constructed Error instance holding a completely
generic stack trace that's internal to tape's source code
(and therefore not much use when a test is failing due to
some code is throwing exceptions instead of failing an
assertion).

Signed-off-by: Peter Somogyvari <peter.metz@unarin.com>
@ljharb ljharb force-pushed the fix/stack-trace-res-name-error branch from c40ec63 to 3e8cf85 Compare January 4, 2021 05:39
@codecov-io
Copy link

codecov-io commented Jan 4, 2021

Codecov Report

Merging #532 (3e8cf85) into master (04da90b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
+ Coverage   73.84%   73.87%   +0.03%     
==========================================
  Files          19       19              
  Lines         757      758       +1     
  Branches      146      147       +1     
==========================================
+ Hits          559      560       +1     
  Misses        198      198              
Impacted Files Coverage Δ
lib/test.js 95.98% <100.00%> (+0.01%) ⬆️

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 04da90b...3e8cf85. Read the comment docs.

@ljharb
Copy link
Collaborator

ljharb commented Jan 4, 2021

@petermetz i've rebased this. it's passing ; but either way it needs a regression test.

@petermetz
Copy link
Author

@petermetz i've rebased this. it's passing ; but either way it needs a regression test.

Thanks @ljharb I'll try to put a specific test case in there.

@ljharb ljharb marked this pull request as draft January 7, 2021 23:55
@ljharb
Copy link
Collaborator

ljharb commented Jul 26, 2021

@petermetz any chance you're still planning to add the regression test?

@petermetz
Copy link
Author

@ljharb Apologies, still planning on it, yes. Might take some more time, but definitely have the intent. :-)

@ljharb
Copy link
Collaborator

ljharb commented Jul 27, 2021

No worries, whenever you can get to it is fine.

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