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

Added includeStackTrace configuration option #161

Conversation

Hargne
Copy link
Owner

@Hargne Hargne commented May 2, 2023

Copy link
Contributor

@StefanPuia StefanPuia left a comment

Choose a reason for hiding this comment

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

I've made those couple changes here
e7e9f04

expect(reportContent).toBeDefined();
expect(reportContent!.toString().indexOf("at stack trace")).toBe(-1);
});
it.only("should keep stack trace in failure messages if set to true", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

.only probably blocks the rest of the tests in the suite here

Copy link
Owner Author

Choose a reason for hiding this comment

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

Perfect! I must have forgotten to remove it while testing 🙏

@Hargne Hargne requested a review from StefanPuia May 2, 2023 19:54
@StefanPuia
Copy link
Contributor

StefanPuia commented May 2, 2023

took me a while to get my head around what's going on
failureMessages is the error.stack value, which means it will always have the message, stack, and Error: at the beginning
failureDetails on the other hand will have maybe always just Error objects: jest source (creates object here)
so we can probably have something like this here, instead of the regex replace:

/////// htmlreporter.ts:394
// Test Failure Messages
if (
  test.failureDetails &&
  test.failureDetails.length > 0 &&
  this.getConfigValue("includeFailureMsg")
) {
  const failureMsgDiv = testResult.ele(
    "div",
    {
      class: "failureMessages",
    },
    " "
  );
  test.failureDetails.forEach((err: Error) => {
    failureMsgDiv.ele(
      "pre",
      { class: "failureMsg" },
      this.sanitizeOutput(err.message)
    );
  });
}

test.failureMessages
.map((failureMsg) =>
!this.getConfigValue("includeStackTrace")
? failureMsg.split("at")[0].trim().replace(/\n+$/, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@StefanPuia StefanPuia left a comment

Choose a reason for hiding this comment

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

could we use failureDetails instead?

@Hargne
Copy link
Owner Author

Hargne commented May 3, 2023

@StefanPuia I get your idea. However, as far as I can see failureDetails is of type: Array<unknown>, meaning that we cannot be sure what it returns. I tried to investigate if there was a type we could use, but to no avail. In your example above you typed it as Array<Error>, but in fact it returns an array of objects:

[
  {
    matcherResult: {
      actual: 'something',
      expected: 'something-else,
      message: 'expect(received).toEqual(expected) // deep equality' +
        '\n' +
        'Expected: "something-else' +
        'Received: "something"',
      name: 'toEqual',
      pass: false
    }
  }
]

I cannot see the stack trace in there either, which is the main reason of this PR: to hide/show the stack trace. And in the end: doing this yields the same result as using failureMessages, so I cannot really see the benefit.

@StefanPuia
Copy link
Contributor

My bad, I only included the replacement for the current version. For this issue specifically we'd just have to replace
this.sanitizeOutput(err.message) with something like this.sanitizeOutput(this.getConfigValue("includeStackTrace") ? err.stack : err.message)

I suppose using unknown is not great, but I'm not a huge fan of the substring solution either... That will have adverse effects if the characters at exist anywhere to the left of the stack trace (e.g.:

                            ⌄ will probably get trimmed here
Error: Some error message that contains the keyword
  Expected: 1
  Received: 2
  
  at Foo.bar (Foo.js:12:5)

maybe something like this would work better?

failureMsg.split(/\n\s+at/)[0].trim().replace(/\n+$/, "")

@Hargne
Copy link
Owner Author

Hargne commented May 3, 2023

Good point about the regex, and I totally agree. I will look at a more solid solution.

For this issue specifically we'd just have to replace
this.sanitizeOutput(err.message) with something like this.sanitizeOutput(this.getConfigValue("includeStackTrace") ? err.stack : err.message)

failureDetails does not have a stack property, only a matcherResult object which may contain a message without the stack trace

@StefanPuia
Copy link
Contributor

That's fair enough. failureDetails would probably be up to the test runner implementation so it's better not to assume anything about it.

@Hargne Hargne requested a review from StefanPuia May 3, 2023 09:55
@Hargne Hargne merged commit 722aebb into master May 3, 2023
@Hargne Hargne deleted the 160-added-into-report-configuration-additional-flag-includestacktrace-false branch May 3, 2023 10:38
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

Successfully merging this pull request may close these issues.

Added into report configuration additional flag "includeStackTrace": false
2 participants