Fix stack trace output for TestNG failures

[Kropie]

This will fixes #1260.

[jdneo]

Thank you for your contribution, I'll take a look this week

[jdneo]

I tested the PR locally, but seems it's still not showing the errors according to the steps in #1260? Did I miss anything?

BTW, the refactoring looks good.

[jdneo]

@Kropie Would you like to update your PR?

[Kropie]

@jdneo I will update my pull request tomorrow morning with the recommendations that you provided above. Thanks for your help on this!

[Kropie]

@jdneo I've updated this PR to address the comments that you have above. Additionally I cleaned up the failure processing logic to clean up the "peek" view for test failures.

[Kropie]

This is the result of the latest commit when running a failing test.

[jdneo]

@Kropie The PR looks good now. the common logic is shared with this refactoring. But seems that it does not fix #1260, did I miss anything?

#1260 is a bug that when the suite is failed to run, nothing shows in UI except for the output channel. You can try download the project attached in #1260, and click run all.

[Kropie]

@jdneo I may have missed a portion of the problem statement for #1260, but this does solve the formatting issues that were previously present when you would print out the results from the "peek error" screen. Additionally this resolves the stack trace issues in the preview.

Should the stack trace also be printing out on the debug console?

[Kropie]

After looking running with the code linked in the pull request, this PR will resolve the formatting issues. That being said, if an exception is thrown while the tests are running all subsequent tests will not be able to run.

[Kropie]

Finally figured out what I believe is the root of the problem. The tests in the example project use groups, but the test runner currently does not support selenium groups. I just pushed another commit that logs the error clearly to the user.

I think with the additional logging we have addressed the original issue in #1260 and an additional ticket should be written for adding TestNG group support to the java test runner extension.

Now when you run the tests you will see the following error message in the debug console.

[Kropie]

This is an example of an error that is logged to the debug console to make things clearer to the user

[jdneo]

The screenshot looks good. I'll take a deep look tomorrow.

I think with the additional logging we have addressed the original issue in #1260 and an additional ticket should be written for adding TestNG group support to the java test runner extension.

Yes as you said, the current implementation of the TestNG runner has a lot of limitations. I'm hoping to migrate to the TestNG's official runner: https://github.com/testng-team/testng-remote, but so far does not have time to do this. 😥

[Kropie]

@jdneo Other than merging in the latest changes in from master is there anything else that needs to be done to merge and close this PR?

[jdneo]

As I commented above, the refactor introduced a regression for the stacktrace of the TestNG cases. See: #1286 (comment)

That needs to be resolved before merging.

[Kropie]

I’ll resolve that tomorrow, thanks!

[Kropie]

As I commented above, the refactor introduced a regression for the stacktrace of the TestNG cases. See: #1286 (comment)

That needs to be resolved before merging.

Okay, @jdneo I believe I've resolved the issue with the stack trace that is being presented in the error message. I'm not positive what exactly it is supposed to look like but I've come up with what I believe is the right information.

JUnit

TestNG

[jdneo]

I just tried with the new commit but I'm afraid it introduced new regression.

Previously, For JUnit, we can show the assertion failure via the diff view:

But now, it becomes to a plain text with a single line stack trace:

I guess you thought it a little bit complicated, base on this commit, you can simply make the following change to make it work:

In TestNGRunnerResultAnalyzer, in the else if (outputData.name === TEST_FAIL) block :

...
if (outputData.attributes.trace) {
    ...
    for (const line of outputData.attributes.trace.split(/\r?\n/)) {
        this.processStackTrace(line, markdownTrace, undefined, this.currentItem, this.projectName);
    }

    const testMessage: TestMessage = new TestMessage(markdownTrace);
    if (item.uri && item.range) {
        testMessage.location = new Location(item.uri, item.range);
    }
    testMessages.push(testMessage);
}
const duration: number = Number.parseInt(outputData.attributes.duration, 10);
setTestState(this.testContext.testRun, item, this.currentTestState, testMessages, duration);
...

Just to provide more background information. We have two different test message:

1. The stack trace, which is the one you are dealing with
2. The assertion failure(expected ... but actual ...) showing with a diff view. This is the third argument of your extracted function processStackTrace. This is optional, in TestNG, we didn't support it, so you can just pass undefined to it.

meanwhile, it's great that you also extracted the code to set the location of the test messages, but that can be addressed in another PR (to avoid this one become bigger and bigger endlessly).

[Kropie]

I just tried with the new commit but I'm afraid it introduced new regression.

Previously, For JUnit, we can show the assertion failure via the diff view:

But now, it becomes to a plain text with a single line stack trace:

I guess you thought it a little bit complicated, base on this commit, you can simply make the following change to make it work:

In TestNGRunnerResultAnalyzer, in the else if (outputData.name === TEST_FAIL) block :

...
if (outputData.attributes.trace) {
    ...
    for (const line of outputData.attributes.trace.split(/\r?\n/)) {
        this.processStackTrace(line, markdownTrace, undefined, this.currentItem, this.projectName);
    }

    const testMessage: TestMessage = new TestMessage(markdownTrace);
    if (item.uri && item.range) {
        testMessage.location = new Location(item.uri, item.range);
    }
    testMessages.push(testMessage);
}
const duration: number = Number.parseInt(outputData.attributes.duration, 10);
setTestState(this.testContext.testRun, item, this.currentTestState, testMessages, duration);
...

Just to provide more background information. We have two different test message:

1. The stack trace, which is the one you are dealing with
2. The assertion failure(expected ... but actual ...) showing with a diff view. This is the third argument of your extracted function processStackTrace. This is optional, in TestNG, we didn't support it, so you can just pass undefined to it.

meanwhile, it's great that you also extracted the code to set the location of the test messages, but that can be addressed in another PR (to avoid this one become bigger and bigger endlessly).

First, thanks for your patience!
Second, I was able to resolve the regression with the "Expected ... but actual ... logic" that you referenced above.

I pushed the update out based on my last commit because the delta was rather small. Let me know if you would still rather have me do the code update that you referenced above on the previous commit.

[jdneo]

Thanks for the quite response! Personally, I would prefer to reset to the commit I mentioned and append commit based on that. (To make the total change as simple as possible). We can definitely address other issues by several PRs separately.

[Kropie]

Thanks for the quite response! Personally, I would prefer to reset to the commit I mentioned and append commit based on that. (To make the total change as simple as possible). We can definitely address other issues by several PRs separately.

Just tried to make the modification to the commit referenced above, but that has issues that are addressed by my latest commit. Specifically if you have a stack trace with multiple elements in it and you have a failure message instead of a "expected .. but actual ..." message you will only see the first element on the stack at the time of the failure.

Additionally for TestNG tests you only ever get the first element on the stack at the time of failure.
Also, for help visualizing the problem:

Limited Stack Trace (Update of referenced commit)

Full Stack Trace (Current latest commit in this PR)

[Kropie]

That being said I am okay with updating that previous commit and just creating another PR if that is your preference.

[jdneo]

I prefer to resolve them separately in different PRs. So this PR is aiming to resolve #1260 & #1291.

I believe the problem you mentioned is similar to #1266, and we can for sure resolve that problem in another PR.

Please correct me if I misunderstand

[Kropie]

That works for me. I'll have the pull request updated tomorrow

[Kropie]

@jdneo reverted back to the previous commit and made the suggested change to the TestNG analyzer.

[jdneo]

Seems there are lint errors:

ERROR: /home/runner/work/vscode-java-test/vscode-java-test/src/runners/testngRunner/TestNGRunnerResultAnalyzer.ts:90:1 - trailing whitespace

[Kropie]

Seems there are lint errors:

ERROR: /home/runner/work/vscode-java-test/vscode-java-test/src/runners/testngRunner/TestNGRunnerResultAnalyzer.ts:90:1 - trailing whitespace

Just resolved them

[jdneo]

Thank you! I'll take a look later today/tomorrow

[jdneo]

Overall good, but just one small peace, please see my comment.

[Kropie]

@jdneo is there anything else for this PR I need to do? (Reference response in commit review)

[jdneo]

@Kropie, looks pretty good! Thank you for the nice job!

[Kropie]

@Kropie, looks pretty good! Thank you for the nice job!

Thanks for the guidance!