tape-testing / tape

tap-producing test harness for node and browsers
MIT License
5.77k stars 307 forks source link

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

Open petermetz opened 3 years ago

petermetz commented 3 years ago

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 commented 3 years ago

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

codecov-io commented 3 years ago

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%) :arrow_up:

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 commented 3 years ago

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

petermetz commented 3 years ago

@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 commented 3 years ago

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

petermetz commented 3 years ago

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

ljharb commented 3 years ago

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