-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Add testLocationInResults
option
#4782
Conversation
packages/jest-jasmine2/src/index.js
Outdated
if (config.testLocationInResults === true) { | ||
const originalIt = environment.global.it; | ||
environment.global.it = (...args) => { | ||
const stack = callsites()[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about hardcoding the index. Thoughts? Can traverse all and return the first one which is not inside node_modules
instead, perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this needs to be an option, instead of appearing by default (which would also be semver-minor)? Is it that much slower?
A complete test run of jest's own tests, with no cache and without parallelization completes in 260s with this enabled, and 245s without it on my machine. Not a huge difference. Maybe even the relatively expensive call to get a stacktrace is offset by the overhead inherent to any spec creation call. I put it behind an option as we looked into adding the same sort of information to a logger at work, and it became 5x slower. Of course that logger does way less than scheduling a spec, so not too relevant, just explaining why I went with the option. @cpojer is the overhead negligible enough to drop the option? @ljharb next release is a major anyways (we've dropped node@4), so semver is not a huge issue in this particular case 🙂 |
(That’s unfortunate; every time you drop a platform you make jest a more unacceptable option for non-apps/published projects) |
Maybe we motivate the node/v8 teams to speed up actualizing a stack frame or finding the location? This feature would be awesome to have. |
Do I understand correctly that the issue is that formatting a stack trace is expensive? I wonder whether something like caching (code object, code offset) => source position already helps. |
Not formatting a stack, but getting a stacktrace to begin with, so that I can give a nice message about who called me (in general). (EDIT: Well, I guess it depends on what you mean when you say "formatting a stack trace". Hopefully there's no ambiguity now 🙂) Use case can be seen in this issue (we want to point to the place in user code that defined a spec, so other tooling can find it without searching for strings). The use case I had at work which I alluded to above is that we append a field called |
Are all collected stack traces used, or only a subset? If the latter, you should be able to improve performance by only accessing the But if I'm reading your previous replies correctly, all traces are used :/ I'm not surprised that this is slow, this path is very much not optimized for performance. I'm sure it could be improved with a bit of work. See https://cs.chromium.org/chromium/src/v8/src/messages.cc?l=956&rcl=82d03a6ec9fe603f2fba2e103f4e31d3db588c8d. |
Well, I only need 1 of the frames. But which one I need varies, so I have to traverse the stack until I find it. In my logging case I need the topmost frame. In Jest I'd want the first one not in Would it be possible with an api where I provided a predicate function for a stackframe I want so that a full stack trace is not needed at all times? I have no idea if that'd help performance-wise |
@@ -189,6 +189,19 @@ Prevent tests from printing messages through the console. | |||
|
|||
Alias: `-t`. Run only tests and test suites with a name that matches the regex. For example, suppose you want to run only tests related to authorization which will have names like `"GET /api/posts with auth"`, then you can use `jest -t=auth`. | |||
|
|||
### `--testLocationInResults` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to update https://facebook.github.io/jest/docs/en/configuration.html#testresultsprocessor-string as well
EDIT: done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The only concern is already mentioned perf, but since it's not a default I'm ok. Maybe worth to leave a todo comment to get back to it after some V8 optimisations?
This is super useful ( we have code in jest-editor-support that does this also ) so it's nice to have it centralized correctly 👍 |
Codecov Report
@@ Coverage Diff @@
## master #4782 +/- ##
==========================================
- Coverage 58.94% 58.88% -0.06%
==========================================
Files 199 199
Lines 6630 6638 +8
Branches 3 4 +1
==========================================
+ Hits 3908 3909 +1
- Misses 2722 2729 +7
Continue to review full report at Codecov.
|
Let's do it. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Fixes #4775. See that for explanation.
I'm not really a fan of how many places a new option has to be specified for it to actually come through.
Test plan
Added integrationtest