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.
not sure about hardcoding the index. Thoughts? Can traverse all and return the first one which is not inside node_modules instead, perhaps?
|
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.
I need to update https://facebook.github.io/jest/docs/en/configuration.html#testresultsprocessor-string as well
EDIT: done
|
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