microsoft / testfx

MSTest framework and adapter
MIT License
780 stars 259 forks source link

AssemblyInitialize/AssemblyCleanup in base class ignored in case of usage of it as base in tests from another assembly #757

Open Mariachi1231 opened 3 years ago

Mariachi1231 commented 3 years ago

Description

Hi guys, I have been facing the behaviour of AssemblyInitialize/AssemblyCleanup logic where they are not executed if you have a TestClass, which is a child of some base class defined in another assembly. From some point of view, this seems partially reasonable. I slightly went through your code and found, that scanning of test classes is based on AssemblyEnumerator and deciding whether a test assembly has some AssemblyInitialize or not is based on that fact. And for sure, in case of another assembly, it decides that, well, the assembly has no appropriate signatures since test classes are defined within such assembly have no direct implementation of AssemblyInitialize/AssemblyCleanup and ignore the fact that such method can be defined in a base class of test classes.

From another point of view, this seems slightly bad, since I am almost sure that most developers/teams/companies have their own infrastructure for tests where they are storing some useful cross-cutting logic, and since such infrastructure is generic, they, for sure, implement the logic for AssemblyInitialize and other lifecycle-based methods. And, for sure, a good idea for such guys is to dedicate their test infrastructure in a separate assembly, in order to reuse such logic across different projects/test-assemblies. But because of the limitations that I described above, unfortunately, it is not possible to do (without dirty workarounds).

Steps to reproduce

Create a library and add the next class that will be base for other tests:


[TestClass]
public class TestBase 
{
public static StringBuilder result;
public static readonly string resultPath = Path.Combine(Environment.CurrentDirectory, "result.txt");
    [AssemblyInitialize]
    public static void AssemblyIntialize(TestContext testContext)
    {
        File.Delete(resultPath);

        result = new StringBuilder();
        result.AppendLine(nameof(AssemblyIntialize));
    }

    [AssemblyCleanup]
    public static void AssemblyCleanup()
    {
        result.AppendLine(nameof(AssemblyCleanup));

        File.AppendAllText(resultPath, result.ToString());
    }

    [TestMethod]
    public void Test()
    {
        result.AppendLine(nameof(Test));
    }
}
> Create a unit-test project and add the next test class

[TestClass] public class UnitTest1 : TestBase { [TestMethod] public void TestMethod1() { result?.AppendLine(nameof(TestMethod1)); } }

> Run TestMethod1.

## Expected behavior

AssemblyIntialize Test AssemblyCleanup



## Actual behaviour
Nothing, since the file will not be created, since `AssemblyIntialize` and `AssemblyCleanup` will not be executed.

## Environment
Described logic is independent of env. 

## What I propose.
I understand that adding such behaviour is a breaking change. So, firstly I want to be able to make this optional when by default we preserve behaviour which is the current one. In order to propagate option I propose to add a constructor to `AssemblyInitialize/AssemblyCleanup` attributes with optional arg like `AssemblyInitialize(AssemblyScope scope = AssemblyScope.OnlyCurrentAssembly)`, where `AssemblyScope` is an enum with a possible value like `AssemblyScope.AnyAssembly`. Then, in `TypeCache.GetAssemblyInfo` slightly change the logic in a next way: analyze declared types as before and if `AssemblyInitialize/AssemblyCleanup` was found, then ok, just do everything as before (because, for sure, `AssemblyInitialize` defined in a current assembly has more priority than inherited one). But besides analyzing declared types (if the scope was `AssemblyScope.AnyAssembly`) we need to analyze the first level of inheriting graph of every test and if the base class exists add it to `HashSet`. As soon as `HashSet` will be filled and if `AssemblyInitialize` is still not found, do the previous step recursively until the first occurrence of `AssemblyInitialize` or empty `HashSet`.

## Contribution
If this seems reasonable to your team, I can make such changes by myself and cover them with tests and make a pull request.

## Additional resources
- The same problem described in [StackOverflow](https://stackoverflow.com/questions/37390356/assemblycleanup-not-being-run)

Thanks for your attention and your work on MsTest v2
andrewphamvk commented 3 years ago

This sounds reasonable. We already have a similar option with EnableBaseClassTestMethodsFromOtherAssemblies.

y87feng commented 2 years ago

We are running into this problem as well, hopefully it can be fixed soon

AlexandrSHad commented 2 years ago

Plus. The same for me.

Evangelink commented 1 year ago

It's a bit more complex for AssemblyInitialize/AssemblyCleanup because we cannot guarantee the location of these methods so we would need to iterate through all types and methods of other assemblies to find them which would highly increase discovery time. Also, you can then start to have base that also inherits from a base from another assembly, meaning we would also need to explore this assembly (etc etc).

Given current implementation, I am not in favour of implementing this feature.

I will keep the ticket open to collect feedback.

ChristopherHaws commented 1 year ago

What if you had to specify the class at an assembly level to ensure that the discovery process can stay fast? I could really use this feature ^_^

Something like:

[assembly: AssemblyTestClass(typeof(TestBase))]
gao-artur commented 1 year ago

I love @ChristopherHaws's proposal! We are currently defining the shared class with AssemblyInitialize/Cleanup attributes in VS Shared Project.

Evangelink commented 1 year ago

What if you had to specify the class at an assembly level to ensure that the discovery process can stay fast? I could really use this feature ^_^

Something like:

[assembly: AssemblyTestClass(typeof(TestBase))]

That's definitely a possibility.

In the meantime, you could "easily" create a method in your project marked with assembly initialize attribute that would simply call the assembly initialize from the other assembly you reference. This is obviously only to provide a temporary workaround for you.

ChristopherHaws commented 1 year ago

your project marked with assembly initialize attribute that would simply call the assembly initialize from the other assembly you reference. This is obviously only to provide a temporary workaround for you.

This is what we are doing currently :)

Youssef1313 commented 2 weeks ago

@Evangelink I think we could use a Roslyn source generator combined with a ModuleInitializer to support this seamlessly and without performance penalty?

Detailed proposal

A new API is added:

public static class AssemblyFixturesHelper
{
    public void RegisterAssemblyInitialize(Action<TestContext> assemblyInitializeAction);
    public void RegisterAssemblyCleanup(Action assemblyCleanupAction);
}

A source generator will look for [AssemblyInitialize] and [AssemblyCleanup] and generates the "registration" code in a ModuleInitializer.

For example, if you have in a library code like:

public class MyClass
{
     [AssemblyInitialize]
     public static void AssemblyIntialize(TestContext testContext)
     {
     }

     [AssemblyCleanup]
     public static void AssemblyCleanup()
     {
     }
}

The generated code will be something like:

namespace AssemblyName;

internal static class AssemblyFixtureModuleInitializer
{
    [global::System.Runtime.CompilerServices.ModuleInitializerAttribute]
    internal static void Initialize()
    {
            AssemblyFixturesHelper.RegisterAssemblyInitialize(MyClass.AssemblyInitialize);
            AssemblyFixturesHelper.RegisterAssemblyCleanup(MyClass.AssemblyCleanup);
    }
}
Evangelink commented 2 weeks ago

Good suggestion @Youssef1313. I have the idea to allow registration of lambdas/methods in addition to the reflection based discovery and that would fit nicely with it being called by the source gen. This way we avoid ModuleInit that would most likely not work with VSTest.