Add failureDetails property to circus test results
#9496
Conversation
|
@SimenB this was meant to be a draft PR to get your input on how I could improve on this, but I ctrl+enter'd by mistake 😂 |
Codecov Report
@@ Coverage Diff @@
## master #9496 +/- ##
==========================================
- Coverage 65.04% 65.03% -0.02%
==========================================
Files 286 286
Lines 12140 12144 +4
Branches 3008 3009 +1
==========================================
+ Hits 7897 7898 +1
- Misses 3605 3608 +3
Partials 638 638
Continue to review full report at Codecov.
|
|
The What you essentially wanna do is almost always use the first exception, as we don't care about the stack trace one (I think - if we care about the stack trace you should merge them into a single object). We don't wanna leak this implementation detail to reporters |
|
@G-Rath were you able to look at ths again? |
|
@SimenB apologies I've not had the bandwidth to jump back into this 😬 If you're wanting to fast track it, I can shuffle things around - the size & complexity of the codebase is one of the primary reasons I've not had a chance to work on it, as it requires me to book out a large chuck of time if I hope to make decent progress, so having someone who knows the codebase would be a huge help 🙂 |
|
No problem at all, just checking you didn't miss my response 🙂 Not sure if it would help or not, but Christoph has put together a video explaining the architecture of Jest, linked here: https://jestjs.io/docs/en/architecture Anything in particular you're stuck on that I could help unblock? |
All good - I should have put a reply or a reaction 😄
I think the biggest issue I have is that there's a lot of components to jest, and testing frameworks in general, that I need to get comfortable with. I'm slowly getting a better understanding of how things work, and where to make changes, but unfortunately it's one of those things that just requires time and practice. For this PR in particular, I think the primary issue I've had is that I don't have a clear understanding on the desired output, as I've not worked with the reporter API so can't really look at the output and say "yes this is what we want and is useful" with my usual confidence. The other issue I've had in in trying to make a good test, which ties into the stuff above - the codebase size & components in play mean I've had a hard time nailing down the actual right way to test this change in a meaningful way. Writing this out & re-reviewing the work has refreshed my head on this, so I'm going to try and tackle it over the next few days :) |
|
This looks like what we want 😄: reporter outputTaken using the following: Promises don't seem to provide |
|
Looks pretty good! I think we should make sure diff --git c/packages/jest-circus/src/utils.ts w/packages/jest-circus/src/utils.ts
index 58d14f513..b3ecc0950 100644
--- c/packages/jest-circus/src/utils.ts
+++ w/packages/jest-circus/src/utils.ts
@@ -288,7 +288,7 @@ const makeTestResults = (
testResults.push({
duration: test.duration,
errors: test.errors.map(_formatError),
- errorsDetailed: test.errors.map(_formatErrorDetails),
+ errorsDetailed: test.errors.map(_getError),
invocations: test.invocations,
location,
status,
@@ -316,9 +316,9 @@ export const getTestID = (test: Circus.TestEntry): string => {
return titles.join(' ');
};
-const _formatError = (
+function _getError(
errors?: Circus.Exception | [Circus.Exception | undefined, Circus.Exception],
-): string => {
+): Error {
let error;
let asyncError;
@@ -330,29 +330,22 @@ const _formatError = (
asyncError = new Error();
}
- if (error) {
- if (error.stack) {
- return error.stack;
- }
- if (error.message) {
- return error.message;
- }
+ if (error && (error.stack || error.message)) {
+ return error;
}
asyncError.message = `thrown: ${prettyFormat(error, {maxDepth: 3})}`;
- return asyncError.stack;
-};
+ return asyncError;
+}
-const _formatErrorDetails = (
+function _formatError(
errors?: Circus.Exception | [Circus.Exception | undefined, Circus.Exception],
-) => {
- if (Array.isArray(errors)) {
- return errors[0];
- }
+): string {
+ const error = _getError(errors);
- return errors;
-};
+ return error.stack || error.message;
+}
export const addErrorToEachTestUnderDescribe = (
describeBlock: Circus.DescribeBlock, |
|
I like it! I'll commit it right now :) |
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #9496 +/- ##
==========================================
- Coverage 65.04% 65.03% -0.02%
==========================================
Files 286 286
Lines 12140 12138 -2
Branches 3008 3006 -2
==========================================
- Hits 7897 7894 -3
Misses 3605 3605
- Partials 638 639 +1
Continue to review full report at Codecov.
|
|
@segrey does this look like something you can use? |
This comment has been minimized.
This comment has been minimized.
Sorry for the delay. Yes, seems If it's possible to build jest locally with the patch applied, I can try it out locally and let you know how it goes. Any hints? |
Depending on what you're referring to by
You can checkout my fork, build, and then link or copy the resulting Here's the repo/code I've been using for testing these changes, complete with applied patches. Running On an aside, it seems that the "highlight problem line in test" inspection has stopped working - I've reported it as WEB-43995. |
|
Awesome, patching leaks is always good - cheers for the heads up. Currently this is actually failing because of
|
Codecov Report
@@ Coverage Diff @@
## master #9496 +/- ##
==========================================
- Coverage 64.24% 64.22% -0.02%
==========================================
Files 292 292
Lines 12466 12465 -1
Branches 3078 3074 -4
==========================================
- Hits 8009 8006 -3
- Misses 3809 3810 +1
- Partials 648 649 +1
Continue to review full report at Codecov.
|
|
@G-Rath fixed on master, sorry about that. We got a bit excited when we decided to do a new major and merged some bad merge conflict resolutions 😛 |
|
Ok now this is just getting silly 😂
|
ugh I was hoping past me had added one, but apparently I was hoping future me would do so. |
|
@SimenB so I've gotten around to taking a stab at implementing tests, but I'm not sure the best way to actually go about it - would you have any recommendations? So far I've got a test that runs the tests in my sample repo using the custom matcher, and makes a snapshot of the output; while that works, the snapshot contains personal file paths in the stacktrace, which'll mean the tests will fail if run anywhere but on my local machine 😬 |
|
Just going to put this here for reference: It seems that sometimes the stack trace can include |
|
@SimenB I think this is good to go, except the tests are failing due to variations in the stack trace - any ideas on how to handle this? There's not a consistent enough pattern that I can see to craft a string transformer that doesn't undermine the point of the snapshot test a bit, unless we're happy with just dropping the strack-trace all together? |
|
Stripping out the stack in the snapshot entirely seems reasonable to me |
|
@G-Rath could you resolve the conflict? Ready to land after that 🙂 |
|
Oh, and a changelog entry 👍 |
|
@SimenB things are looking good here - the failing check is CircleCI, and seems to be because of memory leaks that I don't think are related to this change. |
|
Thanks! |
|
@segrey this is released in https://github.com/facebook/jest/releases/tag/v26.3.0. If you could give it a spin that'd be lovely - Jest Circus is gonna be the default in Jest 27, and while we don't a have a timeline, it'd be awesome if the integration for IDEA IDEs was good from the start 🙂 |
|
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
This is a first pass run at the second part of #8840
I used this to get the details:
This seems to be working nice, but the actual data type is interesting:
It's an array of what seems to be sometimes an array inside another array with just a stracktrace, sometimes it's got matcherResult - I'm sure some of it has good reason (i.e similar to why eslint takes an array of configurations for a rule rather than an object), but I imagine it'd make things easier for @segrey if we could ensure even a little bit of structure.
In saying that, I also would imagine there's only so much jest can do, since it's ultimately on the reporters to report their results.
The matchers results are definitely there:
Test plan
Probably to have a custom reporter that expects the results from circus to include the
failureDetailsproperty.