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.

Sign up for GitHub

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

Jump to bottom

Include test invocation in trace #2723

Closed
wants to merge 2 commits into from
Closed

Include test invocation in trace #2723

wants to merge 2 commits into from

Conversation

MatthiasKunnen
Copy link

Closes #2722.

@novemberborn, before I look into testing this, are you satisfied with this code? There are a few ways this can be accomplished with this one changing the least.

The change

const test = require('ava');

const {macro} = require('../helper/macro');

test('Number must be 5', macro, 4);

now results in the follow output

  Number must be 5

  Difference:

  - 4
  + 5

  › exports.macro (javascript/helper/macro.js:2:7)
+ › test (javascript/spec/something.spec.js:5:1)

@MatthiasKunnen MatthiasKunnen marked this pull request as draft March 23, 2021 21:38
@MatthiasKunnen MatthiasKunnen force-pushed the include-test-invocation-in-trace branch from 5373193 to d735c23 Compare March 23, 2021 21:44
@MatthiasKunnen MatthiasKunnen force-pushed the include-test-invocation-in-trace branch from d735c23 to 337efa2 Compare March 23, 2021 21:50
novemberborn
novemberborn reviewed Mar 27, 2021
View reviewed changes
lib/runner.js
@@ -88,6 +89,8 @@ class Runner extends Emittery {
}

metadata.taskIndex = this.nextTaskIndex++;
const testInvocation = stackUtils.parseLine(stackUtils.captureString().split('\n')[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should be capturing this at runtime. I'm a little concerned about any additional performance overhead, especially in files with lots of tests.

@novemberborn
Copy link
Member

Could we think through the various possible stack trace scenarios, and review which ones show up correctly and which ones don't?

Say:

test('Number must be 5', (t, n) => macro(t, n), 4)

Does this show the invocation of macro() in the test file?

In other words, is this a problem only when the macro (or any other imported function) is used directly?

With AVA 4 I'm looking at having macros be defined explicitly (test.macro((t, n) => …)). We could recognize them and only get the stack for those arguments, not for other imported functions. What do you think?

@novemberborn novemberborn mentioned this pull request Jul 5, 2021
Open
@novemberborn
Copy link
Member

Closing due to inactivity. We'd be happy to pick this up again when you have the time to get back to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@novemberborn novemberborn novemberborn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Report does not contain a reference to the location of the test when using macros or helpers from another file
2 participants
@MatthiasKunnen @novemberborn