Separate the Stack and The Error from a TestCase Failure

[mastrzyz]

Currently a big problem is that we are only using the inner text of the Failure like so:

    <testcase classname="sanity_window_apps.ui.test.ts" name="Tab Test Sanity" time="33.061">
      <failure>Error: [Test] ❌ Unable to find element of type xpath</failure>
    </testcase>

With the code being like this: ie treating what's inside of the failure object as a pure string :

      testCase.testcase.push({
        [tagName]: stripAnsi(failure)
      });

This effectively makes no differentiation between the Stack trace of a Failure or the message.

While the JUNIT standard is loose, officially Azure Devops treats the inner text part of Failure as stack trace while the attribute message as the actual message

| Error message | /Testsuites/testsuite/testcase/failure.Attributes["message"].Value Or /Testsuites/testsuite/testcase/error.Attributes["message"].Value Or /Testsuites/testsuite/testcase/skipped.Attributes["message"].Value
 | Stack trace | /Testsuites/testsuite/testcase/failure.InnerText Or /Testsuites/testsuite/testcase/error.InnerText

So we would get the following back by changing that :

    <testcase classname="sanity_window_apps.ui.test.ts" name="Tab Test Sanity" time="33.061">
      <failure message="Error: [Test] ❌ Unable to find element of type xpath">    at Generator.next (&lt;anonymous&gt;)
    at C:\src\f\node_modules\tslib\tslib.js:116:75
    at new Promise (&lt;anonymous&gt;)</failure>
    </testcase>

[mastrzyz]

@palmerj3 would like your thoughts, this PR isn't ready just this is a major possible change here.

[palmerj3]

I like that.

This makes a lot of sense to me. If you can stabilize this I'd be happy to take a look and merge.

[mastrzyz]

I like that.

This makes a lot of sense to me. If you can stabilize this, I'd be happy to take a look and merge.

Thanks, so far I got the existing UT's fixed, working to see of any edge cases I might of missed..

[mastrzyz]

@palmerj3 verified in our CI it works! we are able to see stack traces in our CI/CD system yay!.

But would like your eyes , in theory is this a major version change since the format may be different?

[mastrzyz]

@palmerj3

[mastrzyz]

@palmerj3

[mastrzyz]

@palmerj3

[palmerj3]

Sorry for the delay in reviewing. I was traveling for work and then was ill.

Looking more closely at this, I am worried it will generate invalid XML. Escape codes in XML won't pass many XML parsers.

There is a PR and related issue that are in direct conflict with this change. See https://github.com/jest-community/jest-junit/pull/230

I would be more willing to add something like this if it wasn't on by default and had a configuration option users could utilize to enable it.

[mastrzyz]

So if I understand correctly, the current worry is that adding this new information might cause the XML to not be valid since some people might have strange stack trace messages with escape characters or unicode.

I can push this under a config option but not sure how that PR conflicts with mine.

[palmerj3]

Perhaps start a thread over there and discuss

[ivailop]

Separation of error-message and stack-trace is the way many (if not most) JUnit reporters do. It would be nice if this reporter also follows the established convention...

Regarding the stack-trace, shouldn't you better wrap it in CDATA - that way there would not be any worry of breaking the XML? Notice the way suite.console (which could also contain unsafe characters) is handled...

[mnoorenberghe]

@mastrzyz Now that #230 has merged can this be rebased?

The lack of @message was causing test-results-reporter/parser to print undefined for errors and that would be fixed by this change: https://github.com/test-results-reporter/parser/issues/73#issuecomment-2220545099