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

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

Closed
DavidAnson opened this issue May 18, 2020 · 6 comments

Comments

@DavidAnson
Copy link
Contributor

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
Copy link
Collaborator

Raynos commented May 18, 2020

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

Lambdas do not have names

@ljharb
Copy link
Collaborator

ljharb commented May 18, 2020

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
Copy link
Contributor Author

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
Copy link
Collaborator

ljharb commented May 18, 2020

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

@DavidAnson
Copy link
Contributor Author

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
Copy link
Collaborator

ljharb commented May 18, 2020

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.

@ljharb ljharb closed this as completed in 470e43b May 20, 2020
ljharb added a commit that referenced this issue May 25, 2020
 - [Fix] `createStream`: `result` payload is not always an object (#519)
 - [Fix] Update RegExp for matching stack frames to handle Promise/then scenario (#515)
 - [readme] add `tape-repeater` (#511)
 - [Dev Deps] update `eslint`
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

No branches or pull requests

3 participants