-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add system-err and system-out to testcase #47
base: master
Are you sure you want to change the base?
Conversation
f89c1a5
to
b2bddea
Compare
.Where(x => x.Descendants().Any(y => y.Name.LocalName == "system-err")) | ||
.ToList(); | ||
|
||
Assert.AreEqual(1, testCasesWithErrorOut.Count()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Siphonophora I'm having trouble getting system-err to work on a per-testcase basis. Here I would expect the stderr from FailTest11()
to be found. Any ideas why it's not getting picked up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mclayton7 I suspect its due to how nunit, which we use for these tests, is behaving. Looking at their docs, they make the distinction between standard output whose messages go to the test level and errors which get displayed right away. https://docs.nunit.org/articles/nunit/writing-tests/TestContext.html
Based on what we see in the loggers, I think the immediate messages are coming across as test messages https://github.com/spekt/testlogger/blob/master/src/TestLogger/Core/TestRunMessageWorkflow.cs but are not attached to the results. While normal console.writeline is going to both the standard out and is being attached to the test result.
I can look more later in the week, but if i'm right then this behavior will depend on the test library and we won't be able to do much on the logging side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Siphonophora I think you're correct. I can remove the stderr logging here since it doesn't work, and just leave the stdout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good if we could do a quick test with mstest and xunit, so we can put a note in the docs about how each behaves.
|
||
if (!string.IsNullOrWhiteSpace(stdOut.ToString())) | ||
{ | ||
testcaseElement.Add(new XElement("system-out", stdOut.ToString())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also didn't wrap this with XCData
like the NUnit one because I wanted to match the existing system-out/system-err printing. I don't know enough about JUnit to know if parsers should expect CDATA or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also something we could do a quick test on. But if the logged text has any xml characters that need to be escaped, I expect thats what will happen here. If we wrap it in CDATA, the characters won't be escaped and should therefore be more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Siphonophora would you like me to wrap the per-testcase output in CDATA and the per-testsuite output? I'd consider this to be a breaking change because it's not currently wrapped on line 229.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I would call it a breaking change so long as we are still passing the xml validation. But, we can break this out to its own issue and decide on it later.
The other thing I think we need here is to change the documentation in the main readme to reflect the change in the schema we are using. It should also clarify that the system-out will show up at the test case level AND at the file level. I don't have any good ideas on how to prevent this duplication. |
@mclayton7 Do you want to circle back to finish this? If you don't have time I will see if I can move it forward. |
@Siphonophora sorry I lost track of this, I started looking into how other test runners worked in this repository: https://github.com/mclayton7/dotnet_test_output_reporting which led me down the rabbit hole of GitHub actions. I'll pick this back up and try to finish it this weekend! |
@mclayton7 No worries. I appreciate the help. |
b2bddea
to
507b9cb
Compare
@@ -47,6 +47,89 @@ test logs from over-writing eachother. | |||
|
|||
[config-wiki]: https://github.com/spekt/testlogger/wiki/Logger-Configuration | |||
|
|||
### Junit Output Notes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: JUnit
@@ -47,6 +47,89 @@ test logs from over-writing eachother. | |||
|
|||
[config-wiki]: https://github.com/spekt/testlogger/wiki/Logger-Configuration | |||
|
|||
### Junit Output Notes | |||
|
|||
The xml reports will have standard output (stdout) and standard error (stderr) from the test output. Stdout is logged on a per-testcase basis, but stderr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section looks a bit incomplete. The beginning trails off and parts of mstest seems missing. I think the examples are helpful, but lets put them in another file for just the standard and error out.
Since you are pointing out that failed tests behave differently can we just show all the options. Just 2 test methods should be fine.
*Passing Test
* std out
* error out
*Failing Test
* std out
* error out
I think it would be fine to have the docs show more complete examples in a details
tag if its getting too long.
In this file, lets have a short section after ### Customizing Junit XML Contents
which points out that this stuff varies per testing framework, and links to all the details.
|
||
- <https://github.com/spekt/nunit.testlogger> | ||
- <https://github.com/spekt/xunit.testlogger> | ||
- <https://github.com/spekt/appveyor.testlogger> | ||
|
||
## Usage | ||
|
||
The JUnit Test Logger generates xml reports in the [Ant Junit Format](https://github.com/windyroad/JUnit-Schema), which the JUnit 5 repository refers to as the de-facto standard. While the generated xml complies with that schema, it does not contain values in every case. For example, the logger currently does not log any `properties`. Please [refer to a sample file](docs/assets/TestResults.xml) to see an example. If you find that the format is missing data required by your CI/CD system, please open an issue or PR. | ||
The JUnit Test Logger generates xml reports in the [Jenkins JUnit Format](https://github.com/junit-team/junit5/blob/main/platform-tests/src/test/resources/jenkins-junit.xsd), which the JUnit 5 repository refers to as the de-facto standard. While the generated xml complies with that schema, it does not contain values in every case. For example, the logger currently does not log any `properties`. Please [refer to a sample file](docs/assets/TestResults.xml) to see an example. If you find that the format is missing data required by your CI/CD system, please open an issue or PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets make sure to update the sample file once all other changes are complete.
Is this MR still alive? last updates were from 2021. I have an urgent requirement to provide a way to supress and redirect std-our and std-error from the test results XML to a separate file and wondered if I should pick up from here or start fresh? Background: We have multiple test suites generating 170-250 mb of std-out/err per suite. Gitlab completely chokes on such large results files. We obviously need the output for debugging and diagnostics so suppression on its own isn't good enough, I want to redirect to a secondary log file instead and expose as a build artefact. Thanks |
@davetheninja This one isn't dead per se, but I think what you are talking about is a different change altogether. It seems like what you really need is a feature flag to suppress logging of all std/error out in the junit report. That would be a simple change, and I could see it making sense to do. For the other piece, I don't think this logger should do redirection. I would suggest taking a look at the trx or html loggers that are built into vstest. https://github.com/microsoft/vstest-docs/blob/main/docs/report.md You can run more than one logger at a time |
Yes, actually came to same conclusion after going through the code. Redirection doesn’t fit the contract for the serialiser. That being said, suppression is not an option for me, I need the data available. Have come up with a possible solution which I will try tomorrow where I have the test results cml being generated as usuals, but have a .post hook in gitlab ci stage and use power shell to extract the error and out elements to two log files - removing the out err body within the results xmlThat should hopefully kill both birds with one stone.
Thanks
David
|
properties
tagsystem-out
andsystem-err
tags to individual test cases