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
Mangled test output regression after problematic fix #1124
Comments
Can you enable the Dump feature (runsettings) and check if the xml from the framework/engine appends that character, or see what is actually present there. That way we can see if this is adapter or engine/framework. |
@OsirisTerje sure 🙂 I already verified that the problem was with the adapter, though, by changing the package version. The problem was not present in 4.2.1, but has been present in all versions since 4.3.0. Anyways, running <test-suite type='TestSuite' id='0-1003' name='TestProject2' fullname='TestProject2' runstate='Runnable' testcasecount='1' result='Passed' start-time='2023-09-20T14:10:15.2894068Z' end-time='2023-09-20T14:10:15.2990287Z' duration='0.009621' total='1' passed='1' failed='0' warnings='0' inconclusive='0' skipped='0' asserts='0'>
<test-suite type='TestFixture' id='0-1000' name='Tests' fullname='TestProject2.Tests' classname='TestProject2.Tests' runstate='Runnable' testcasecount='1' result='Passed' start-time='2023-09-20T14:10:15.2905973Z' end-time='2023-09-20T14:10:15.2986402Z' duration='0.008043' total='1' passed='1' failed='0' warnings='0' inconclusive='0' skipped='0' asserts='0'>
<test-case id='0-1001' name='Test1' fullname='TestProject2.Tests.Test1' methodname='Test1' classname='TestProject2.Tests' runstate='Runnable' seed='1895503531' result='Passed' start-time='2023-09-20T14:10:15.2922751Z' end-time='2023-09-20T14:10:15.2959240Z' duration='0.003737' asserts='0'>
<output><![CDATA[UMMM b\3597805
C:\Windows\System32\user32.dll
UMMM b\3597805
]]></output>
</test-case>
</test-suite>
</test-suite> |
@egraff Since you have already found where the error happens, could you do a PR for this? |
@OsirisTerje well, even though I found where the problem is, I don't know how to fix it in a satisfactory way - that seems much more complicated. The problem is that IMO the PR/feature that introduced this bug is fundamentally broken, and should be reverted. But ideally, a fix would try to preserve the idea of what the feature was supposed to be - but that is essentially trying to re-invent a way to achieve that feature without adverse side-effects to the test output. |
@OsirisTerje having though about this some more, I think the right fix - considering the NUnit3TestAdapter is a separate project (and nuget) from the framework/engine - is to revert the aforementioned changeset after all! The author did have a well thought through evaluation (here: nunit/nunit#4126 (comment)) of different options for fixing nunit/nunit#4126, but just (IMHO) landed on the wrong option in the end. My assertion is that it is not possible to fix nunit/nunit#4126 in the test adapter, because by the time the NUnit framework has escaped the color control characters in @Socolin being the original author of the issue and following fix, do you have any thoughts on this? |
For me it seems that if some things are escaped with I agree with the fact that if only some parts are escaped and not the other then we will have a hard time knowing what can be escape or not. And I think this task should be left to the serializer alone and there should not have any custom escaping before and custom un-escaping after. To resolve the issue I see 2 solutions right now:
Let me know if I can help with this |
Ok, I'll do a PR when I find some times, was the code reverted ? or are we waiting after the fix ? (To help me prioritize this) |
The code is not reverted. Better if you can fix this . |
I created a PR #1147 |
Present in NUnit3TestAdapter 4.3.0 and newer.
Description
When printing full file paths for a collection of DLLs on Windows, I found that after the string
\u
has been printed to the test output, subsequent test output would be mangled.Minimal test case:
Resulting output:
Expected output:
Cause
I've found that it's likely caused by this commit: 6a6e422 (#984).
I've not delved into the details of that commit, but it's clear that there is a problem that needs fixing.
The text was updated successfully, but these errors were encountered: