SpecFlowOSS / SpecFlow

#1 .NET BDD Framework. SpecFlow automates your testing & works with your existing code. Find Bugs before they happen. Behavior Driven Development helps developers, testers, and business representatives to get a better understanding of their collaboration
https://www.specflow.org/
Other
2.25k stars 754 forks source link

[.NETCore] New UnitTestProvider & Plugin Configuration #1158

Closed SabotageAndi closed 6 years ago

SabotageAndi commented 6 years ago

We want that the normal user only needs to add the correct NuGet packages to his project to get if configured for the used unit test provider. Currently this works because of a app.config file transformation in the NuGet Packages (https://github.com/techtalk/SpecFlow/blob/master/Installer/NuGetPackages/SpecFlow.NUnit/App.config.transform).

With PackageReference (in old and new csproj format) this doesn't work anymore and so we need something different.

Proposal:

We already have NuGet packages for XUnit, NUnit and MsTest. These will be now used for more than changing the app.config. Also the the plugin configuration is removed from the config file and is also put into MSBuild and references.

Generator

MSBuild is used.

Plugins have to add an entry to the SpecFlowPlugins item group, which is passed to the new GeneratorTask (https://github.com/techtalk/SpecFlow/blob/DotNetCore/SpecFlow.Tools.MsBuild.Generation/GenerateFeatureFileCodeBehindTask.cs) The generator loads every plugin that's passed to it.

To have control over the order how plugins are loaded, we enhance the IGeneratorPlugin interface (https://github.com/techtalk/SpecFlow/blob/DotNetCore/TechTalk.SpecFlow.Generator/Plugins/IGeneratorPlugin.cs) with an int Order { get; } property

If the plugin is for a unit test framework, they have to add an entry to the SpecFlowUnitTestProvider item group. This is also passed to the task. If the item group contains more than one entry, we throw an error. With that, we can recognize if the user has wrongly added multiple nuget packages for different unit test provider.

Runtime

The plugin has to be referenced in the test project. We load every referenced plugin. The identification is like the generator plugins (ending in .SpecFlowPlugin or .SpecFlow).

To have control over the order how plugins are loaded, we enhance the IRuntimePlugin interface (https://github.com/techtalk/SpecFlow/blob/DotNetCore/TechTalk.SpecFlow/Plugins/IRuntimePlugin.cs) with an int Order { get; } property

To configure the unit test provider at runtime, we create a new class that contains this state and is registered in the global container. This has to be done in the RaiseConfigurationDefaults event.

SabotageAndi commented 6 years ago

@gasparnagy @ChristopherHaws @kant2002 What do you think about this suggestion?

ChristopherHaws commented 6 years ago

@SabotageAndi Looks good to me. I would really like to have the ability for the code behind files to live in a temp directory rather than side-by-side with the feature file. One option which I like is putting them into the IntermediateOutputDirectory (obj folder). This is what Microsoft does when they generate AssemblyInfo.cs in the SDK based project system. This would eliminate the need for users to add *.feature.cs to their .gitignore files. If they live in the IntermediateOutputDirectory, I believe that supporting Clean also becomes a lot easier as we can just add the generated files to the FileWrites item group (at least this is my understanding of how that works).

Would it be possible to make the GenerateFeatureFileCodeBehindTask inherit the ToolTask or ToolTaskExtension base class? This base class is designed to run out of process tools and my understanding is that they designed it so that there are ways to make the tool keep running between builds so that the tool does not need to start a new process every time the user builds. https://docs.microsoft.com/en-us/dotnet/api/microsoft.build.utilities.tooltask https://docs.microsoft.com/en-us/visualstudio/msbuild/tooltaskextension-base-class

For future versions (doesn't need to happen in v3 but I would like to keep it in the roadmap) is potentially supporting multiple unit test frameworks in a single project which I briefly talked about here https://github.com/techtalk/SpecFlow/issues/1033#issuecomment-384447567. It is more of an advanced use case so I would support having a project property to light up the feature instead of having it on by default.

Also, I am still not fully satisfied by the way Runtime plugins work. Is there a technical limitation as to why we cant do something similar to what Xunit does with attributes? Instead of having the plugin named a certain way and control its own order, could we have an attribute that is added to AssemblyInfo.cs? For example, in my unit test project, perhaps my AssemblyInfo.cs file contains:

[assembly: SpecFlowRuntimePlugin(typeof(MyRuntimePlugin), Order = 1)]

This would make it more declarative and would eliminate the need for plugins to follow a naming standard.

Another option would be to have a new hook attribute which could look like this:

public class Hooks
{
    [SpecFlowStartup]
    public static void Startup(ITestHost testHost)
    {
        testHost.AddRuntimePlugin<MyRuntimePlugin>();
    }
}

I really like this approach because there have been times where I have wanted to hook into specflow in areas that only plugins can hook in. This would allow me to do that without the need to create a plugin. For example:

public class Hooks
{
    [SpecFlowStartup]
    public static void Startup(ITestHost testHost)
    {
        testHost.Events.RegisterGlobalDependencies(container =>
        {
            // Register global dependencies here...
        });
    }
}
SabotageAndi commented 6 years ago

generate code behind to IntermediateOutputDirectory

Let's do that. No idea why we didn't thought about this before.

ToolTask & ToolTaskExtension

Looks like the stuff we would need, but it's not available on .NET Core. :-(

support for multiple test frameworks

You can achieve this already with generator plugins. We are doing something similar for the Specs (https://github.com/techtalk/SpecFlow/tree/DotNetCore/Tests/TechTalk.SpecFlow.Specs.Generator.SpecFlowPlugin) to test everything for the different test frameworks.

Runtime plugins

No idea why the config file was choosen. Perhaps @gasparnagy can remember. Probably so that you can change the used plugins without recompiling. While thinking about it, I came to an additional idea: Let's drop the naming filtering for plugins and look into every referenced assembly and the current assembly. We look into every assembly for the assembly:GeneratorPlugin and assembly:RuntimePlugin attributes and load them. So you can also write plugins in your own test assembly. So we wouldn't need to add a new hook.

Code-Grump commented 6 years ago

I think ToolTask and ToolTaskExtension are available in the .NET Core version of MSBuild. They're in the current source on GitHub. What makes you think otherwise?

If there is an importance to the ordering of plugins, it might be best to expose a collection of plugin "registries" which can be modified by whatever is setting up SpecFlow. In this way, you can examine the list of plugins and insert your own at the relevant place before any of the plugins are actually initialized.

public class SpecFlowStartup
{
   public void Configure(ITestHostBuilder testHost)
   {
      testHost.Plugins.Insert(2, new MyPluginDescription());
   }
}

I am shamelessly stealing from the Startup class concept from the OWIN/ASP.NET application startup routines. I love this convention-based pattern and having my own SpecFlowStartup class as a way to configure my test runner would be dreamy. No attributes; just explicit configuration in code.

SabotageAndi commented 6 years ago

ToolTask & ToolTaskExtension

The documentation says it's only available in full framework: image I didn't looked into the source.

Plugin ordering

In most of the cases the ordering of the plugins doesn't matter, as they override different parts. But there are some edge cases where it is important. So I think it should be something optional for the user to specify it. Perhaps it's not that often needed anyways and we can skip this for the 3.0 release and wait for feedback.

Code-Grump commented 6 years ago

Any appetite for a Startup-like configuration for SpecFlow as part of 3.0?

ChristopherHaws commented 6 years ago

@Tragedian I created a plugin for SpecFlow that gives me startup-like configuration which we use at my work. I will see if I can pull those bits out at some point and open source it. I too really love how ASP.NET Core startup configuration works, which is why I suggested it above.

@SabotageAndi ToolTask definitely works in .NET Core, I am not sure why their documentation doesn't mention it. That list of supported frameworks is only including the .NET Framework versions. I believe ToolTask has been supported by .NET Core since day one as part of this nuget package: Microsoft.Build.Utilities.Core..

Here is an example implementation for yarn written by Nate McMaster (one of the guys on the MSBuild team) which is compiled against the .NET Standard: https://github.com/natemcmaster/Yarn.MSBuild/blob/2813c1442403f69f66f525cf7e64e34319a3e678/src/Yarn.MSBuild/Yarn.cs

SabotageAndi commented 6 years ago

I extracted Startup configuration (#1161) and the MSBuild Task (#1162) into separate issues. Let's discuss these there and let this issue about the plugin & unit test configuration.

Code-Grump commented 6 years ago

For generating feature files as intermediary output, I actually wrote a NuGet package to do just that: https://github.com/Tragedian/Rhubarb.SpecFlow.NetCore. It just invokes the specflow.exe tool to generate the feature files and incorporates them into the compilation. It's just a workaround until .NET Core support is there, but my preference is to move code-generation to a build-time activity.

Code-Grump commented 6 years ago

I don't think an Order property can ever really solve the sequencing problem. If a plugin is written in isolation, it can only change its order value reactively when it finds some kind of conflict with another plugin. Changes to one value can cascade to cause a failure elsewhere. I think either ordering matters sufficiently that users need to be able to specify the order in which they want things to run (i.e. a list needs to be maintained somewhere), or the ordering does not matter by design.

I have only ever used plugins I've written myself, so I haven't ever experienced a "conflict" with another plugin. What's the worse-case scenario that ordering tries to solve?

gasparnagy commented 6 years ago

Here are my comments to this:

I generally agree on enabling plugins just by installing the nuget package, so that you don't have to list them in the configuration file as well. I think this would work for both generator and runtime plugins.

I am also fine to drop the naming convention and just scan through all referenced assemblies, but we need to check if this is possible (how can you get the list of referenced assemblies runtime?). Earlier we had problems with projects that have references that cannot be loaded in all context. E.g. imagine a native x64 reference that is only loaded by the project if the context if x64. Now we have to load all assemblies in order to scan for the specflow pluins, so we might cause troubles and kill the entire delay loaded assembly contept.

I am also fine with the obj directory thing. The Single-file generator infrastructure that we used was forcing us to place the generated file next to the feature file and git/gitignore were absolutely uncommon at that time.

I am also fine to move the entire generation to MsBuild-only (or even without generation).

Ordering. I agree with @Tragedian, that the Order attribute will cause a lot of confusion as they developed independently. I think we can either have no order by default or maybe have some categories that imply the order (e.g. "unit test provider" plugins come first). Another way I can imagine is that plugins can (optionally) specify their dependencies in terms of plugin names. E.g. a plugin that changes the behavior of the SpecFlow+ Runner plugin could specify that it "depends" on the SpecFlow+ Runner plugin (by NuGet package name or assembly name or ???) and then the plugin loader would make the necessary ordering so that they are loaded in the "right" order.

In any case, I would just provide an option to override the plugin loading order per project if necessary.

Startup-like configuration: I am also fine with that. It will make the tooling harder though. (e.g. so far the VS integration could get the default feature file culture by reading the config XML/json file, now it would need to run code (how?) to get that. But maybe this is the only config setting that is really needed by the IDE, all the others are enough to be processed runtime (BTW: what about the configuration of the generators/generator plugins?).

ChristopherHaws commented 6 years ago

@gasparnagy Regarding runtime plugins, IMHO they should be configured directly in code, whether that is via an assembly attribute or some form of startup class or hook (or a hybrid of several). I tend to prefer a startup hook since it aligns well with the rest of specflow; maybe the BeforeTestRunAttribute method can take an optional parameter of ITestRunHostBuilder or something? As for full blown startup class and middleware infrastructure, I've played around with the idea, and even have a working prototype, but while it was very flexible, usability suffered and I fear people would give up in trying to configure it because it is too complicated.

As for Generator plugins, they would all be configured directly in the csproj file so that the settings can be passed directly to the MSBuild task. The simplest solution I can think of right now (I will need to sleep on this to see if I can come up with something better) is to use ItemGroups. Something like:

<ItemGroup>

    <SpecFlowConfig Update="xUnit">
        <AllowDebugGeneratedFiles>false</AllowDebugGeneratedFiles>
        <AllowRowTests>true</AllowRowTests>
        <GenerateAsyncTests>true</GenerateAsyncTests>
    </SpecFlowConfig>

    <SpecFlowConfig Update="StepDefinitionReport">
        <OutputName>$(MSBuildProjectName)</OutputName>
        <OutputFormat>json</OutputFormat>
    </SpecFlowConfig>

</ItemGroup>

The generator task could take a parameter that accepts all of the SpecFlowConfig's as an ITaskItem[]. Then all the metadata could be read into a usable format (such as a Microsoft.Extensions.Configuration.IConfigurationProvider) and can be passed to the generator plugin. I would argue that any generator that requires configuration more complicated than simple key/value pairs should have it's own config file (similar to tsconfig.json in TypeScript land).

Another option would be a simple ini style property similar to this solution: https://stackoverflow.com/questions/46226120/how-to-pass-dynamic-properties-to-an-msbuild-task

There also seems to be a feature in MSBuild where you can have XML nested in a property, but I don't know how users would modify this. https://github.com/MicrosoftDocs/visualstudio-docs/blob/master/docs/msbuild/msbuild-properties.md#storing-xml-in-properties

SabotageAndi commented 6 years ago

In the first step, I wouldn't provide any option for ordering the pluings. There aren't that many out there and we don't know if the order is really a problem.

For configuring the generator plugins:

In the NuGet package of the plugin in the .props file we have a

<ItemGroup>
    <SpecFlowGeneratorPlugins Include="<Path to Generator Plugin dll>" />
</ItemGroup>

This itemgroup is then passed to our MSBuild task (https://github.com/techtalk/SpecFlow/blob/DotNetCore/SpecFlow.Tools.MsBuild.Generation/build/SpecFlow.Tools.MsBuild.Generation.targets#L22)

 <Target Name="GenerateFeatureCodeBehindFiles" Inputs="@(SpecFlowFeatureFiles)" Outputs="@(SpecFlowFeatureFiles->'$(SpecFlowCodeBehindOutputPath)\%(RelativeDir)%(Filename).feature.cs')" DependsOnTargets="UpdateFeatureFilesInProject">
    <GenerateFeatureFileCodeBehindTask
      ProjectPath="$(MSBuildProjectFullPath)"
      OutputPath="$(SpecFlowCodeBehindOutputPath)"
      FeatureFiles="@(SpecFlowFeatureFiles)"
      RootNamespace="$(RootNamespace)"
      GeneratorPlugins="@(SpecFlowGeneratorPlugins)" >
      <Output TaskParameter="GeneratedFiles" ItemName="SpecFlowGeneratedFiles" />
    </GenerateFeatureFileCodeBehindTask>
  </Target>

We pass this list down to GeneratorContainerBuilder.CreateContainer and load them all.

For runtime plugins:

@gasparnagy you are right, with normal Assembly.Load we will probably run into problems. But we could use the ReflectionOnly context and probe if the attribute exists and only then load it fully with Assembly.Load

This worked in LinqPad (needs probably adaptions for real usage):

public bool IsRuntimePlugin(string assembly)
{
    Assembly.ReflectionOnlyLoadFrom(@"TechTalk.SpecFlow.dll"); //I needed to load the SpecFlow assembly in the reflection context. Not sure if we need this or it's only necessary in LinqPad.
    var assembly = Assembly.ReflectionOnlyLoadFrom(assembly);
    return assembly.GetCustomAttributesData().Where(a => a.AttributeType.FullName == "TechTalk.SpecFlow.Plugins.RuntimePluginAttribute").Any();
}

Configuring the Unit test provider

I have following singleton in mind:

public class UnitTestProviderConfiguration
{
    public void UseUnitTestProvider(string unitTestProviderName) {} //throws an exception if UnitTestProvider is already set

    public string UnitTestProvider {get;}
}

The plugin interfaces are extended with an parameter of this class:

public interface IGeneratorPlugin
{
    void Initialize(GeneratorPluginEvents generatorPluginEvents, GeneratorPluginParameters generatorPluginParameters, UnitTestProviderConfiguration unitTestProviderConfiguration);
}
public interface IRuntimePlugin
{
    void Initialize(RuntimePluginEvents runtimePluginEvents, RuntimePluginParameters runtimePluginParameters, UnitTestProviderConfiguration unitTestProviderConfiguration);
}

If a plugin is for a unit test provider, they have to call the UseUnitTestProvider method in it's Initialize method. So we need to laod the plugins as early as possible when we are creating the container.

Configuring the rest

Here is a nearly complete example of all configuration values:

<specFlow>
    <language feature=""en"" tool=""en"" /> 
    <unitTestProvider name=""NUnit"" 
                      generatorProvider=""TechTalk.SpecFlow.TestFrameworkIntegration.NUnitRuntimeProvider, TechTalk.SpecFlow""
                      runtimeProvider=""TechTalk.SpecFlow.UnitTestProvider.NUnitRuntimeProvider, TechTalk.SpecFlow"" />
    <generator allowDebugGeneratedFiles=""false"" allowRowTests=""false"" markFeaturesParallelizable=""false"">
        <skipParallelizableMarkerForTags>
            <tag value="Sequential"/>
            <tag value="sequential"/>
        </skipParallelizableMarkerForTags>
    </generator>

    <runtime detectAmbiguousMatches=""true""
             stopAtFirstError=""false""
             missingOrPendingStepsOutcome=""Inconclusive"" />
    <trace traceSuccessfulSteps=""true""
           traceTimings=""false""
           minTracedDuration=""0:0:0.1""
           listener=""TechTalk.SpecFlow.Tracing.DefaultListener, TechTalk.SpecFlow""
            />
</specFlow>

language: needed by the runtime & VS Extension
unitTestProvider: replaced by above
unitTestProvider.generatorProvider: I would drop it, as it's not needed to add a additional unit test provider. Can be overwriten in a plugin.
unitTestProvider.runtimeProvider: I would drop it, as it's not needed to add a additional unit test provider. Can be overwriten in a plugin.
generator: needed by generator runtime: needed by the runtime
trace: needed by the runtime
StepAssemblies: needed by the runtime & VS Extension

I see the reason to move the generator configuration into MSBuild. But for now I won't move that to props file. Reason is, that I and my team are on a time budget for the .NET Core support and this change will not save us time. @ChristopherHaws if you want to contribute this, I am happy to merge this PR (probably best after https://github.com/techtalk/SpecFlow/issues/1032).

All configuration except unit test provider and plugins will be like it is now (app.config & specflow.json).

SabotageAndi commented 6 years ago

Tasks:

gasparnagy commented 6 years ago

@SabotageAndi I don't fully understand. What is the benefit of adding the UnitTestProviderConfiguration class and the reference to it to the plugin interfaces? You can anyway do it already right now, just need to register your unit test generator/runtime and set the UnitTestProvider setting in the configuration on the "configure" event. This is exactly what the SpecFlow+ Runner does right now. What is the benefit that we get with UnitTestProviderConfiguration in comparison to the current way?

SabotageAndi commented 6 years ago

The Runner doesn't set anything in the configuration. It only registers additional unittestproviders.

Here is it from the runtime plugin part:

private void RegisterGlobalDependencies(object sender, RegisterGlobalDependenciesEventArgs eventArgs)
{
    var container = eventArgs.ObjectContainer;

    container.RegisterTypeAs<SpecRunRuntimeProvider, IUnitTestRuntimeProvider>("SpecRun");

    foreach (var supportedAdditionalUnitTestProvider in SupportedAdditionalUnitTestProviders.List)
    {
        container.RegisterTypeAs<SpecRunRuntimeProvider, IUnitTestRuntimeProvider>("SpecRun+" + supportedAdditionalUnitTestProvider);
    }
}

I don't want it in the current SpecFlowConfiguration class, because this information comes all from app.config/specflow.json. I would like that this is separated and explicit and not setting some string property 3 properties deep in the object graph.

gasparnagy commented 6 years ago

@SabotageAndi Uhm. I still don't understand. Does this mean that the users will never be able to change the unit test provider by config but only by replacing a NuGet package (containing the plugin of the unit test provider) with another? I don't have a problem with this, but just that we talk about the same thing.

I also have a plugin (SpecSync), that adds an additonal MsTest generation to the already configured one (because TFS test cases need MsTest). (For me the order was important already.) How can such a thing be implemented in the new unit test configuration world?

Code-Grump commented 6 years ago

What should happen if you install two test runner packages?

ChristopherHaws commented 6 years ago

@Tragedian Per the initial post, if multiple test adapters are configured for one test project, it will throw an exception (which will probably end up showing up as a build error).

If the item group contains more than one entry, we throw an error. With that, we can recognize if the user has wrongly added multiple nuget packages for different unit test provider.

In my initial proposal (https://github.com/techtalk/SpecFlow/issues/1033#issuecomment-384447567), I also proposed a possible solution for how it might be possible to handle multiple test adapters, however I believe that since this is not a common scenario this feature will not make it into v3, but might happen in the future.

gasparnagy commented 6 years ago

I had a phone call with @SabotageAndi about the unit test provider interface configuration. Here is the summary:

This problem is independent of:

Possible solutions:

With the last 2 options, we lose the ability to reconfigure runtime behavior without recompiling, but this is probably not such a big issue. (I don't remember ever using that.)

SabotageAndi commented 6 years ago

@gasparnagy @ChristopherHaws Looking into the referenced assemblies of the test project doesn't work. Looks like there is only data about the reference, when you use something from it. So the assemblies are referenced, but you don't find it out at runtime. 😔

Quickes workaround at the moment is to look into the filesystem. See https://github.com/techtalk/SpecFlow/blob/DotNetCore_UnitTestProviderConfiguration/TechTalk.SpecFlow/Plugins/RuntimePluginLocator.cs#L18 For that, I left the name filtering for *.SpecFlowPlugin. When we have the path, checking for the attribute with the ReflectionOnly context works without problems up to now.

gasparnagy commented 6 years ago

@SabotageAndi :( do we need the ReflectionOnly load then? if the name matches, we can just load the assembly like normal and find the plugin...

gasparnagy commented 6 years ago

@SabotageAndi @ChristopherHaws Just a correction for myself. In mid-term it would be also useful to be able to change the runtime configuration without recompiling the project. Think about changing reporting or output formatting details or the dry-run functionality (existing in Cucumber for long time) that "runs" the tests without actually running any automation code -- only checks the bindings.

lock[bot] commented 5 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.