search

xunit / xunit

xUnit.net is a free, open source, community-focused unit testing tool for .NET.
https://xunit.net/
Other
4.22k stars 778 forks source link

InlineData and implicit conversions #1742

Open FantasticFiasco opened 6 years ago

FantasticFiasco commented 6 years ago

Expected Behavior

  1. The ReSharper test runner and the VSTest runner should not hang.
  2. dotnet test should pass if dotnet xunit pass

Actual Behavior

  1. When running the test defined below the runners hang. One has to restart Visual Studio or JetBrains Rider in order to run the test again.
  2. dotnet xunit will successfully run the test while dotnet test will report the following error, which actually doesn't describe the real problem, i.e. that the implicit conversion for some reason cannot be handled.
[xUnit.net 00:00:00.5745340] Exception discovering tests from vstest-error: System.ArgumentException: There is at least one object in this array that cannot be serialized
Parameter name: array
   at Xunit.Serialization.XunitSerializationInfo.ArraySerializer..ctor(Array array) in C:\Dev\xunit\xunit\src\common\XunitSerializationInfo.cs:line 417
   at Xunit.Sdk.SerializationHelper.Serialize(Object value) in

Steps to Reproduce the Problem

  1. Create a new xUnit project using the command dotnet new xunit
  2. Paste the following code in the created project: 
    
public class UnitTest1
{
[Theory]
[InlineData("someId")]
public void Test1(Id id)
{
}
}

public class Id { public Id(string value) { Value = value; }

public string Value { get; }

public static implicit operator Id(string id)
{
    return id != null ? new Id(id) : null;
}

}


3. Run the test using the ReSharper test runner or VSTest runner. You will see that the runner hangs.
4. Run the test using `dotnet test`. You will see the cryptic error message
5. Run the test using `dotnet xunit`. You will see that the test pass

## Versions

- Microsoft.NET.Test.Sdk: 15.7.0
- xunit: 2.3.1
- xunit.runner.visualstudio: 2.3.1
- dotnet-xunit: 2.3.1
joshua-light commented 6 years ago

Hey, guys!

I've found similar behaviour in some tests of mine, so I decided to investigate this.

Description

It looks like on xUnit 2.4.0.4010 this works well, at least Resharper isn't hanging. The error is the same as reported by @FantasticFiasco.

But if I take 2.3.1 — everything will freeze. And I actually couldn't be able to start any other tests before shutting down dotnet process through Task Manager.

I've also found that there are other cases when ReSharper hangs, for example:

[Theory]
[InlineData(double.MaxValue)]
public void Test(decimal a) { }

Which in, as I mentioned above, breaks in 2.3.1 but works in 2.4.0.4010 as:

System.OverflowException
Value was either too large or too small for a Decimal.
   at System.Decimal..ctor(Double value)
   at System.Decimal.op_Explicit(Double value)

So it looks like any exception during some prewarm phase can cause freeze no matter of concrete semantics. Good news are that it is probably already fixed in 2.4.0.

Versions

bradwilson commented 6 years ago

@JoshuaLight Thanks for the information! I don't think it's the same problem as the original (in your case, the values "look" convertible to one another once they've been serialized, but don't fit in the destination data type).

joshua-light commented 6 years ago

@bradwilson

Maybe I'm wrong, but I think in both cases we have exceptional situtations: 1) System.ArgumentException: There is at least one object in this array that cannot be serialized for case when you use custom type in method parameters. 2) System.OverflowException: Value was either too large or too small for a Decimal for case, when there is actually uncompilable situation, because you cannot cast double to decimal neither implicitly nor explicitly.

They should be reported as they are (and they're reported in some cases), but it looks like sometimes both exceptions kinda stuck in the throat of ReSharper runner. ))

It can be worth considering to check whether any other exception can hang an environment.

bradwilson commented 6 years ago

The original bug here (1) is an actual bug.

Your new issue (2) is by design (that is supposed to throw an exception).

It can be worth considering to check whether any other exception can hang an environment.

If you find anything which does, please open a new issue. This one is not about exceptions hanging environments, but rather about a feature that should work, that isn't working.

joshua-light commented 6 years ago

Ah now I see, thanks!

mkokabi commented 5 years ago

The original bug here (1) is an actual bug.

Hi, Any update on this issue (bug).

britebit commented 5 years ago

Hi All,

I've had a quick look and I think the problem lies in the CanSerializeObject method of the XUnitSerializationInfo class. I wonder if adding code similar to below would help solve the problem. What do you think?

using System;
using System.Linq;
using System.Reflection;
using Shouldly;
using Xunit;

namespace XunitIssue1742
{
    public class DetectImplicitConversion
    {
        public class Id
        {
            public Id(string value)
            {
                Value = value;
            }

            public string Value { get; }

            public static implicit operator Id(string id)
            {
                return id != null ? new Id(id) : null;
            }

            public static implicit operator string(Id id)
            {
                return id?.Value;
            }
        }

        public class NoImplicitConversions
        {
        }

        [Fact]
        public void DetectsImplicitConversionFromString()
        {
            var otherType =
                typeof(string);

            HasImplicitConversionFromOtherType<Id>(otherType)
                .ShouldBeTrue();

            HasImplicitConversionFromOtherType<NoImplicitConversions>(otherType)
                .ShouldBeFalse();
        }

        [Fact]
        public void DetectsImplicitConversionToString()
        {
            var otherType =
                typeof(string);

            HasImplicitConversionToOtherType<Id>(otherType)
                .ShouldBeTrue();

            HasImplicitConversionToOtherType<NoImplicitConversions>(otherType)
                .ShouldBeFalse();
        }

        /// <summary>
        /// Based on https://stackoverflow.com/questions/32025201/how-can-i-determine-if-an-implicit-cast-exists-in-c/32025393#32025393
        /// </summary>
        private static bool HasImplicitConversionFromOtherType<T>(Type otherType) where T : class
        {
            var typeToConvertTo = typeof(T);

            return typeToConvertTo.GetMethods(BindingFlags.Public | BindingFlags.Static)
                .Where(mi => mi.Name == "op_Implicit" && mi.ReturnType == typeToConvertTo)
                .Any(mi =>
                {
                    ParameterInfo pi = mi.GetParameters().FirstOrDefault();
                    return pi != null && pi.ParameterType == otherType;
                });
        }

        private static bool HasImplicitConversionToOtherType<T>(Type otherType) where T : class
        {
            var typeToConvertFrom = typeof(T);

            return typeToConvertFrom.GetMethods(BindingFlags.Public | BindingFlags.Static)
                .Where(mi => mi.Name == "op_Implicit" && mi.ReturnType == otherType)
                .Any(mi =>
                {
                    ParameterInfo pi = mi.GetParameters().FirstOrDefault();
                    return pi != null && pi.ParameterType == typeToConvertFrom;
                });
        }
    }
}
bradwilson commented 5 years ago

I believe the custom serialization system is being removed in v3 (and there are no more planned releases for v2), so this would become unnecessary.

britebit commented 5 years ago

OK, cool. I couldn't find a release roadmap. Do you know when v3 is scheduled for release?

kolbasik commented 1 year ago

As a workaround for unit test. I have added support of IXunitSerializable to help XUnit to build the correct test case data.

public sealed record CountryCode(string Value) : IXunitSerializable
{
    // It's required by IXunitSerializable
    public CountryCode(): this(string.Empty) { }

    public override string ToString()
    {
        return Value;
    }

    public static implicit operator CountryCode(string self)
    {
        return new CountryCode(self);
    }

    public static implicit operator string(CountryCode self)
    {
        return self.Value;
    }

    void IXunitSerializable.Serialize(IXunitSerializationInfo info)
    {
        info.AddValue(nameof(Value), Value, typeof(string));
    }

    void IXunitSerializable.Deserialize(IXunitSerializationInfo info)
    {
        throw new NotSupportedException("This should not be used.");
    }
}
bradwilson commented 1 year ago

@kolbasik You're throwing in Deserialize. That would never work in Test Explorer (where serialization comes into play).

kolbasik commented 1 year ago

Hi, @bradwilson. You might be right that in some cases it might not work. It have described a case when it helped me. I have made a small demo app.

XUnitRecordsDemo.zip

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.4.1" />
    <PackageReference Include="xunit" Version="2.4.2" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.4.5">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

</Project>

using Xunit;
using Xunit.Abstractions;

namespace XUnitRecordsDemo
{
    public sealed class XUnitTest
    {
        [Theory]
        [InlineData("BE")]
        [InlineData("NL")]
        [InlineData("LU")]
        public void ShouldAcceptRecordsAsArguments(CountryCode countryCode)
        {
            Assert.NotNull(countryCode);
        }
    }

    public sealed record CountryCode(string Value) : IXunitSerializable
    {
        // It's required by IXunitSerializable
        public CountryCode(): this(string.Empty) { }

        public override string ToString()
        {
            return Value;
        }

        public static implicit operator CountryCode(string self)
        {
            return new CountryCode(self);
        }

        public static implicit operator string(CountryCode self)
        {
            return self.Value;
        }

        void IXunitSerializable.Serialize(IXunitSerializationInfo info)
        {
            info.AddValue(nameof(Value), Value, typeof(string));
        }

        void IXunitSerializable.Deserialize(IXunitSerializationInfo info)
        {
            throw new NotSupportedException("It should not be used.");
        }
    }
}

Using cli dotnet test --nologo --logger "console;verbosity=detailed"

image

Using ReSharper Unit Test Session

image

Using Visual Studio 2022 Test Explorer

image

I would be glad if it would help anyone.

atruskie commented 1 year ago

Hi all,

I want to resurrect this just because it was a huge pain to try and debug. I accidentally relied on an implicit cast without knowing the side effects. I used a ReadOnlyMemory<byte> instead of a byte[]

image

The only way I managed to track this down is by disabling every theory and then reenabling them one-by-one.

I'm mainly writing this because searches for the problem led me to this issue and I'm hoping documenting this will help someone else.