Added flag to always include output in junit reports

[alexxed]

fixes #118

[alexxed]

No test has failed, so I see no reason to remove the output from memory here, even if the flag is off. If we do want to remove it if the flag is off, due to memory concerns, need to pass the cfg up to here.

[dnephin]

Ya, the reason for deleting the output here is that it could be very large, and holding all the output in memory could cause problems in CI.

I'm wondering if we can change the JUnit XML file handling to be more like the jsonfile handling. That's done here:

gotestsum/cmd/handler.go

Lines 39 to 46 in 90a35cf

	if err := writeWithNewline(h.jsonFile, event.Bytes()); err != nil {
	return fmt.Errorf("failed to write JSON file: %w", err)
	}
	if event.Action.IsTerminal() {
	if err := writeWithNewline(h.jsonFileTimingEvents, event.Bytes()); err != nil {
	return fmt.Errorf("failed to write JSON file: %w", err)
	}
	}

If we can write the output each time a test passes or fails then we only need to buffer output for running tests, instead of buffering all of the output for every test in every package.

There will definitely be some challenges with that approach. The handler.Event runs after Package.addTestEvent, so the operation to "remove output" would need to move somewhere else, and there's currently no good place for it to move. The junit xml writer requires a lot more state than the jsonfile, so the implementation would be more involved.

Maybe the Junit XML writer could buffer the output itself, instead of relying on the buffer in Package.

[dnephin]

It might also be possible to change the interface between handler and Package so that the output is returned before it is removed, but I'm not sure yet what that would look like.

[ethan-gallant]

I've tested this change in my Jenkins environment and can confirm it works as expected. It would be awesome if this could make it into a release 🚀

[dnephin]

Thank you for the PR! This is looking good, but as you noticed the big challenge here is how to buffer the output for running tests without having to keep the output of all tests in memory.

[dnephin]

Ya, the reason for deleting the output here is that it could be very large, and holding all the output in memory could cause problems in CI.

I'm wondering if we can change the JUnit XML file handling to be more like the jsonfile handling. That's done here:

gotestsum/cmd/handler.go

Lines 39 to 46 in 90a35cf

	if err := writeWithNewline(h.jsonFile, event.Bytes()); err != nil {
	return fmt.Errorf("failed to write JSON file: %w", err)
	}
	if event.Action.IsTerminal() {
	if err := writeWithNewline(h.jsonFileTimingEvents, event.Bytes()); err != nil {
	return fmt.Errorf("failed to write JSON file: %w", err)
	}
	}

If we can write the output each time a test passes or fails then we only need to buffer output for running tests, instead of buffering all of the output for every test in every package.

There will definitely be some challenges with that approach. The handler.Event runs after Package.addTestEvent, so the operation to "remove output" would need to move somewhere else, and there's currently no good place for it to move. The junit xml writer requires a lot more state than the jsonfile, so the implementation would be more involved.

Maybe the Junit XML writer could buffer the output itself, instead of relying on the buffer in Package.

[dnephin]

Maybe --junit-include-output=all (with a default value of failures) or something like that? "always" I think doesn't make a clear distinction between the two modes: "all output", and "only failure output".

[alexxed]

I'll get back to this eventually to address the feedback, meanwhile I pushed small change to address reruns problems. Currently if there is a rerun that passes, junit report will include both fail and pass, and tools that read that (e.g. Jenkins) will sum those up and consider the test failed.

[dnephin]

Does anyone have an example of the schema that puts system-out under test-case ? If I look at all the top hits on google (https://github.com/windyroad/JUnit-Schema/blob/master/JUnit.xsd, https://github.com/testmoapp/junitxml, https://stackoverflow.com/a/9410271/444646) they put system-out as a sibling to test-case, which does not match the implementation in this PR.