Check added stack trace parts for filename match #387
Conversation
|
I think that tests kind of need to not ignore stack trace lines that originate within the tests directory, so I'd think updating those tests is the right path here (also totally fine to do that in a separate PR, and this can be rebased after that's merged) |
|
Updated tests are in #388. This PR is rebased off of that one. |
|
Please rebase this on latest master when you can :-D |
|
Rebased! |
lib/test.js
Outdated
| var filemRe = /((?:\/|[A-Z]:\\)[^:\s]+:(\d+)(?::(\d+))?)/; | ||
| var filem; | ||
| var sIndex; | ||
| for (sIndex in s.slice(0, 4)) { |
There was a problem hiding this comment.
for..in loops should be avoided; this would be better done as a for loop, or as Array.prototype.some.call or something
There was a problem hiding this comment.
Array.prototype.forEach would be the cleanest way to do this, but it's not supported in older versions of Node (or older browsers) without polyfill support. Not sure what Tape's polyfill policy is.
I can switch it to some variation of for (sIndex = 0; sIndex < s.slice(0, 4).length; s++) { ... } though.
(I'm not sure what expectations Tape makes
There was a problem hiding this comment.
True; we have the for-each package to handle forEach but that doesn't handle some.
lib/test.js
Outdated
| res.file = filem[1]; | ||
| res.line = Number(filem[2]); | ||
| if (filem[3]) res.column = filem[3]; | ||
|
|
||
| res.at = m[1]; | ||
| res.at = s.length > 1 ? m[1] : '<anonymous> (' + m[1] + ')'; |
There was a problem hiding this comment.
why s.length instead of m.length here?
There was a problem hiding this comment.
m is the result of the regular expression matching something like at /path/to/file.js:123:10 or at functionName /path/to/file.js:123:10. If we're at this point in the code, m.length should always be the same, where m[1] is the portion of the regex matching everything after at.
s is m[1].split(/\s+/) So if s has a length of 1, we know thatm[1] is just case where there's just a path with no function name.
Although in writing this, it now occurs to me that the original m[1].split(/\s+/) code fails to account for directory and file names with spaces ... lemme go fix that.
There was a problem hiding this comment.
Maybe we could store s.length > 1 in a nicely named variable, for clarity?
|
Added test for a file name with a space in it. Rewrote the stack trace regex to handle that scenario. New approach also gets rid of the |
lib/test.js
Outdated
| at Test.bound [as run] (/path/to/file.js:123:45) | ||
| at /path/to/file.js:123:45 | ||
| */ | ||
| var m = new RegExp( |
There was a problem hiding this comment.
it would be really nice if we could use a regex literal, and repeat the parts you want to comment, in the comment, instead of using new RegExp
lib/test.js
Outdated
| res.functionName = callDescription.split(/s+/)[0] | ||
| res.file = filePath; | ||
| res.line = Number(m[3]); | ||
| if (m[4]) res.column = m[4]; |
|
Comments addressed (amended old commit). |
- [New] use `process.env.NODE_TAPE_OBJECT_PRINT_DEPTH` for the default object print depth (#420) - [New] Add "onFailure" listener to test harness (#408) - [Fix] fix stack where actual is falsy (#400) - [Fix] normalize path separators in stacks (#402) - [Fix] fix line numbers in stack traces from inside anon functions (#387) - [Fix] Fix dirname in stack traces (#388) - [Robustness] Use local reference for clearTimeout global (#385) - [Deps] update `function-bind`
Fixes #386. Also refactors regex checking code a little.
One thing I'm not sure about is how to test this. The issue is actually this line here:
https://github.com/fongandrew/tape/blob/2c242ca9715b6b0bfded23fccb522dd414d96612/lib/test.js#L219
Tape ignores stack trace lines that originate from files starting with that
dirvariable. Currently, it means ignoring all stack trace lines from within tape's own package directory. The problem is that tape's own tests also live within its package directory, so theatvariable is never set on the result object.We could try changing
dirto point to just thelibdirectory inside the package. This would result in theatbeing set and we could check it. This should be fine to do, since the stack trace for an assertion error shouldn't run through any file outside of thelibdirectory, but I'm not sure if that could change in the future.Also, this would result in 25 or so existing tests failing (since their test outputs don't include the
atvariable). It wouldn't be a big deal to update those tests (correct behavior should be including theatvariable in those test results), but feedback welcome on that.