ArgumentException when whitespace sent to logger #516
Comments
|
There are different types of events, and vstest depends on the start- and stop test events, so we need to send them, but this is the pure test-output, IIRC, so I can't see why we should not just stop sending anything if it is empty.
@CharliePoole When you did the change on #220 , was there any particular reason not to just skip sending anything if it IsNullOrWhiteSpace ? |
…is empty or just whitespace #516
|
Sorry, for some reason I didn't get an email when the question was asked about a PR. My apologies! |
|
No problem :-) And thanks for raising the issue :-) The package with the fix is available at the myget.org. |
|
@OsirisTerje Neither did I (get an email). Strange. Anyway, I didn't ignore whitespace lines because the assumption is that the user wanted to send them. |
|
@CharliePoole @slide Strange with the emails. I do get the notifications all the time, is there some new approvals needed due to the GDPR checks stopping them (just guessing) ? |
|
I normally do too, but this is the first I've seen from this project for a while. |
|
Hmm.... That may explain why I get very little response lately. |
|
Very odd. Neither @slide nor I are team members, but you @mentioned us so we should get it. |
|
@CharliePoole But do you get emails from the mentions today? |
|
The @mentions were made from an edit which may be why the emails didn't get sent? |
|
That seems like a feasible explanation |
|
@CharliePoole I agree, I think I need to raise an issue with the VSTest project because sending whitespace only should be allowed, it is useful. Thanks for fixing this up! |
|
I noticed I had |
|
@kswoll Can you include a minimal test showing the problem, as I cannot make it fail. E.g. the following works on my machine [Test]
public void TestConsoleWriteLine()
{
Console.WriteLine();
Console.WriteLine("a");
Console.WriteLine((string)null);
Console.WriteLine("b");
Console.WriteLine(" ");
} |
|
Thanks @mikkelbu, it seems to be related to threading. Here's a minimal example: Produces the following exception in the Output Window when Tests is selected: Moving the |
|
Thanks @kswoll ! Can you try this with the 4.0 beta ? |
|
We're running into this issue with the 4.0.0 release of the adapter. I've been able to repro with the following test setup: using System;
using NUnit.Framework;
namespace whitespace_output_test {
public class Tests {
[TestCase( null )]
[TestCase( "" )]
[TestCase( " " )]
[TestCase( "\t" )]
[TestCase( "\r" )]
[TestCase( "\n" )]
[TestCase( "\r\n" )]
public void Test1( string output ) {
Console.Write( output );
Assert.Pass();
}
}
}and project file: <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>net472;net5.0</TargetFrameworks>
<RootNamespace>whitespace_output_test</RootNamespace>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.11.0" />
<PackageReference Include="NUnit" Version="3.13.2" />
<PackageReference Include="NUnit3TestAdapter" Version="4.0.0" />
</ItemGroup>
</Project>Both the I'm not super familiar with the code here, but it seems like the cause might be the discrepancy between the |
|
@mthjones Can you point/link to the code where you see these two checks ? |
|
Hey @OsirisTerje. From the stacktrace, I believe in the NUnit adapter it's hitting this section of code: nunit3-vs-adapter/src/NUnitTestAdapter/NUnitEventListener.cs Lines 162 to 168 in 4acde44 where which is where the |
|
@mthjones Thanks and very nice! I'll make a repro and check if this fixes it. |
|
I confirm that @mthjones point the root problem that I also face. The next code is working with '\t' pass Both the "\t" and " " test cases that produce the error: The strange, in Appveyor, these test although it fire an exception, the Test framework don't account it ( not a fail, or not a pass, it's ignored !!!) 56 test case are ignored.!!! The common error for all test cases: WorkAround |
|
@mthjones @moh-hassan Please try the attached alpha version, 4.1.0-alpha02 |
|
@OsirisTerje |
|
@moh-hassan Thanks! It'll go into the 4.1 release, which I believe will go out within a week. |
This is related to #220. The previous fix was to send a single space when there was no text in the message, but TestSessionMessageLogger checks for IsNullOrWhitespace and throws an ArgumentException if the message is null or only whitespace, (https://github.com/Microsoft/vstest/blob/master/src/Microsoft.TestPlatform.Common/Logging/TestSessionMessageLogger.cs#L65) which means sending the single space causes an argument exception.
These are the versions we are referencing:
We use nunit to run unit tests for IronPython. Cloning https://github.com/IronLanguages/ironpython2 and running
make.ps1thenmake.ps1 test-test_macurl2pathwill show the issue. I worked around this in a local build of the adapter by implementing my own IsNullOrWhiteSpace (since the adapter is being built with a framework prior to IsNullOrWhiteSpace) and didn't send the message if it is null or only whitespace.This is the output we see from tests:
We see this on both net45 and netcoreapp2.0
/cc @slozier
The text was updated successfully, but these errors were encountered: