NUnit #158

Merged
merged 97 commits into from Sep 24, 2013

Projects

None yet

5 participants

@gertjvr
Contributor
gertjvr commented Aug 25, 2013

Ported AutoFixture.xUnit.net on a one to one basis and just created an NUnitAddin called AutoFixtureNUnitExtensions as suggested on NUnit.org.

I have ran all test in the scenario and they pass.

As explained on NUnit.org, all you have to do is copy the addin dll in bin\addins folder where you are running nunit.exe or nunit-console.exe from.

Example:

[TestFixture]
public class Scenario
{
    [Test, AutoData]
    public void AutoDataProvidesCorrectInteger(int primitiveValue)
    {
        Assert.AreNotEqual(0, primitiveValue);
    }
}
gertjvr added some commits Aug 20, 2013
@gertjvr gertjvr AutoFixture.NUnit Initial Commit 76ad7dc
@gertjvr gertjvr Fixed Version Numbers 1deb235
@gertjvr gertjvr Added NuGet Spec For AutoFixture.NUnit f2398fb
@gertjvr gertjvr Added AutoFixture.NUnit into build scripts. 853df32
@gertjvr gertjvr Added NUnit.Runners 2.6.2 ea21b0e
@gertjvr gertjvr Reverted Initial Commit 4726c35
@gertjvr gertjvr Added GlobalAssemblyInfo and added it too all projects 1ca3761
@gertjvr gertjvr Ported AutoFixture.xUnit.org to AutoFixture.NUnit.org def4f23
@gertjvr gertjvr Started NUnitAddin c781ccb
@gertjvr gertjvr Copied NUnitTestCaseBuilder current implementation 491423b
@gertjvr gertjvr Copied NUnitTestCaseProvider current implementation 3a0a529
@gertjvr gertjvr Missed Constants 34ff5fd
@gertjvr gertjvr Renamed AutoData to AutoDataAddin ae80f43
@gertjvr gertjvr Added AutoFixture.NUnit.org.Addins.UnitTest f026467
@gertjvr gertjvr Added AutoTestCaseData copy of ParameterSet 0e32ca1
@gertjvr gertjvr Missed checkin when renaming AutoDataAddin c1e2a24
@gertjvr gertjvr Missed NUnit reference a699eb3
@gertjvr gertjvr Fixed wrong assumtion that AreEqual is the same as AreSame 27fe06b
@gertjvr gertjvr Removed unnecessary file. 78769e2
@gertjvr gertjvr Downgraded all project to .net 4 frameworks for be inline with nunit …
…libraries
9f8bb0d
@gertjvr gertjvr Added AutoTestCaseDataBuilderTest e8035ef
@gertjvr gertjvr Added dependency on the AutoDataAddin 1d8a53c
@gertjvr gertjvr Registered the AutoDataTestCaseBuilder with Addin 14f6d7c
@gertjvr gertjvr Fixed NUnitAddin a835759
@gertjvr gertjvr Moved ClassData Attribute to an extentions folder inline with xUnit.E…
…xtensions.
a7110bc
@gertjvr gertjvr Renamed AutoDataAttribute to AutoTestCaseAttribute a497a06
@gertjvr gertjvr Removed the Addin project and intergrated it into AutoFixture.NUnit.org 81f8024
@gertjvr gertjvr Removed CaseDataAttribute reliased it can do the same using TestCaseA…
…ttribute
80d0ea6
@gertjvr gertjvr Removed e867524
@gertjvr gertjvr Missed checkins 1e7ad61
@gertjvr gertjvr Removed 6fbfca1
@gertjvr gertjvr Removed AutoTestCaseBuilder after a conversation with Charlie Poole h… 24880b1
@gertjvr gertjvr Added TestCaseProvider after a conversation with Charlie Poole https:… f9d5ef0
@gertjvr gertjvr Fixed Assert.ReferenceEquals to Assert.AreSame 4dfd373
@gertjvr gertjvr Removed old solution 7fc5c5f
@gertjvr gertjvr Removed resharper copy localtions dc3b540
@moodmosaic moodmosaic and 1 other commented on an outdated diff Aug 25, 2013
Packages/repositories.config
@@ -4,6 +4,8 @@
<repository path="..\Src\AutoFakeItEasyUnitTest\packages.config" />
<repository path="..\Src\AutoFixture.xUnit.net.UnitTest\packages.config" />
<repository path="..\Src\AutoFixture.xUnit.net\packages.config" />
+ <repository path="..\Src\AutoFixture.NUnit.org.UnitTest\packages.config" />
@moodmosaic
moodmosaic Aug 25, 2013 Member

Is NUnit.org the official name of NUnit?

@gertjvr
gertjvr Aug 25, 2013 Contributor

NUnit, but tried to keep to how xUnit projects was named, but easy to change if required.

@gertjvr
gertjvr Aug 25, 2013 Contributor

Ok renamed the NUnit.org to just NUnit

@moodmosaic moodmosaic and 1 other commented on an outdated diff Aug 25, 2013
...est/AutoFixture.NUnit.UnitTest.csproj.VisualState.xml
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="utf-8"?>
@moodmosaic
moodmosaic Aug 25, 2013 Member

Is .csproj.VisualState.xml a mandatory project/solution file?

@gertjvr
gertjvr Aug 25, 2013 Contributor

this file should not have been committed

@moodmosaic moodmosaic and 1 other commented on an outdated diff Aug 25, 2013
...xture.NUnit.org.UnitTest/AutoTestCaseAttributeTest.cs
@@ -0,0 +1,156 @@
+using System;
+using System.Linq;
+using NUnit.Framework;
+using Ploeh.AutoFixture;
+using Ploeh.AutoFixture.NUnit.org;
+using Ploeh.TestTypeFoundation;
+
+namespace Ploe.AutoFixture.NUnit.org.UnitTest
+{
+ [TestFixture]
@moodmosaic
moodmosaic Aug 25, 2013 Member

Throughout the AutoFixture codebase, unit tests are written using the xUnit.net unit testing framework. It would be great if we can be consistent otherwise it may become quite hard to maintain two different versions of assertion methods and test discovery mechanisms.

@gertjvr
gertjvr Aug 25, 2013 Contributor

I understand, not sure how you would test this extensions with xUnit, it designed to use NUnit?

@moodmosaic
moodmosaic Aug 25, 2013 Member

It would be nice to have some tests, written using NUnit, to verify that the new extension is usable from NUnit tests.

Is it required though to have all unit tests written using NUnit?

@gertjvr
gertjvr Aug 25, 2013 Contributor

I have converted all the test to use xUnit except Scenario.cs which proves AutoTestCase works.

@moodmosaic
Member

@gertjvr Thank you for your contribution! I left some comments (up to the point that I could review at the moment).

@gertjvr
Contributor
gertjvr commented Aug 28, 2013

Think I am at a stage where its working for me, so feel free to review it, and give any comments. Only tests not covered in the build process is the AutoFixture.NUnit.UnitTest.Scenarios because they have to NUnit tests.

To make the NUnitAddin work with Resharper copy dll below to your Resharper Bin\addins folder.
Ploeh.AutoFixture.dll
Ploeh.AutoFixture.NUnit.dll
Ploeh.AutoFixture.NUnit.Addin.dll

@moodmosaic moodmosaic and 1 other commented on an outdated diff Aug 30, 2013
...AutoFixture.NUnit.UnitTest/Properties/AssemblyInfo.cs
+using System;
+using System.Reflection;
+using System.Resources;
+using System.Runtime.InteropServices;
+using NUnit.Framework;
+
+[assembly: AssemblyTitle("AutoFixture.NUnit.UnitTest")]
+[assembly: AssemblyDescription("AutoFixture.NUnit.UnitTest")]
+[assembly: AssemblyProduct("AutoFixture.NUnit.UnitTest")]
+
+[assembly: Guid("97cf94f0-6d4c-4e30-8c72-f90a1a6edb8c")]
+
+[assembly: CLSCompliant(true)]
+[assembly: NeutralResourcesLanguage("en")]
+
+[assembly: RequiredAddin("AutoTestCaseExtension")]
@moodmosaic
moodmosaic Aug 30, 2013 Member

What is the purpose of this attribute? Are there any side effects if the plug-in is not available (or wasn't loaded correctly)?

@gertjvr
gertjvr Aug 30, 2013 Contributor

The attribute allows NUnit to look at all the NUnitAddins in the bin\addins folder and load only what is needed, if the NUnitAddin isn't availible it throws an exception RequiredAddinException, principal of fail fast, otherwise it will try and execute a normal Test and then the exception will be incorrect paramter count? which is really vague to what the real problem is.

@moodmosaic moodmosaic and 1 other commented on an outdated diff Aug 30, 2013
BuildRelease.msbuild
@@ -28,6 +28,10 @@
<BuildOutput Include="Src\AutoNSubstitute\bin\Release\Ploeh.AutoFixture.AutoNSubstitute.XML" />
<BuildOutput Include="Src\AutoFixture.xUnit.net\bin\Release\Ploeh.AutoFixture.Xunit.dll" />
<BuildOutput Include="Src\AutoFixture.xUnit.net\bin\Release\Ploeh.AutoFixture.Xunit.XML" />
+ <BuildOutput Include="Src\AutoFixture.NUnit\bin\Release\Ploeh.AutoFixture.NUnit.dll" />
+ <BuildOutput Include="Src\AutoFixture.NUnit\bin\Release\Ploeh.AutoFixture.NUnit.XML" />
+ <BuildOutput Include="Src\AutoFixture.NUnit.Addin\bin\Release\Ploeh.AutoFixture.NUnit.Addin.dll" />
@moodmosaic
moodmosaic Aug 30, 2013 Member

What is the role of the AutoFixture.NUnit.Addin? FWIW, I removed the AutoFixture assembly reference and it compiled successfully..

Also, I noticed that it references the AutoFixture.NUnit assembly only in one place (here).

Is it really necessary to introduce, support, and maintain the AutoFixture.NUnit.Addin project in AutoFixture? Wouldn't it be better if this project could be (somehow) part of the NUnit project itself?

@gertjvr
gertjvr Aug 30, 2013 Contributor
  1. Tried to build AutoFixture.NUnit.Addin locally without the AutoFixture "project" reference and it fails?
  2. It the only place its needed, I could combined the AutoFixture.NUnit and AutoFixture.NUnit.Addin project into one, the reason why they are separated was to allow the Addin not to be tided to a specific version of NUnit, but because I am using NUnit.Core that argument goes out of the window.
  3. NUnit is a closed source project and they are busy rebuild for NUnit 3.0 which will probably not need addins. so chances are they will not accept this code.
@moodmosaic
Member

Unfortunately, I can't get the plug-in to work correctly..

I copied the generated assemblies in various locations but none of them worked:

  • Packages\NUnit.Runners.2.6.2\tools\bin\addins
  • Packages\NUnit.Runners.2.6.2\tools\addins
  • AutoFixture.NUnit.UnitTest\bin\Debug\bin\addins
@gertjvr
Contributor
gertjvr commented Aug 30, 2013

This is what I found too, hate the concept of addins but needs to be where your running nunit from.

Where is your nunit.exe / nunit-console.exe running from? folder needs to be near your nunit runner executable.

if you have nunit 2.6.2 installed then: C:\Program Files (x86)\NUnit 2.6.2\bin\addins
With Resharper v8.0: C:\Program Files (x86)\JetBrains\ReSharper\v8.0\Bin\addins

@moodmosaic
Member

Wouldn't it be a lot easier if the AutoFixture.NUnit.Addin could be part of the NUnit project?

OTOH the primary library (AutoFixture.NUnit) doesn't even reference the NUnit itself.. It's actually an exact copy of AutoFixture.xUnit.net plus the DataAttribute class from the original xUnit.net source.

All the above can be treated as warning signs regarding maintainability, usability, and probably licensing, issues..

@gertjvr
Contributor
gertjvr commented Aug 30, 2013
  1. Agreed will be easier, will integrated The Addins project into NUnit project.
  2. There was a previous request to move the xUnit attributes to a common project #118 @ploeh choice was to just copy the attributes.

Not sure on the licensing issues, we could rename the DataAttribute class to something else to differentiate between the two implementations?

@moodmosaic
Member

You may copy some attributes which are trivial to implement. Copying an entire project is a different concept though.

As an example, maintaining two copies of the [CompositeDataAttribute] class wouldn't be easy, I think..

@gertjvr
Contributor
gertjvr commented Aug 30, 2013

how would you propose we go ahead then? I sent you my skype nickname if you want to discuss this offline?

@moodmosaic
Member

It would be better if the discussion could take place only here, on GitHub, so everyone can participate.

The problem with Skype (and any other instant messaging client) is that the history of the conversation gets lost after some point and there is no permanent URL to track the messages..

@gertjvr
Contributor
gertjvr commented Aug 30, 2013

I have combine the projects as suggested, also setup AutoFixture.NUnit.UnitTest to debug with nunit-gui.

To make this possible you have to install nunit-2.6.2.msi once installed.

When you build the AutoFixture.NUnit project the correct binaries are copied to C:\Program Files (x86)\NUnit 2.6.2\bin\addins

In Visual Studio select the AutoFixture.NUnit.UnitTest project -> right click -> debug -> start a new instance, this will launch the nunit-gui and you should be able to run the scenario unit tests which proves the addin works.

@gertjvr gertjvr closed this Aug 30, 2013
@gertjvr gertjvr reopened this Aug 30, 2013
@moodmosaic moodmosaic and 2 others commented on an outdated diff Aug 30, 2013
Nuget/Ploeh.AutoFixture.NUnit.nuspec
@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="utf-8"?>
+<package>
+ <metadata>
+ <id>AutoFixture.NUnit</id>
+ <version>1.0-alpha</version>
+ <title>AutoFixture with NUnit</title>
+ <authors>Gert Jansen van Rensburg</authors>
+ <owners>Gert Jansen van Rensburg</owners>
+ <dependencies>
+ <dependency id="autofixture" version="2.2" />
@moodmosaic
moodmosaic Aug 30, 2013 Member

Did you run the tests with AutoFixture 2.x?

@gertjvr
gertjvr Aug 30, 2013 Contributor

No did not, will run and see what version is the minimum the addin will work with.

@ploeh
ploeh Sep 3, 2013 Member

It's OK - just leave the version at 2.2 - that's what all the other nuspec files do. If done correctly, the build script should replace this number with the correct number.

@gertjvr
gertjvr Sep 4, 2013 Contributor

Changed it to 2.2

@moodmosaic moodmosaic and 1 other commented on an outdated diff Aug 30, 2013
...AutoFixture.NUnit/Addins/AutoFixtureNUnitFramework.cs
@@ -0,0 +1,7 @@
+namespace Ploeh.AutoFixture.NUnit.Addins
+{
+ public static class AutoFixtureNUnitFramework
@moodmosaic
moodmosaic Aug 30, 2013 Member

Is it necessary to declare this class as public static?

@gertjvr
gertjvr Aug 30, 2013 Contributor

Will remove this class, was necessary because the solution was split.

@moodmosaic moodmosaic and 1 other commented on an outdated diff Aug 30, 2013
...Fixture.NUnit/Addins/Builders/AutoTestCaseProvider.cs
@@ -0,0 +1,82 @@
+using System;
+using System.Collections;
+using System.Linq;
+using System.Reflection;
+using NUnit.Core;
+using NUnit.Core.Extensibility;
+
+namespace Ploeh.AutoFixture.NUnit.Addins.Builders
+{
+ /// <summary>
+ /// TestCaseSourceProvider provides data for methods
+ /// annotated with the TestCaseSourceAttribute.
+ /// </summary>
+ public class AutoTestCaseProvider : ITestCaseProvider2
@moodmosaic
moodmosaic Aug 30, 2013 Member

Is it necessary to declare this class as public?

@gertjvr
gertjvr Aug 31, 2013 Contributor

ITestCaseProvider2 requires it to be public

@moodmosaic moodmosaic commented on an outdated diff Aug 30, 2013
...utoFixture.NUnit/Addins/Decorators/CustomDecorator.cs
@@ -0,0 +1,21 @@
+using System.Linq;
+using System.Reflection;
+using NUnit.Core;
+using NUnit.Core.Extensibility;
+
+namespace Ploeh.AutoFixture.NUnit.Addins.Decorators
+{
+ public class CustomDecorator : ITestDecorator
@moodmosaic
moodmosaic Aug 30, 2013 Member

Is it necessary to declare this class as public?

@moodmosaic moodmosaic and 1 other commented on an outdated diff Aug 30, 2013
...Fixture.NUnit/Addins/Listeners/CustomEventListener.cs
@@ -0,0 +1,69 @@
+using System;
+using NUnit.Core;
+
+namespace Ploeh.AutoFixture.NUnit.Addins.Listeners
+{
+ public class CustomEventListener : EventListener
@moodmosaic
moodmosaic Aug 30, 2013 Member

Is it necessary to declare this class as public? Also this class does not follow the Open Source Contribution Etiquette for this project..

@gertjvr
gertjvr Aug 30, 2013 Contributor

Nope can be an internal class

@moodmosaic moodmosaic and 1 other commented on an outdated diff Aug 30, 2013
Src/AutoFixture.NUnit/Addins/TestMethodWrapper.cs
@@ -0,0 +1,21 @@
+using NUnit.Core;
+using Ploeh.AutoFixture.NUnit.Addins.Listeners;
+
+namespace Ploeh.AutoFixture.NUnit.Addins
+{
+ public class TestMethodWrapper : NUnitTestMethod
@moodmosaic
moodmosaic Aug 30, 2013 Member

Is it necessary to declare this class as public? Also this class does not follow the Open Source Contribution Etiquette for this project..

@gertjvr
gertjvr Aug 30, 2013 Contributor

Nope can be an internal class

@moodmosaic
Member

While this looks like an awesome way to extend NUnit, the are a few problems with the pull request itself:

  • It still contains an exact copy of the AutoFixture.xUnit.net source.
  • It still contains a copy of the [DataAttribute] class from the xUnit.net source.
  • BuildRelease.ps1 still fails to run the tests.
  • It introduces too many public types which are unrelated to AutoFixture itself.

Perhaps @ploeh, or @ecampidoglio, could express a different opinion but my personal opinion is that this pull request doesn't move the (hypothetical) AutoFixture plug-in for NUnit in the right direction..

@gertjvr
Contributor
gertjvr commented Aug 30, 2013

I must say I am disappointed with your opinion, this was and still is one of the most wanted extensions if I look at the codeplex issues for a few years.

    • Yes its an copy of AutoFixture.xUnit.net source, but as stated in my opening statement this is a port.
    • Is a copy because of a previous comment @ploeh opinion was copy the attributes rather than abstract it out into a common library. #118
  1. It does contain the concept of the DataAttribute but that is required to keep the code base aligned. There are alot of code sharing between xUnit.net and NUnit already Assert.True this doesn't justify the copy but rather that its not an exact match to the xUnit.net source.
  2. Last commit fixes BuildRelease.ps1 and runs the nunit tests.
  3. I have removed and made those classes mention internal,

I agree usability is probably not ideal but thats not by my design NUnit team is addressing this in NUnit 3.0

So yes I too would like to hear from @ploeh, @ecampidoglio or anyone else's opinion?

@ploeh ploeh referenced this pull request Sep 1, 2013
Closed

AutoFixture.NUnit #156

@ploeh
Member
ploeh commented Sep 1, 2013

Thank you for your contribution.

I'm horribly hung up at the moment, but will review this PR as soon as possible. I do realize that it's already been sitting here for a week, but realistically, I'm not going to have time to do a review until some time next week. Please accept my apologies for the delay.

@gertjvr
Contributor
gertjvr commented Sep 2, 2013

No problem I might not be available / around next week (expecting a baby this week).

@gertjvr
Contributor
gertjvr commented Sep 12, 2013

I am tried, arguing what if, could have's and should's...

@moodmosaic have previously commented:

this pull request doesn't move the (hypothetical) AutoFixture plug-in for NUnit in the right direction..

But from all his previous comments yet to understand what the right direction is, the only option you have provided so far is submit the PR to NUnit, But looking at the NUnit source on bazaar there is no other addin included in the NUnit source. Looking at other solutions, they favor including their addins with their solutions, not unlike what was done with AutoFixture.xUnit extensions.

@ecampidoglio wrote:

In the second, a misconfigured NUnit environment (i.e. missing or improperly installed add-in) would cause a subset of the tests to fail in some (potentially obscure) ways. For example, all tests are green on the local machine but fail on the CI server.

I agree that having the tests just fail randomly it a huge concern, luckily NUnit developers had the same concern, and included a way to ensure that if I test class library depends on an NUNIt addin, that you can add [assembly: NUnit.Framework.RequiredAddin("AutoTestCaseExtension")] in the project's properties/assemblyinfo.cs which will make the test runner fail in a predictable way, and it fails fast. I did mention this before when @moodmosaic ask what the RequiredAddin was in outdated diff and here the nuget install.ps1 was an idea, and I still think its valid, if the default locations exists, combined with altering the AssemblyInfo.cs which will make all the tests fails if the addin isn't found.

@gertjvr
Contributor
gertjvr commented Sep 12, 2013

A resounding issue I get from all the threads are maintainability and after writing this NUnit Addin I do understand, but why, but I found enough documentation and support from the NUnit community how to get things working.

JetBrains Resharper have native support for NUnit Addins, I struggled to getting the AutoFixture.NUnit Addin to work with Resharper but again with some research it was solved with guidance from @hhariri and @kkozmic who pointed me too this link.

Companies like JetBrains have even invested into this sector by writing there own Teamcity NUnit.Addin

I feel the AutoFixture.NUnit Addin is feature complete if compared with AutoFixture.xUnit and like you have mentioned would be wrong not to share it with the wider AutoFixture and NUnit community. Just wish that meant it was included with AutoFixture, and not in some library hidden away.

@ploeh I have tried to address all of your concerns, and will keep doing so.

@moodmosaic
Member

@gertjvr AutoFixture.Xunit uses the xUnit.net existing extensibility model, it doesn't (re)define one into AutoFixture.

@ploeh
Member
ploeh commented Sep 13, 2013

Hi all

Thanks for all the good comments on both sides of the debate. If I may be allowed to summarize my own thoughts, I think that @gertjvr raises some important points about the degree of friction. It would seem that, personally, I'm so used to having my development environment set up in a certain way, that I kind of forgot that it might be different. It's true that there's friction involved in using xUnit.net, in that none of the commercially available test runners support it out of the box. Even with my preferred Test-Driven.NET, I have to do a one-time configuration before I can start using xUnit.net. Since I don't repave my box every other day, I tend to forget that.

Thus, since I (we) can accept that degree of friction when working with AutoFixture.Xunit, I think that it's a reasonable argument that one must accept a bit of friction of using AutoFixture.NUnit, too.

I do agree with @ecampidoglio that making this work on CI is important. However, concerning our own CI build, I believe that if BuildRelease.ps1 succeeds without warning, then there's a really good chance the CI build will succeed as well - and BuildRelease.ps1 succeeds on my machine. If it fails on any other machine, now's the time to inform us :)

Then there's the debate about whether or not the code was ported from AutoFixture.Xunit, and that it has nothing to do with NUnit itself. First of all, @ecampidoglio previously wanted to pull most of the types in AutoFixture.Xunit into a reusable library, and I quite strongly opposed that (which I still believe was the right decision, although I'm rarely 100 % sure about anything). Thus, I don't think we should blame @gertjvr for porting the code.

Second, the fact that most of that code has nothing to do with NUnit is again an artefact of the limited extensibility model of NUnit. The code base itself seems quite maintainable, so I'm not worried about that.

Actually, what may have concerned me most about this PR isn't so much its maintainability, but rather the supportability of it. However, at this point, I have come to believe that we should give it the benefit of the doubt. There certainly seems to be a demand for AutoFixture.NUnit, and I believe @gertjvr has argued consistently and rationally for his case.

My main remaining concern regards the future roadmap of NUnit, as described by @gertjvr :

NUnit 3.0 is scheduled to be released at the end of the year

Does this mean that the solution in this PR will not be an idiomatic NUnit 3 add-on? It probably does, but then, many organizations will probably remain stuck on NUnit 2 for a long time yet. Apart from that, I don't believe in holding back software just because some vapourware is scheduled for release some time in the future. Rather, I think that we could explicitly address the versioning issue by calling this package AutoFixture.NUnit2.

What do you all think?

@gertjvr
Contributor
gertjvr commented Sep 13, 2013

@ploeh Thank you,

This is a breath of fresh air, I am not opposed to renaming it to AutoFixture.NUnit2

@ecampidoglio
Member

It seems to me that all of the raised issues have been addressed and/or argued for in one way or another.

As I said before, it would be a pity to let this go. Let's release it and keep an eye on the feedback from the community.

@moodmosaic
Member

Aside the technical parts, and the maintainability, I also tend to agree with @gertjvr's point about the degree of friction.

Yet, we should not rush though.. (Speed Kills) :)

Wouldn't it be much more maintainable if the types in the Addins folder could be referenced from an external package?

All the other projects have external dependencies so that wouldn't be a concern..

About porting/copying, #118 was about attributes that are trivial to implement. How safe and easy would it be to maintain two copies of:

CompositeDataAttribute
CompositeDataAttributeSufficientDataTest
CompositeDataAttributeInsufficientDataTest

These classes are quite complex to be copied and maintained twice afterwards.

@moodmosaic
Member

@ecampidoglio If #156 gets released under a contrib project and #158 gets released under the main project would that be fair for @BrianZell? Both implementations have/had pros and cons..

@ecampidoglio
Member

@moodmosaic True. However, the programming model offered by #158 IMO is more intuitive compared to the one in #156. It's more like what I'd expect it to be, given how it currently works in xUnit.

Don't get me wrong, I still think that the installation aspect of the AddIn is going to be a hassle. It puts a dependency on the out-of-process environment where the tests are executed, which is exactly what unit testing is meant to isolate from.

Still, it does seem that AddIns are the currently preferred way to extend the runtime behavior of NUnit. If we're going to support it in AutoFixture at this point in time, there really isn't any other way.

@moodmosaic
Member

@ecampidoglio What I am trying to describe is that after a potential release we should also consider accepting a similar AutoFixture.MSTest candidate with similar pros and cons, additional types to extend a limited extensibility model, as well as duplication of complex types.

Probably then, the rule of three will be invoked and an Extensions library is going to be created and eventually AutoFixture is going to be all about supporting its glue libraries and complex, or even complicated, extensibility models which might break even now and then because not everyone follows Semantic Versioning.

That is the reason why I still hold my opinion that this might not be the way forward...

@ecampidoglio
Member

@moodmosaic You have a valid point. However, there seems to be a high enough demand for NUnit support to justify the hassle in this case. The alternative would be to only integrate with third-party frameworks that offer a "nice" API within AutoFixture's core and leave the rest to the Contrib project.

@gertjvr
Contributor
gertjvr commented Sep 13, 2013

@moodmosaic Thank you for actually agreeing on one point with me, but really arguing about feelings really, it is far too dangerous wars have been started on feelings without facts.

Feelings have no place in a debate or logical discussion and should be avoided at all costs But I'll bite...

@moodmosaic wrote:

@ecampidoglio If #156 gets released under a contrib project and #158 gets released under the main project would that be fair for @BrianZell? Both implementations have/had pros and cons..

I'll propose this premise, testing frameworks extensions should be included if they use the testing frameworks extensibility model (whether you agree with its testing frameworks extensibility model or not)

Then there is this statement Both implementations have/had pros and cons..
Can you quantify and list the pros / cons? and from which perspective too? I'll try

From the intended user view:

  • #158
    • Any con you can raise will also be present in AutoFixture.xUnit
    • Overwhelming pro, all the current training on how to use AutoFixture.xUnit is relevant for AutoFixture.NUnit
  • #156
    • biggest con is it violates the DRY Principal we can't assume a developeris using any code refactor extensions like Resharper, so renaming a method there is an inherent risk that the test fails in an inconsistent way, because the user forgot to change it in both places. Something @ecampidoglio has raised before.

From the maintainers view:

  • #158

    • Only con is the maintenance of copied class you have mentioned before here
    • @ploeh has summaries all other cons here with excepting there associated risks.
  • #156

    • You haven't discussed these I have my own opinion about the maintainability / supportability but this thread isn't the place for them.

    So to conclude @moodmosaic everything you are trying to argue for the exclusion of AutoFixture.NUnit can be argued and applied too AutoFixture.xUnit.

@moodmosaic you stated

Yet, we should not rush though.. (Speed Kills) :)

But stagnation is by far worst,

I am surprised you have released any software and if you have the software by your standards be faultless and perfect but that is just my humble opinion.

@ploeh
Member
ploeh commented Sep 14, 2013

While I agree that speed kills, I don't think we're rushing things in any way here. This PR is 20 days old now, so I do believe we've already given it quite a bit of thought.

I fully agree that it's not perfect, but there's much of the existing AutoFixture code base that isn't perfect either.

AFAICT, the only remaining 'real' argument against accepting this PR is the duplication of code across AutoFixture.Xunit and AutoFixture.NUnit2. There are a couple of arguments related to that:

Re: Duplication

We already had the discussion when @ecampidoglio wanted to pull out the attributes into a reusable library. My position back then is still my position today: most of those attributes are only thin Adapters over functionality offered in the AutoFixture kernel.

In my opinion, if we were to pull those attributes into a reusable library, we couldn't very well define them in a namespace called AutoFixture.Xunit. Thus, we'd need to come up with a namespace like AutoFixture.Declarative. However, if you consider the AutoFixture.Xunit package, that would mean that in order to write something as trivial as

[Theory, AutoData]
public void SomeTest([Frozen]IBar bar)

you'd have to import (using) not only Ploeh.AutoFixture.Xunit, but also Ploeh.AutoFixture.Declarative. It's not an insurmountable obstacle, but just adds to the friction by making those attributes a bit less discoverable.

Up until now, I've been of the opinion that this is a bad trade-off, because it puts more burden on the user, in exchange of less of a burden on us. I don't believe this will help adoption of this way of writing automated tests.

However, as I've tried to allude to here, I'm doing a bit of second-guessing here, so I may be wrong.

Re: Maintainability

Although most of the attributes in the above discussion are thin Adapters, it's true, as @moodmosaic points out, that there's complex logic in CompositeDataAttribute.

However, if you look at the history of the existing implementation (Ploeh.AutoFixture.Xunit.CompositeDataAttribute), it was last modified two years ago. This seems to indicate that while the implementation is complex, the maintenance burden is low. Thus, I'm inclined to take the chance and accept the duplication, in the hope that it's not going to be a big deal.

Re: The rule of three

Let's see what happens if we ever get into the situation where we need a third implementation of similar attributes. Although we think we can predict future extensibility requirements, we may not be able to do so. AutoFixture.MSTest sounds like such a prediction, but personally, I have no particular interest in such a thing. Let's just say that it's not on the AutoFixture roadmap, but I agree with @moodmosaic that if we receive a PR for AutoFixture.MSTest, we should consider it as seriously as we're considering the current PR.

Even if this happens, I still think that my previous argument about namespaces holds. Thus, at this point, we could consider pulling the complex implementation of CompositeDataAttribute into a shared library. However, that's a true refactoring in the sense that it wouldn't be a breaking change. The result would then be that we'd have three CompositeDataAttribute types (in three different namespaces), but only a single implementation.

Thus, I don't think it's risky to accept the duplication at this moment, because we can always address the maintainability issue if it arises.


@gertjvr FWIW, we normally accept or reject PRs without quite as much deliberation as here. Usually, it's quite clear if a PR adds value, or if it isn't useful. This PR is different, perhaps because I'm too prejudiced towards a particular way of working. Still, I try to listen to every argument without too much prejudice...

@moodmosaic
Member

@ploeh's comment solves the major point of duplication and maintainability (which is great!) since the other attributes are just a few lines of code.

That is, pulling the implementation of CompositeDataAttribute (in case another PR for a glue library opens) into a shared library looks like a great idea!

About speed, all I am suggesting is that we should at least try to test AutoFixture.NUnit2 under real conditions.


After reading this comment, it looks to me as @gertjvr was offended by my code review, or my writing.

@gertjvr If that is true, I apologize.

@gertjvr
Contributor
gertjvr commented Sep 15, 2013

@moodmosaic apology accepted, I normally try to stay neutral as possible and see the positive side, but might have been a bit quick on the trigger in my last post.

About speed, all I am suggesting is that we should at least try to test AutoFixture.NUnit2 under real conditions.

Re:
I have given the addin a workout as stated in a previous comment by implementing the StringCalculator Kata

Will rename the solution to AutoFixture.NUnit2, still working on the install.ps1

Thank you for all the time reviews and feedback, much appreciated.

@gertjvr
Contributor
gertjvr commented Sep 15, 2013

I have finished renaming the solution,

Having second thoughs about the naming of AutoTestCaseAttribute, I have seen @ecampidoglio named it WithAutoDataAttribute before we proceed what would you guys prefer we call it?

[Test, AutoTestCase]
public void SomeTest([Frozen]IBar bar)

or

[TestCase, WithAutoData]
public void SomeTest([Frozen]IBar bar)

or

[TestCase, AutoData]
public void SomeTest([Frozen]IBar bar)
@moodmosaic
Member

I think, it would be great if we can keep the same unit testing DSL in AutoFixture.NUnit2 so I tend to like [AutoData] :)

@gertjvr
Contributor
gertjvr commented Sep 15, 2013

Done, I have renamed it, ran the tests which 0 Warnings and 0 Errors.

I will be offline most of next week, my wife is being induced on monday evening.

Thanks again.

@gertjvr
Contributor
gertjvr commented Sep 16, 2013

One last commit, I have figured out how to add install.ps1 to the Ploeh.AutoFixture.NUnit2.nuspec to add the RequiredAddin to project's properties\AssemblyInfo.cs

use case

[Test, AutoData]
public void SomeTest([Frozen]IBar bar)

I have updated StringCalculatorKata to reflect these changes

@gertjvr
Contributor
gertjvr commented Sep 20, 2013

Does anybody have any feedback about the install.ps1 file?

@ploeh is there anything outstanding before you can merge this request?

@ploeh
Member
ploeh commented Sep 20, 2013

I have to admit that I haven't tested the install.ps1 file, but as far as I can tell, it adds the add-in assembly level attribute to the project into which one is installing the NuGet package. That seems like a good fix.

Regarding the PR, it seems to me that @moodmosaic is agreeing with me, and we haven't heard anything from @ecampidoglio for a while, which I will also interpret as acceptance :)

Thus, I think we're ready to move forward with this.

However, one last thing is that this PR currently can't be merged with master, so if you'd be so kind to update it to a state where it's mergeable, I'll pull when that's done.

Thank you for your patience, as well as your contribution!

@moodmosaic moodmosaic commented on an outdated diff Sep 20, 2013
Src/AutoFixture.NUnit2/Reharper/EventListenerWrapper.cs
@@ -0,0 +1,136 @@
+using System;
+using NUnit.Core;
+
+namespace Ploeh.AutoFixture.NUnit2.Reharper
@moodmosaic
moodmosaic Sep 20, 2013 Member

(Do you mean ReSharper with Reharper?)

@moodmosaic moodmosaic commented on an outdated diff Sep 20, 2013
Src/AutoFixture.NUnit2/Reharper/TestDecorator.cs
@@ -0,0 +1,26 @@
+//http://youtrack.jetbrains.com/issue/RSRP-205480
+
+using System.Reflection;
+using NUnit.Core;
+using NUnit.Core.Extensibility;
+
+namespace Ploeh.AutoFixture.NUnit2.Reharper
@moodmosaic
moodmosaic Sep 20, 2013 Member

(Do you mean ReSharper with Reharper?)

@moodmosaic moodmosaic commented on an outdated diff Sep 20, 2013
Src/AutoFixture.NUnit2/Reharper/TestMethodWrapper.cs
@@ -0,0 +1,22 @@
+//http://youtrack.jetbrains.com/issue/RSRP-205480
+
+using NUnit.Core;
+
+namespace Ploeh.AutoFixture.NUnit2.Reharper
@moodmosaic
moodmosaic Sep 20, 2013 Member

(Do you mean ReSharper with Reharper?)

@moodmosaic moodmosaic commented on an outdated diff Sep 20, 2013
Src/AutoFixture.NUnit2/Builders/AutoDataProvider.cs
@@ -0,0 +1,80 @@
+using System.Collections;
+using System.Reflection;
+using NUnit.Core;
+using NUnit.Core.Extensibility;
+
+namespace Ploeh.AutoFixture.NUnit2.Builders
+{
+ /// <summary>
+ /// AutoTestCaseProvider provides data for methods
+ /// annotated with the AutoTestCaseAttribute.
+ /// </summary>
+ public class AutoDataProvider : ITestCaseProvider2
+ {
+ #region ITestCaseProvider Members
@moodmosaic
moodmosaic Sep 20, 2013 Member

If it's easy would you mind removing the regions? Long time ago we stopped using them and we removed them throughout the code base.

@moodmosaic moodmosaic commented on an outdated diff Sep 20, 2013
Src/AutoFixture.NUnit2/Builders/AutoDataProvider.cs
+ }
+
+ /// <summary>
+ /// Return an IEnumerable providing test cases for use in
+ /// running a parameterized test.
+ /// </summary>
+ /// <param name="method"></param>
+ /// <returns></returns>
+ public IEnumerable GetTestCasesFor(MethodInfo method)
+ {
+ return GetTestCasesFor(method, null);
+ }
+
+ #endregion
+
+ #region ITestCaseProvider2 Members
@moodmosaic
moodmosaic Sep 20, 2013 Member

If it's easy would you mind removing the regions? Long time ago we stopped using them and we removed them throughout the code base.

@moodmosaic
Member

Yeap :) I think it looks great!

I left a few comments but it's nothing special.. I think :)

@ecampidoglio
Member

Yes, it looks fine by me 👍

@gertjvr
Contributor
gertjvr commented Sep 20, 2013

Hi @ploeh, have merge master into nunit branch, hope this is what you meant with:

However, one last thing is that this PR currently can't be merged with master, so if you'd be so kind to update it to a state where it's mergeable, I'll pull when that's done.

Fixed my spelling mistake and removed the regions as requested by @moodmosaic

@moodmosaic
Member

I checked out a new branch to test the changes and the BuildRelease.ps1 run successfully.

However, after installing AutoFixture.NUnit2 through NuGet (local package source), I couldn't make it work:

image

I am sure I've missed something.. yes? :)

@gertjvr
Contributor
gertjvr commented Sep 20, 2013

@moodmosaic think so too, did the same test and works with resharper and nunit-gui.exe with the addins copied to the write locations.

  1. What test runner are you using?
  2. What error message is the assert give you? Expect it to say something about missing the required addin.
@moodmosaic
Member

I use the TestDriven.Net add-in which supports NUnit.

Here is the output:

------ Test started: Assembly: ClassLibrary1.dll ------

Test 'M:ClassLibrary1.Scenario.AutoTestCaseProvidesCorrectInteger(System.Int32)' failed:   
  Expected: not 0
  But was:  0

    NUnit.Framework.AssertionException:   
      Expected: not 0
      But was:  0

    at NUnit.Framework.Assert.That(
        Object actual, IResolveConstraint expression, String message, Object[] args)
    at NUnit.Framework.Assert.AreNotEqual(Int32 expected, Int32 actual)
    Class1.cs(13,0): at ClassLibrary1.Scenario.AutoTestCaseProvidesCorrectInteger(
        Int32 primitiveValue)

0 passed, 1 failed, 1 skipped (see 'Task List'), took 0.86 seconds (Ad hoc).

@ecampidoglio
Member

Sorry to hijack the discussion, but I'd like to report that I'm also experiencing some problems. It turns out I was a little too quick with my previous response.

I pulled down the latest version of the branch and run BuildRelease.ps1. So far so good.

Then, I created a new library project targeting .NET 4.0 and installed the local AutoFixture.NUnit2 package with:

Install-Package AutoFixture.NUnit2 -Source C:\Sources\AutoFixture\NuGetPackages

And I got this error:

Attempting to resolve dependency 'autofixture (≥ 3.7.1)'.
Attempting to resolve dependency 'nunit (≥ 2.6.2)'.
Installing 'AutoFixture 3.7.1'.
Successfully installed 'AutoFixture 3.7.1'.
Installing 'NUnit 2.6.2'.
Successfully installed 'NUnit 2.6.2'.
Installing 'AutoFixture.NUnit2 3.7.1'.
Successfully installed 'AutoFixture.NUnit2 3.7.1'.
Adding 'AutoFixture 3.7.1' to ConsoleApplication1.
Successfully added 'AutoFixture 3.7.1' to ConsoleApplication1.
Adding 'NUnit 2.6.2' to ConsoleApplication1.
Successfully added 'NUnit 2.6.2' to ConsoleApplication1.
Adding 'AutoFixture.NUnit2 3.7.1' to ConsoleApplication1.
Successfully added 'AutoFixture.NUnit2 3.7.1' to ConsoleApplication1.
Method invocation failed because [System.Object[]] doesn't contain a method named 'Contains'.
At C:\Sources\ClassLibrary1\packages\AutoFixture.NUnit2.3.7.1\tools\install.ps1:9 char:20
**+ if(-not $c.Contains <<<< ($addinContent))

  • CategoryInfo : InvalidOperation: (Contains:String) [], RuntimeException
  • FullyQualifiedErrorId : MethodNotFound**

Ideas?

@gertjvr
Contributor
gertjvr commented Sep 20, 2013

@moodmosaic not sure, will have install testdriven.net and see why what version of testdriven.net are you using?

@ecampidoglio that is a powershell issue, will look into it, I am assuming @moodmosaic the installation work, because he was able to run the test.

@moodmosaic
Member

@ecampidoglio Hm.. I installed AutoFixture.NUnit2 by adding a local package source via

Tools > Options > Package Manager > Package Sources

I use TestDriven.NET 3.4.2808 RTM.

@gertjvr
Contributor
gertjvr commented Sep 20, 2013

Hehe downloaded TestDriven.NET-3.4.2808, copied Ploeh.AutoFixture.dll and Ploeh.AutoFixture.NUnit2.dll to C:\Program Files (x86)\TestDriven.NET 3\NUnit\2.6\addins

------ Test started: Assembly: ClassLibrary2.dll ------

1 passed, 0 failed, 0 skipped, took 0.74 seconds (NUnit 2.6.1).
@gertjvr
Contributor
gertjvr commented Sep 20, 2013

@ecampidoglio what version of package manager are you using?

@gertjvr
Contributor
gertjvr commented Sep 20, 2013

@ecampidoglio installed without issues, can you try in the package manager Get-ExecutionPolicy is it set too?

Package Manager Console Host Version 2.7.40808.167

Type 'get-help NuGet' to see all available NuGet commands.

PM> Install-Package AutoFixture.NUnit2 -Source C:\Users\gertj_000\Documents\GitHub\AutoFixture\NuGetPackages
Attempting to resolve dependency 'autofixture (≥ 3.7.1)'.
Attempting to resolve dependency 'nunit (≥ 2.6.2)'.
Installing 'AutoFixture 3.7.1'.
Successfully installed 'AutoFixture 3.7.1'.
Installing 'NUnit 2.6.2'.
Successfully installed 'NUnit 2.6.2'.
Installing 'AutoFixture.NUnit2 3.7.1'.
Successfully installed 'AutoFixture.NUnit2 3.7.1'.
Adding 'AutoFixture 3.7.1' to ClassLibrary3.
Successfully added 'AutoFixture 3.7.1' to ClassLibrary3.
Adding 'NUnit 2.6.2' to ClassLibrary3.
Successfully added 'NUnit 2.6.2' to ClassLibrary3.
Adding 'AutoFixture.NUnit2 3.7.1' to ClassLibrary3.
Successfully added 'AutoFixture.NUnit2 3.7.1' to ClassLibrary3.
@gertjvr
Contributor
gertjvr commented Sep 20, 2013

@ecampidoglio:

param($installPath, $toolsPath, $package, $project)

$projectPath = Split-Path -Parent $project.FullName
$projAssemblyInfo = Join-Path $projectPath "Properties\AssemblyInfo.cs"
$addinContent = "[assembly: NUnit.Framework.RequiredAddin(Ploeh.AutoFixture.NUnit2.Constants.AutoDataExtension)]";

$c = Get-Content $projAssemblyInfo

if(-not $c.Contains($addinContent)) 
{
    Add-Content $projAssemblyInfo $addinContent
}

the line $c = Get-Content $projAssemblyInfo is failing to find your project location $c should be an string which would have a contains method, speculating could be a illegal powershell char in the file path.

I could try use the following Get-Content c:\scripts\test.txt | Select-String rather but that will have to be later, need some sleep.

@ecampidoglio
Member

@gertjvr

I'm running on NuGet version 2.7.40808.167. The Execution Policy is set to unrestricted.

I tried to install the package using the Manage NuGet Packages dialog after adding my local package directory C:\Sources\AutoFixture\NuGetPackages to the list of package sources. I don't see any errors but the AssemblyInfo.cs file doesn't get modified.

If I run this in the Package Manager Console:

Get-Project | select -ExpandProperty FullName | Split-Path -Parent

I get:

C:\Sources\ClassLibrary1\ClassLibrary1
@ecampidoglio
Member

I think I found the problem. If you do:

Get-Content "Path\To\Some\File.txt"

PowerShell will give you an array where each element contains a line in the text file. In order to retrieve the contents of the file as a single string, you'll have to pipe it through Out-String:

$content = Get-Content "Path\To\Some\File.txt" | Out-String

or use the .NET classes directly:

$content = [System.IO.File]::ReadAllText("Path\To\Some\File.txt")

In PowerShell 3.0 they even added a new -Raw parameter to the Get-Content cmdlet that does just that.

@moodmosaic
Member

Hehe downloaded TestDriven.NET-3.4.2808, copied Ploeh.AutoFixture.dll and Ploeh.AutoFixture.NUnit2.dll to C:\Program Files (x86)\TestDriven.NET 3\NUnit\2.6\addins

I had to run an elevated command prompt, or explorer window, to copy the files.. :$

It might sound odd, but I (and perhaps other TestDriven.Net users) develop using a non-administrator account.

@gertjvr
Contributor
gertjvr commented Sep 20, 2013

@ecampidoglio I have added the | Out-String and worked for me. thanks

@gertjvr
Contributor
gertjvr commented Sep 20, 2013

@moodmosaic

It might sound odd, but I (and perhaps other TestDriven.Net users) develop using a non-administrator account.

If doesn't sound odd, at my previous employer our pc's where on lock down had to ask permission for everything, so not surprised,

I am more puzzled why TestDriven.net worked without the addin, and didn't throw the expected exception that the addin is required, like nunit-gui.exe nunit-console.exe, resharper and ncrunch.

@gertjvr
Contributor
gertjvr commented Sep 22, 2013

@ploeh @moodmosaic @ecampidoglio I have found an alternative solution using Select-String been unable to make it fail locally would you guys mind testing it?

@moodmosaic
Member

Hm.. I get the same results with this comment. (Did you get it working on your local TestDriven.Net installation?)

@gertjvr
Contributor
gertjvr commented Sep 22, 2013

@moodmosaic short answer is yes,

Testdrivent.net does work if the addins dll's (Ploeh.AutoFixture.dll and Ploeh.AutoFixture.NUnit2.dll) are copied under C:\Program Files (x86)\TestDriven.NET 3\NUnit\2.6\addins as per instructions in C:\Program Files (x86)\TestDriven.NET 3\NUnit\2.6\addins.txt and as you said previously need elevated account to copy the dlls to that location.

I wrote before:

I am more puzzled why TestDriven.net worked without the addin, and didn't throw the expected exception that the addin is required, like nunit-gui.exe nunit-console.exe, resharper and ncrunch.

Trying to get to the bottom of this, but not making any traction, have a assumption that the Resharper Fix might be hiding the exception in TestDriven.net but I dont have any proof of this yet.

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

I wrote:

Trying to get to the bottom of this, but not making any traction, have a assumption that the Resharper Fix might be hiding the exception in TestDriven.net but I dont have any proof of this yet.

@ploeh @ecampidoglio @moodmosaic figured it out, when running TestDriven.Net with the addin dll's missing you will get the following exeception

------ Test started: Assembly: ClassLibrary1.dll ------

Test 'M:ClassLibrary1.Class1.AutoTestCaseProvidesCorrectInteger(System.Int32)' failed:   Expected: True
  But was:  False

    NUnit.Framework.AssertionException:   Expected: True
      But was:  False

    at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
    at NUnit.Framework.Assert.IsTrue(Boolean condition)
    Class1.cs(12,0): at ClassLibrary1.Class1.AutoTestCaseProvidesCorrectInteger(Int32 primitiveValue)

0 passed, 1 failed, 1 skipped (see 'Task List'), took 0.47 seconds (Ad hoc).

TestDriven.Net displays the RequireAddin execption under Task List -> Add-ins and Macro ...its really hidden away stumbled upon it by acident...

@ploeh What is outstanding before you can merge this PR? I haven't changed any of the code for 18 days if you exclude the install.ps1 script, only thing in question is how 3rd party tools interact with the addin, and inperticuar how TestDriven.Net report the missing RequiredAddin.

@moodmosaic
Member

@gertjvr Besides TestDriven.Net and ReSharper, are there any other tools we could try AutoFixture.NUnit2?

@ecampidoglio As I don't have (currently) ReSharper installed on my machine, would you mind trying the latest changes and tell us if it works out of the box now? :)

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@moodmosaic I have tried with nunit.exe and nunit-console.exe, R# and TestDriven.Net in my mind this covers the rule of three :) (as in works with three independent implementations)

@ecampidoglio remember to copy Ploeh.AutoFixture.dll and Ploeh.AutoFixture.NUnit2.dll to C:\Program Files (x86)\JetBrains\ReSharper{version}\Bin\addins, should just work, but could also try without the addin to verify the RequiredAddin exception.

@moodmosaic
Member

@gertjvr So basically when the user updates to a new version of AutoFixture, he would have to remember copying the required assemblies to each test runner's add-ins location, yes?

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@moodmosaic I am unsure to be honest, might be because AutoDataAttribute depends on IFixture (unchanged 6 months ago) and Fixture (unchanged 4 months ago)

@moodmosaic
Member

@gertjvr And if the user wants (for some reason) to uninstall the package these files will become orphan?

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@moodmosaic That would be the case too, again this is the limitation of the NUnit extensibility model :(

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@moodmosaic Do you see these issues as a show stopper?

@ecampidoglio
Member

So, I tried to use the add-in with R# 8.0.1 (the version I'm currently on).

First, I tried to run the following test with R# after adding the AutoFixture.NUnit2 package to my project but without copying the required add-in DLLs:

using NUnit.Framework;
using Ploeh.AutoFixture.NUnit2;

[Test, AutoData]
public void It_should_work(string aString, int aNumber)
{
    Assert.That(aString, Is.Not.Null.Or.Empty);
    Assert.That(aNumber, Is.Not.EqualTo(0));
}

but got this exception:

System.Reflection.TargetParameterCountException : Parameter count mismatch.
at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at NUnit.Core.Reflect.InvokeMethod(MethodInfo method, Object fixture, Object[] args)
at NUnit.Core.TestMethod.RunTestMethod(TestResult testResult)
at NUnit.Core.TestMethod.RunTestCase(TestResult testResult)

I verified that the RequiredAddIn attribute had been added to the AssemblyInfo.cs file:

// ...
[assembly: AssemblyVersion("1.0.0.0")]
[assembly: AssemblyFileVersion("1.0.0.0")]
[assembly: NUnit.Framework.RequiredAddin(Ploeh.AutoFixture.NUnit2.Constants.AutoDataExtension)]

Then, I manually copied the following DLLs from their respective NuGet packages:

  • Ploeh.AutoFixture.dll (from AutoFixture.3.8.1)
  • Ploeh.AutoFixture.NUnit2.dll (from AutoFixture.NUnit2.3.8.1)

into C:\Program Files (x86)\JetBrains\ReSharper\v8.0\Bin\addins and restarted Visual Studio but still got the exact same exception and then everything worked as expected 👍

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@ecampidoglio Will see if I can replicate your findings, but must say very confused atm.

@ecampidoglio
Member

For convenience, I think we should include some batch files that copy the required DLLs from their respective NuGet packages into the specific folders for different test runners. Something like:

  • InstallForTDNet.bat
  • InstallForNCrunch.bat
  • InstallForReSharper-7.0.bat
  • InstallForReSharper-7.1.bat
  • InstallForReSharper-8.0.bat

what do you think?

@moodmosaic
Member

@ecampidoglio But then, are we going to include batch files to handle updates and removals too?

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@ecampidoglio I just did a msbuild again, replicated what you did, I am not running as administrator, without the addin I get parameter miscount, and with the dll's under the addins folder it works. Didn't need to restart VS just used Ctrl + U, Ctrl + L to run the test.

Also using R# 8.0.1

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@ecampidoglio just for my own sanity you got it working with R# 8.0.1?

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@ecampidoglio wrote:

For convenience, I think we should include some batch files that copy the required DLLs from their respective NuGet packages into the specific folders for different test runners. Something like:

We could include that in the install.ps1 and uninstall.ps1 file verify known locations if they exists copy the required dll to those locations? would make it a lot more convenient, so many comment back @moodmosaic said that it would be ideal as user would be installing using nuget package exclusively.

But we could have a two phase approach have the individual bat files, and then figure out which ones to run in the insall.ps1 and uninstall.ps1 files.

@ecampidoglio
Member

@gertjvr Yes, I got it working but the RequiredAddIn attribute didn't work as expected. Even if I hadn't copied the DLLs into the addin folder, I was still able to run the test with R#'s test runner and got the aforementioned exception.

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@ecampidoglio aforementioned exception being parameter count mismatch or required addin exception?

@ecampidoglio
Member

@gertjvr Correct.

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@ploeh @moodmosaic @ecampidoglio before we go down the rabbit hole again, allow me to summarise what my thoughts are.

I was able to get the AutoFixture.NUnit2 working with nunit.exe and nunit-console.exe, R# and now TestDriven.Net three independent implementation of the nunit-runner, not without a few issues like the location and what dll's to copy.

@moodmosaic assume you have verified it working with TestDriven.Net and @ecampidoglio has got it working with R#

@ploeh has already explained that nunit extensibility model is to blame for the distribution issues (install / uninstall) and accepted the friction it might cause.

I don't want to waste your time or mine anymore reviewing this PR, the code has been unchanged for 18 days and the PR is over a month old.

I think it getting to the point either this is adding value or its not? I don't expect that there won't be any growing pains but think we have covered most common cases in the PR that might pop-up.

What do you say?

@ecampidoglio
Member

@moodmosaic

But then, are we going to include batch files to handle updates and removals too?

Well, at the very least we should find a solution for the update problem.

Each test runner add-in is going to have its own private copy of the Ploeh.AutoFixture.dll assembly, which may or may not be of the same version as the one used in the current project. This isn't necessarily a problem per se. One version of AutoFixture would be used to build the project while another one would be used to run the tests. As long as their compatible with each other, everything is fine.

However, if we were to add a new feature or a bug fix in AutoFixture core that the user wishes to take advantage of in her project, she'd have to manually update the DLL for all of the test runners, both on the local machine as well as on any CI servers.
By the same token, if we were to introduce a breaking change in Ploeh.AutoFixture.dll, some tests might suddenly begin to fail on configurations where the test runner add-in and the package used in the project weren't kept in sync.

One way to mitigate this could be to include some code in the add-in that, upon startup, compares the version of AutoFixture that is being linked to from the project and the one used by the add-in itself. This way it could warn the user if the versions mismatch and, depending on which part of the version number is different, recommend to update.

@moodmosaic
Member

@moodmosaic assume you have verified it working with TestDriven.Net

No, it's not working for me out of the box – but please don't interpret this as a discouragement.

All I am saying is that it would be nice to protect our users so they should not be left frustrated on the release date because the batteries were not included..

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@ploeh @moodmosaic @ecampidoglio I looked at the dependency hierarchy moved the NUnitAddin to its own project and moved DataAttribute to the same project thereby removing any dependency on AutoFixture itself. Allowing us to only have to copy one dll Ploeh.AutoFixture.NUnit2.Addins.dll to the addins folders. This will solve the updates and versioning issue.

I have ran the msbuild script without any errors or warnings. and retested with nunit.exe R# and testdriven.net with success.

Unless we include the xcopy of this single dll to known locations withing the install.ps1 there will never be an out of the box solutions, this will ensure as @moodmosaic put it "that the batteries are included".

Please could you guys review this for a final time, but I am going to park it there.

@ploeh
Member
ploeh commented Sep 23, 2013

@gertjvr wrote:

@ploeh What is outstanding before you can merge this PR? I haven't changed any of the code for 18 days if you exclude the install.ps1 script, only thing in question is how 3rd party tools interact with the addin, and inperticuar how TestDriven.Net report the missing RequiredAddin.

Well, I was pretty much ready to pull, but then you started churning on the Install.ps1 file. AFAICT, that particular file doesn't work.

I don't think we necessarily need this automation in order to release the package. I'd rather release the package without a faulty automation script, than with a faulty script.

If we release something with a script that doesn't always work, we'll have to support it, and it may be that we'll never be able to make it work. Releasing a script can be interpreted as a promise that this is part of the package.

If we don't release the script, it's pretty clear to everyone that some assembly may be required. Then we can always add the script later, if we can make it work.

Thus, it seems to me that it would be wiser to yank the script for now...

@gertjvr
Contributor
gertjvr commented Sep 23, 2013

@ploeh The script isn't in question anymore @moodmosaic started the discussion about nunit extensibility, and down the discussion went.

I think we park this PR as its not going anywhere, and I don't have the energy, time or patience anymore.

Thanks everyone cheers.

@gertjvr gertjvr closed this Sep 23, 2013
@gertjvr
Contributor
gertjvr commented Sep 24, 2013

@ploeh few of my colleagues convinced me this will be a waste not to pursue. This feature will be main counter argument for other devs / clients to use AutoFixture (thank you @robdmoore and @bradleyboveinis)

I might have overreacted, and I apologise (its no excuse but not getting much sleep these last few days)

The code is functional has been tested with at least three independent implementations of nunit-runners (nunit, R#, TestDriven.Net) (Also removed the AutoFixture dependency from AutoFixture.NUnit2.Addins.dll so only need to copy single assembly to the required addins folder, and is future proof as isn't depended on a specific version of the AutoFixture.NUnit2 assembly)

The install.ps1 uses only powershell functions and will add the RequiredAddin reliably imo, do you still want to remove this? (only remaining issue why this could fail is if the developer doesn't have execution rights)

@moodmosaic I am not going to discuss the nunit extensibility issue, out of the box isn't possible and will require some friction. (ie copying dll or orphan dll removal), it is what it is!

I'll understand if you don't want to merge this anymore.

@gertjvr gertjvr reopened this Sep 24, 2013
@ploeh
Member
ploeh commented Sep 24, 2013

@gertjvr Glad to see you haven't given up completely. I agree that we can't keep thrashing on the NUnit extensibility part, because we can't do anything about it.

There's no purpose in attempting to make this perfect, because it's not going to be perfect - just like the rest of the AutoFixture code base isn't perfect. The question is whether or not this PR adds more value than friction, and I believe there's quite a good chance that this is the case.

I have some time later today when I'll attempt to pull and look this over again.

@moodmosaic and @ecampidoglio Thank you very much for your diligent and thorough review of this PR. I agree that perhaps more batch files etc. might be an added benefit, but isn't this something that can be added later? Given that the NUnit extensibility story is as it is, are there any blockers in this PR?

@moodmosaic
Member

@ploeh Up to commit ba9bd81 there are no blockers on my machine. (I haven't tested the changes from yesterday's commits.)

@gertjvr
Contributor
gertjvr commented Sep 24, 2013

@ploeh Just as a guide, you only have to copy the Ploeh.AutoFixture.NUnit2.Addins.dll to addins folder

  • NUnit -> C:\Program Files (x86)\NUnit 2.6.2\bin\addins
  • TD.Net -> C:\Program Files (x86)\TestDriven.NET 3\NUnit\2.6\addins
  • R# -> C:\Program Files (x86)\JetBrains\ReSharper\v8.0\Bin\addins

If you install the AutoFixture.NUnit2 package using package manager
Install-Package AutoFixture.NUnit2 -source <package location> then PowerShell ExecutionPolicy must be set to RemoteSigned if you use a predefined packages source like "local nuget feed" then this is unessesary.

@ploeh ploeh merged commit 5568f02 into AutoFixture:master Sep 24, 2013
@ploeh
Member
ploeh commented Sep 24, 2013

This is now live as AutoFixture.NUnit2 3.9.0. Thank you for your contribution and your patience.

P.S. I've noticed that the number of tests passed reported by the build server is constant at 4089 tests passed (and 0 failed). I would have expected that number to have increased, so it looks like the tests aren't being executed on the build server.

@gertjvr gertjvr deleted the unknown repository branch Sep 25, 2013
@moodmosaic
Member

Here is another surprising behavior:

If you make a few tests to fail (in the AutoFixture.NUnit2 extension) you will notice that the number of tests failed reported by the BuildRelease.ps1 is constant at 1 failed:

...\BuildRelease.msbuild" (default target) (1) ->
(Test target) ->
...\BuildRelease.msbuild(95,9): error MSB6006: "nunit-console.exe" exited with code 2.

    0 Warning(s)
    1 Error(s)
@gertjvr
Contributor
gertjvr commented Sep 30, 2013

@moodmosaic the reasons for this is the nunit runner has been setup to not continue on failure 'ContinueOnError="false"'

<Target Name="Test" DependsOnTargets="CopyToNUnitAddinsFolder">
    <xunitproject ProjectFile="Src\All.xunit" />
    <NUnit ToolPath="$(NUnitToolsFolder)" Assemblies="Src\AutoFixture.NUnit2.UnitTest\bin\Release\Ploeh.AutoFixture.NUnit2.UnitTest.dll" ContinueOnError="false" />
    <NUnit ToolPath="$(NUnitToolsFolder)" Assemblies="Src\AutoFixture.NUnit2.Addins.UnitTest\bin\Release\Ploeh.AutoFixture.NUnit2.Addins.UnitTest.dll" ContinueOnError="false" />
</Target>

If this isn't the desired result then you can remove the attribute or set it to true.

@ploeh
Member
ploeh commented Sep 30, 2013

FWIW I've now changed ContinueOnError to true.

@kmcginnes

I wanted to chime in quickly about a comment made a while back about TestDriven.Net failing to execute as expected when running a parameterized test with the AutoData attribute. The reason behind this is simple, but not obvious.

As a reminder, here is the test output that @gertjvr reported:

------ Test started: Assembly: ClassLibrary1.dll ------

Test 'M:ClassLibrary1.Class1.AutoTestCaseProvidesCorrectInteger(System.Int32)' failed:   Expected: True
  But was:  False

    NUnit.Framework.AssertionException:   Expected: True
      But was:  False

    at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
    at NUnit.Framework.Assert.IsTrue(Boolean condition)
    Class1.cs(12,0): at ClassLibrary1.Class1.AutoTestCaseProvidesCorrectInteger(Int32 primitiveValue)

0 passed, 1 failed, 1 skipped (see 'Task List'), took 0.47 seconds (Ad hoc).

I'm going to quote Patrick Lioi who is currently on an adventure to write a convention based testing framework he calls Fixie. In his very first post regarding Fixie he encounters this very "issue". He states, "...TD.NET has a convenient feature that it can run methods even if they are not officially recognized as test methods in a test fixture."

When the test was executed, NUnit didn't recognize it as a test because the method signature included parameters, and TestCase attribute was not found. Therefore, TestDriven.Net executed the method in ad hoc mode. This is evident by the last line of the test output.


As an aside, I'm glad everyone decided to move ahead with this pull request. Even with the added friction of NUnit's poorly thought out extensibility model, the value added from this functionality is tremendous. My deepest thanks to all those involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment