tape-testing / tape

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

Test.prototype._assert fails to identify source file/line for calls to assert methods inside Promise/then context #515

Closed DavidAnson closed 4 years ago

DavidAnson commented 4 years ago
pi@claw:~/tape-issue $ node --version
v12.16.3
pi@claw:~/tape-issue $ npm install tape
+ tape@5.0.0
pi@claw:~/tape-issue $ cat tape-issue.js 
const tape = require("tape");

tape("repro", (test) => {
  test.plan(2);
  test.ok(false, "This call identifies the file/line");
  new Promise((resolve) => {
    resolve();
  }).then(() => {
    test.ok(false, "This call does NOT identify the file/line");
    test.end();
  });
});
pi@claw:~/tape-issue $ node tape-issue.js 
TAP version 13
# repro
not ok 1 This call identifies the file/line
  ---
    operator: ok
    expected: true
    actual:   false
    at: Test.<anonymous> (/home/pi/tape-issue/tape-issue.js:5:8)
    stack: |-
      Error: This call identifies the file/line
          at Test.assert [as _assert] (/home/pi/tape-issue/node_modules/tape/lib/test.js:260:54)
          at Test.bound [as _assert] (/home/pi/tape-issue/node_modules/tape/lib/test.js:84:32)
          at Test.assert (/home/pi/tape-issue/node_modules/tape/lib/test.js:379:10)
          at Test.bound [as ok] (/home/pi/tape-issue/node_modules/tape/lib/test.js:84:32)
          at Test.<anonymous> (/home/pi/tape-issue/tape-issue.js:5:8)
          at Test.bound [as _cb] (/home/pi/tape-issue/node_modules/tape/lib/test.js:84:32)
          at Test.run (/home/pi/tape-issue/node_modules/tape/lib/test.js:101:31)
          at Test.bound [as run] (/home/pi/tape-issue/node_modules/tape/lib/test.js:84:32)
          at Immediate.next [as _onImmediate] (/home/pi/tape-issue/node_modules/tape/lib/results.js:83:19)
          at processImmediate (internal/timers.js:456:21)
  ...
not ok 2 This call does NOT identify the file/line
  ---
    operator: ok
    expected: true
    actual:   false
    stack: |-
      Error: This call does NOT identify the file/line
          at Test.assert [as _assert] (/home/pi/tape-issue/node_modules/tape/lib/test.js:260:54)
          at Test.bound [as _assert] (/home/pi/tape-issue/node_modules/tape/lib/test.js:84:32)
          at Test.assert (/home/pi/tape-issue/node_modules/tape/lib/test.js:379:10)
          at Test.bound [as ok] (/home/pi/tape-issue/node_modules/tape/lib/test.js:84:32)
          at /home/pi/tape-issue/tape-issue.js:9:10
          at processTicksAndRejections (internal/process/task_queues.js:97:5)
  ...

1..2
# tests 2
# pass  0
# fail  2

The code inside the following loop seems to fail the RegExp match against the non-tape stack frames in the second case, so the at: field is missing from the output. (Specifically, the res object doesn't get its functionName, file, line, column, or at properties set.) An update of the RegExp to match lines like at /home/pi/tape-issue/tape-issue.js:9:10 would seem to address this problem.

https://github.com/substack/tape/blob/0b5804d43068602e1615dfd395a3d85949bb03da/lib/test.js#L268-L328

Raynos commented 4 years ago

This may be because of lambda functions. Try using named function expressions instead.

Lambdas do not have names

ljharb commented 4 years ago

Arrow functions ("lambdas" can falsely imply traits JS doesn't have) don't have names, indeed - they can infer them, but in this case, they can't, so they're anonymous.

However, that shouldn't affect what line is indicated, only what name is available to help debugging.

I'd be interested in a PR with a failing test case; if you're willing to submit one, it'd be great to compare with a named function as well as an anonymous one, just to eliminate that as the issue.

DavidAnson commented 4 years ago

I can invest more time here, but it’s worth pointing out that the RegExp used by the project (line 304 in the excerpt above) does not successfully match the last line of the sample stack in the relevant comment (line 281) and furthermore that the unmatched sample line has exactly the problematic form that appears in the repro I already provided above. To expand upon my suggestion in the original comment, it seems the RegExp being used requires a trailing ) after the path which may not be present. The following tweak to the end of that RegExp seems to allow it to match all of the lines of the sample: (?:[^\s]*\s*\bat\s+)(?:(.*)\s+\()?((?:\/|[a-zA-Z]:\\)[^:\)]+:(\d+)(?::(\d+))?)\)?.

ljharb commented 4 years ago

Would you be OK making a PR with that fix and regression tests? :-)

DavidAnson commented 4 years ago

My approach would be to start by pulling the stack parsing code out into its own module so that it can be exercised directly by test code, then to add a new test file to verify the sample forms in the comment are all recognized correctly. I think the new function would take as input the stack array and return an object with properties for each of the res fields it can populate. This would be ideal for destructuring by the caller, though I do not see a minimum Node.js version for this project, so that may not be viable. I would not include negative tests because the input here is not user-controlled. Is that what you have in mind?

ljharb commented 4 years ago

I'd prefer not to exercise it directly; instead to write the tests, test them, and validate the output. Check the existing tests for how to do that with tap.