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

v3.3.0 (and v3.3.1) breaks test: Could not load file or assembly in method with AssemblyInitialize attribute #2653

Closed
Andreas-Dorfer opened this issue Apr 4, 2024 · 18 comments

Comments

@Andreas-Dorfer
Copy link

I have the following test code that works with no issues up to v3.2.2, but fails with v3.3.0 and v3.3.1 (see MsTestRegression.zip for running example):

using FsCheck;

namespace MsTestRegression;

[TestClass]
public class TestClass
{
    [TestMethod]
    public void TestMethod1()
    {
        Prop.ForAll((IEnumerable<int> x) => true).QuickCheckThrowOnFailure();
    }

    [AssemblyInitialize]
    public static void TestInitialize(TestContext _)
    {
        Arb.Register<Arbitraries>();
    }
}

public class Arbitraries
{
    public static Arbitrary<IEnumerable<T>> EnumArb<T>() => new EnumerableArb<T>();
}

public sealed class EnumerableArb<T> : Arbitrary<IEnumerable<T>>
{
    public override Gen<IEnumerable<T>> Generator => Arb.Generate<List<T>>().Select(l => (IEnumerable<T>)l);
}

(I'm using FsCheck).

When I debug the test with v3.2.2, there are no exceptions. However, when I debug the test with v3.3.0 (or v3.3.1), there are two exceptions thrown.

First:
image

Second:
image
image

I think these exceptions cause the test to fail. It seems like something about loading assemblies in a method with the AssemblyInitialize attribute has changed with v3.3.0. The actual test method TestMethod1 then fails because Arb.Register<Arbitraries>() doesn't work as expected.

The test also fails with v3.3.0 when run from the CLI (but works with up to v3.2.2).

*) .net8
*) MSTest.TestFramework 3.3.1
*) Visual Studio 17.9.5

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Apr 4, 2024

I'm getting different exception in VS and also in CLI(the same exception)
image

can you cleanup the solution removing all obj/bin folders?

Which version of VS are you using?

@Andreas-Dorfer
Copy link
Author

I do get that exception too with v3.3.0. But that's a subsequent error, because the registration of the arbitrary in TestInitialize doesn't work. That's what the exception message is saying: It cannot find an arbitrary for IEnumerable<int>, although I'm registrating the arbitrary for IEnumerable<int> in TestInitialize. When you downgrade to v3.2.2, the arbitrary registration works again, and the test runs successfully.

To reproduce the other 2 exceptions, you have to enable all "Common Language Runtime Exceptions" and uncheck "Enable Just My Code".

@Andreas-Dorfer
Copy link
Author

I don't know if it helps you with evaluating the issue, but I'm maintaining a project that integrates FsCheck with MsTest and I use MsTest + FsCheck in production. Upgrading MsTest to v3.3.0 breaks a lot of my production tests that depend on arbitrary registration (both locally and in the CI pipeline).

@Evangelink
Copy link
Member

I will have a look next week but I recall having done some change to enable VSInstallationUtilities in different frameworks to add support for Assembly Resolution for netcore as requested by some company. It's possible this change has some side effect consequences.

@Evangelink
Copy link
Member

I do confirm that with the repro and MSTest 3.2.2, the test pass while with 3.3.0 or 3.3.1 the test fails.

I have done some changes in local to try to revert #2110 and despite the few debugging exceptions all gone, I still end up with the same FsCheck exception: System.Exception: The type System.Collections.Generic.IEnumerable1[[System.Int32, System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]] is not handled automatically by FsCheck. Consider using another type or writing and registering a generator for it.`

With or without the changes, we are correctly able to call the content of the TestMethod1 so it would seem that the issue is on FsCheck. It's possible that some change in what is loaded causes FsCheck to fail but I don't know the logic they use.

I'd recommend creating a bug on their side so they can provide some more insights.

@osexpert
Copy link

osexpert commented Apr 6, 2024

I got the same\similar problem in 3.3.0-3.3.1.
It was a [TestMethod] with multiple [DataRow]. If I ran each [DataRow] individually, they worked. If I ran the [TestMethod], the first [DataRow] worked but the second one failed (weird COMException). So it's kind of a mystery error (race condition?).
I downgraded to 2.2.10.

@Evangelink
Copy link
Member

Evangelink commented Apr 6, 2024

@osexpert are you also using FsCheck? Would it be possible for you to provide a repro?

@osexpert
Copy link

osexpert commented Apr 6, 2024

@osexpert are you also using FsCheck?

Not using FsCheck. But I tried to reproduce the problem again, I may not have remembered it clearly.....
When I tried to reproduce it again, it seems to work fine when tests succeed. But if a test fail and I try to debug it, then this exception is the first one I hit:

image
The exception will be catched thou, as the screenshot shows. But the comment is not right:
// SetupConfiguration won't work if VS is not installed or VS is pre-vs2017 version.
I have VS 2022.

So I think...this exception may not be harmful since catched, but it is confusing when debugging failing tests, so confusing\scary, that I downgraded instantly:-)

@Andreas-Dorfer
Copy link
Author

@Evangelink I guess I found the issue. FsCheck's arbitrary registration uses ThreadLocal:

image

Up to v3.2.2, that is no issue as long as I don't run my tests in parallel. The AssemblyInitialize code and all my tests run on the same thread. However, starting with v3.3.0, the AssemblyInitialize code runs on a different thread than my tests - even when I don't parallelize my test run.

[assembly: DoNotParallelize]

namespace TestProject;

[TestClass]
public class UnitTest
{
    [AssemblyInitialize]
    public static void InitAssembly(TestContext _)
    {
        Console.WriteLine($"'AssemblyInitialize' thread id: {Environment.CurrentManagedThreadId}");
    }

    [ClassInitialize]
    public static void InitClass(TestContext _)
    {
        Console.WriteLine($"'ClassInitialize' thread id: {Environment.CurrentManagedThreadId}");
    }

    [TestMethod]
    public void TestMethod()
    {
        Console.WriteLine($"'TestMethod' thread id: {Environment.CurrentManagedThreadId}");
    }
}

Produces an output like this:

'AssemblyInitialize' thread id: 10
'ClassInitialize' thread id: 10
'TestMethod' thread id: 5

@nohwnd
Copy link
Member

nohwnd commented Apr 8, 2024

So likely linked to this change where each callback is now invoked as task, which might end up running on different threads. https://github.com/microsoft/testfx/pull/2570/files

I don't see a way to implement cancellation and still carry out the rest of the tests on the same thread.

E.g. in this situation:

AssemblyInitialize - thread 1 - passes
	Class1Initialize - thread 1 - passes
		Test1 - Thread 1 - passes
	Class2Initialize - thread 1 - is hanging, and times out
		Test2 - not run because init timed out
	
	Class3Initialize - wants thread 1, but cannot resume on it 
	    because we cannot abort Class2Initialize work

@Evangelink
Copy link
Member

We need to use custom thread for that but this is IMO a new feature request.

I understand this break is problematic but I am not super keen on considering this a bug/regression on our side as we are changing internals and the behavior wasn't a documented/supported behavior.

@Andreas-Dorfer
Copy link
Author

Now that I know what's causing the issue, I can work around it. E.g., with a custom TestMethodAttribute:

using FsCheck;
using System.Collections.Concurrent;

namespace MsTestRegression;

public class FsCheckTestMethodAttribute : TestMethodAttribute
{
    static readonly ConcurrentDictionary<int, int> Threads = new();

    public override Microsoft.VisualStudio.TestTools.UnitTesting.TestResult[] Execute(ITestMethod testMethod)
    {
        var threadId = Environment.CurrentManagedThreadId;
        if (Threads.TryAdd(threadId, threadId))
        {
            Arb.Register<Arbitraries>();
        }

        return base.Execute(testMethod);
    }
}

[TestClass]
public class TestClass
{
    [FsCheckTestMethod]
    public void TestMethod1()
    {
        Prop.ForAll((IEnumerable<int> x) => true).QuickCheckThrowOnFailure();
    }
}

public class Arbitraries
{
    public static Arbitrary<IEnumerable<T>> EnumArb<T>() => new EnumerableArb<T>();
}

public sealed class EnumerableArb<T> : Arbitrary<IEnumerable<T>>
{
    public override Gen<IEnumerable<T>> Generator => Arb.Generate<List<T>>().Select(l => (IEnumerable<T>)l);
}

Btw, FsCheck v3 will be out soon (there's already an RC3). FsCheck v3 completely changes the way arbitraries are registered. It no longer uses ThreadLocal. Therefore, the issue will go away with the next FsCheck version anyway.

I'm ok with you closing the issue if you want to. Thanks to everyone for looking into it!

@nohwnd
Copy link
Member

nohwnd commented Apr 10, 2024

There is some good news. Good luck with your release!

@nohwnd nohwnd closed this as completed Apr 10, 2024
@nohwnd nohwnd closed this as not planned Won't fix, can't repro, duplicate, stale Apr 10, 2024
@charlesdwmorrison
Copy link

charlesdwmorrison commented Apr 10, 2024

I am seeing this issue in .Net 8.
Downgrading to MSTest3.2.2 fixed the problem for me as well.
Not sure why this is closed? What is the real fix? Stay on MSTest 3.2.2 forever?

Update 4/12/24: No I am not using FsCheck (I actually didn't understand that FsCheck was part of this issue). I am using a plain old MSTest to test web services and serialization of XML.
BUT I WILL say (now that I read the full issue), that it is interesting the issue was seen while using some file system libraries.
My work laptop, where I encounter this issue, has a ton of disk-based security, for instance its SSD drive is encrypted with Bitlocker.
AND the issue appeared for me right after I got forced-rebooted (so another version of the disk security could have been pushed out, slowing down my hard drive.)
If you are looking for a repro, maybe that's the secret: try it on some encrypted drives and also try using MSTest when there is a lot of activity on the disk.
I'm using
Microsoft Visual Studio Enterprise 2022 (64-bit) - Current
Version 17.9.6

@Evangelink
Copy link
Member

Hi @charlesdwmorrison!

Can you confirm you are also using FsCheck? If not, could you provide a repro or give us some details about your scenario?

FsCheck is relying on some internals of MSTest (not a documented or supported behavior) and we had to change this behavior to enable some new timeout features so we are stuck here.

@Andreas-Dorfer do you know when the new version of FsCheck will be released?

@osexpert
Copy link

osexpert commented Apr 11, 2024

I also think this case should not be closed. It throws there (VSInstallPath property) even if VS >= 2017 is installed. It did not before and it is in concflict with the comment in the code. So there is a regression there IMO. It "works", but this makes it annoying to debug tests, you always get into this ugly COMException first and it is not very trustworthy (bad impression).

@Evangelink
Copy link
Member

It throws there (VSInstallPath property) even if VS >= 2017 is installed. It did not before and it is in concflict with the comment in the code. So there is a regression there IMO.

It cannot be considered as regression as it's some internal exception being caught. If you debug runtime or most of the tools you will see such behavior happening. Yet I agree this is not the best experience and that's why I fixed it in #2654 that will be available as part of the 3.4 release.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Apr 11, 2024

I understand this break is problematic but I am not super keen on considering this a bug/regression on our side as we are changing internals and the behavior wasn't a documented/supported behavior.

I would not consider a breaking change too, MSTest doesn't give any guarantee for what I know(I don't see documentation around it) on the nature of the threads that are running the "tests method", we only guarantee that the execution order is the expected one.
Tools/Tests should not take "dependency" on the runtime non documented/guarantee context.

We need the freedom to change it for more than one reason, timeout is one...but there're others that could come in future.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants