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

Integrate test logger #33

Merged
merged 5 commits into from
Apr 7, 2021

Conversation

Siphonophora
Copy link
Collaborator

Integrate the test logger

Comment on lines 204 to 211
StringBuilder stdErr = new StringBuilder();
foreach (var m in results.SelectMany(x => x.Messages))
{
if (TestResultMessage.StandardErrorCategory.Equals(m.Category, StringComparison.OrdinalIgnoreCase))
{
stdOut.AppendLine(m.Text);
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@codito The acceptance test that checks for stdErr is failing. Any idea why this isn't working? The stdOut is passing based on the code right above it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Siphonophora there may be a typo in line 209, stdErr instead of stdOut.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@codito Oh yeah thanks for spotting that. Unfortunately that didn't fix it. Should the testlogger be capturing text from Console.Error.WriteLine?

Copy link
Contributor

Choose a reason for hiding this comment

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

There may be a bit of confusion. The stderr messages are actually thrown by the Test Framework, not the individual tests, and those are captured separately in Test Logger (see TestRunMessageWorkflow in testlogger). These were earlier captured by the loggers directly:

this.stdErr.AppendLine(e.Message);

This is not same as the TestResult.Messages, which only appears to capture the stdout messages for a single TestResult.

if (e.Result.Messages.Count > 0)

We will need to expose the run level messages to Serializer in this call: https://github.com/spekt/testlogger/blob/f5704c09657c020278c25f7d344f6648274530c8/src/TestLogger/Core/TestRunCompleteWorkflow.cs#L38.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do let me know if you'd like to take a shot at fixing the Serializer API in testlogger

Copy link
Collaborator Author

@Siphonophora Siphonophora Mar 30, 2021

Choose a reason for hiding this comment

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

Yeah, I think I understand what you mean. The Junit format doesn't support run level test messages so I haven't been thinking in those terms much.

I was looking at the code the other day and wasn't sure how the messages were getting into the TestResultInfo. I'm was kind of thrown by this, because it looked like something was supposed to happen with the messages in the TestResultStore but was missing. I saw the Xunit adapter isn't implemented yet, so maybe something special was going to happen there.
https://github.com/spekt/testlogger/blob/f5704c09657c020278c25f7d344f6648274530c8/src/TestLogger/Extensions/DefaultTestAdapter.cs#L9-L16

For the change, are you just thinking we would add a parameter for messages here, or something more complex? I can take care of it either way.

https://github.com/spekt/testlogger/blob/f5704c09657c020278c25f7d344f6648274530c8/src/TestLogger/Core/TestRunCompleteWorkflow.cs#L38-L41

Copy link
Contributor

Choose a reason for hiding this comment

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

the Xunit adapter isn't implemented yet, so maybe something special was going to happen there.
https://github.com/spekt/testlogger/blob/f5704c09657c020278c25f7d344f6648274530c8/src/TestLogger/Extensions/DefaultTestAdapter.cs#L9-L16

This is an interesting case. Xunit test framework has a unique behavior to output the skipped test reasons as run level messages. That forced us to create https://github.com/spekt/testlogger/blob/master/src/TestLogger/Extensions/XunitTestAdapter.cs#L19 so that the loggers do not replicate adapter specific logic. Fortunately, other test frameworks do not need this, hence DefaultTestAdapter remains empty :)

For the change, are you just thinking we would add a parameter for messages here, or something more complex?

We can just add a new parameter there.

@Siphonophora
Copy link
Collaborator Author

This should be all set. This is what it outputs:

    <system-out>{EEEE1DA6-6296-4486-BDA5-A50A19672F0F}
{C33FF4B5-75E1-4882-B968-DF9608BFE7C2}

{2010CAE3-7BC0-4841-A5A3-7D5F947BB9FB}
{998AC9EC-7429-42CD-AD55-72037E7AF3D8}


Test Framework Informational Messages:
NUnit Adapter 3.10.0.21: Test execution started
Running all tests in C:\Users\mjc82\Documents\GitHub\junit.testlogger\test\assets\JUnit.Xml.TestLogger.NetCore.Tests\bin\Debug\netcoreapp3.1\JUnit.Xml.TestLogger.NetCore.Tests.dll
NUnit3TestExecutor converted 52 of 52 NUnit test cases
NUnit Adapter 3.10.0.21: Test execution complete
</system-out>
    <system-err>Warning - SetUp failed for test fixture JUnit.Xml.TestLogger.NetFull.Tests.FailingOneTimeSetUp
Warning - System.InvalidOperationException : Operation is not valid due to the current state of the object.
Warning -    at JUnit.Xml.TestLogger.NetFull.Tests.FailingOneTimeSetUp.OneTimeSetUp() in C:\Users\mjc82\Documents\GitHub\junit.testlogger\test\assets\JUnit.Xml.TestLogger.NetCore.Tests\UnitTest1.cs:line 152
Warning - TearDown failed for test fixture JUnit.Xml.TestLogger.NetFull.Tests.FailingOneTimeTearDown
Warning - TearDown : System.InvalidOperationException : Operation is not valid due to the current state of the object.
Warning - --TearDown
   at JUnit.Xml.TestLogger.NetFull.Tests.FailingOneTimeTearDown.OneTimeTearDown() in C:\Users\mjc82\Documents\GitHub\junit.testlogger\test\assets\JUnit.Xml.TestLogger.NetCore.Tests\UnitTest1.cs:line 197
Warning - {D46DFA10-EEDD-49E5-804D-FE43051331A7}
Warning - {33F5FD22-6F40-499D-98E4-481D87FAEAA1}
Warning - SetUp failed for test fixture JUnit.Xml.TestLogger.Tests2.FailingOneTimeSetUp
Warning - System.InvalidOperationException : Operation is not valid due to the current state of the object.
Warning -    at JUnit.Xml.TestLogger.Tests2.FailingOneTimeSetUp.OneTimeSetUp() in C:\Users\mjc82\Documents\GitHub\junit.testlogger\test\assets\JUnit.Xml.TestLogger.NetCore.Tests\UnitTest2.cs:line 107
Warning - TearDown failed for test fixture JUnit.Xml.TestLogger.Tests2.FailingOneTimeTearDown
Warning - TearDown : System.InvalidOperationException : Operation is not valid due to the current state of the object.
Warning - --TearDown
   at JUnit.Xml.TestLogger.Tests2.FailingOneTimeTearDown.OneTimeTearDown() in C:\Users\mjc82\Documents\GitHub\junit.testlogger\test\assets\JUnit.Xml.TestLogger.NetCore.Tests\UnitTest2.cs:line 152
</system-err>

Copy link
Contributor

@codito codito left a comment

Choose a reason for hiding this comment

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

Looks great! 🚀

@Siphonophora Siphonophora merged commit 422c0ff into spekt:master Apr 7, 2021
@Siphonophora Siphonophora deleted the integrate-test-logger branch April 7, 2021 20:22
@Siphonophora
Copy link
Collaborator Author

@codito Awesome. whenever you get a chance to cut a release to nuget, I will write up release notes.

By the way, I have really appreciated working with you on this. Its turned out to be much more successful than I would have ever guessed. Nuget download numbers aren't the best metric, but clearing 1M downloads was very cool.

@codito
Copy link
Contributor

codito commented Apr 8, 2021

@Siphonophora 🎆 🎉 it has been a fantastic journey and probably the fastest to 1.4M :) Thank you for all the hard work! To many more awesome releases 🍻

We have published https://www.nuget.org/packages/JunitXml.TestLogger/3.0.87.

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.

2 participants