nunit / nunit-vs-adapter

Runs NUnit V2 tests inside the Visual Studio 2012 or later Test Explorer window.
MIT License
50 stars 43 forks source link

VS2015 Test Adapter does not see "naked" Test method override as a Test method #159

Closed mwpowellhtx closed 6 years ago

mwpowellhtx commented 6 years ago

I did not mine the current issues for anything similar. If it's a duplicate, please mark it as such. Also, I haven't verified across Visual Studio versions (i.e. 2012, 2013, 2017, etc).

It is easier to illustrate what's going on. In the following example, the Adapter does not see the overridden Verify method as a Test method.

[TestFixture]
public class BaseTests
{
  [Test]
  public virtual void Verify()
  {
  }
}

public class DerivedTests : BaseTests
{
  public override void Verify()
  {
    base.Verify();
  }
}

However, as soon as I add the TestAttribute decoration, the Adapter then does see the method accordingly.

[Test]
public override void Verify()
{
  // ...
}

As a hint, and without reviewing the code, from a Reflection perspective, probably something to do with the target type, in this case DerivedTests, and Base Signature in terms of MethodInfo.

CharliePoole commented 6 years ago

This is most likely not an issue with the adapter. The adapter's only responsibility is to forward to VS the tests that the NUnit Test Engine gives it. The engine, in turn, forwards the tests it gets from the framework. Actual detection of tests takes place in the framework.

To my knowledge, as the author, there is no code in the adapter that fails to pass on tests to VS based on attributes. In fact, the adapter only receives an XML representation of the tests and doesn't know anything about the metadata in the test assembly. IOW, it's an adapter.

We could move this to the framework, but couple of confirmation steps are needed first:

  1. Does this happen with the nunit console runner as well? If so, that absolutely confirms it as an issue of the framework.
  2. Does this happen with the latest version of the framework? Since we don't maintain NUNit V2, that's necessary if we are going to do anything about it.

As I indicated in the discussion forum, I believe it's unlikely that we will want to change this long-standing behavior that was designed into NUnit from the start. But if you can confirm it, let's have the discussion.

mwpowellhtx commented 6 years ago

@CharliePoole So, in other words, my path forward there is v3; fair enough, no problem. It's easy enough to decorate with TestAttribute when necessary.

For NUnit v2/v3 migration paths, just be aware, when attributes are Inherited=true (the default, I think), one expects those details to propagate. That's not currently happening for v2.6.4 TestAttribute, as contrasted with TestFixtureAttribute which does propagate.

mwpowellhtx commented 6 years ago

Just for sake of discussion, literally, this is the code that is exposed via inspection:

#region Assembly nunit.framework, Version=2.6.4.14350, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77
// <path/>\packages\NUnit.2.6.4\lib\nunit.framework.dll
#endregion

using System;

namespace NUnit.Framework
{
    //
    // Summary:
    //     Adding this attribute to a method within a NUnit.Framework.TestFixtureAttribute
    //     class makes the method callable from the NUnit test runner. There is a property
    //     called Description which is optional which you can provide a more detailed test
    //     description. This class cannot be inherited.
    [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
    public class TestAttribute : Attribute
    {
        public TestAttribute();

        //
        // Summary:
        //     Descriptive text for this test
        public string Description { get; set; }
    }
}
CharliePoole commented 6 years ago

See https://github.com/nunit/nunitv2/blob/master/src/NUnitCore/core/Builders/NUnitTestCaseBuilder.cs#L46

On Sat, Aug 5, 2017 at 8:47 AM, Michael W Powell notifications@github.com wrote:

Just for sake of discussion, literally, this is the code that is exposed via inspection:

region Assembly nunit.framework, Version=2.6.4.14350, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77// \packages\NUnit.2.6.4\lib\nunit.framework.dll

endregion

using System; namespace NUnit.Framework { // // Summary: // Adding this attribute to a method within a NUnit.Framework.TestFixtureAttribute // class makes the method callable from the NUnit test runner. There is a property // called Description which is optional which you can provide a more detailed test // description. This class cannot be inherited. [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] public class TestAttribute : Attribute { public TestAttribute();

    //
    // Summary:
    //     Descriptive text for this test
    public string Description { get; set; }
}

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nunit/nunit-vs-adapter/issues/159#issuecomment-320450933, or mute the thread https://github.com/notifications/unsubscribe-auth/ACjfhSAvJtt-hV7fH7N1rVWs_oQ829nQks5sVI6dgaJpZM4OufNM .

CharliePoole commented 6 years ago

Hopefully, this info will help you present the issue in a light that gives it a bit of a chance to be accepted. :-)

Charlie

On Sat, Aug 5, 2017 at 8:58 AM, Charlie Poole charliepoole@gmail.com wrote:

See https://github.com/nunit/nunitv2/blob/master/src/ NUnitCore/core/Builders/NUnitTestCaseBuilder.cs#L46

On Sat, Aug 5, 2017 at 8:47 AM, Michael W Powell <notifications@github.com

wrote:

Just for sake of discussion, literally, this is the code that is exposed via inspection:

region Assembly nunit.framework, Version=2.6.4.14350, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77// \packages\NUnit.2.6.4\lib\nunit.framework.dll

endregion

using System; namespace NUnit.Framework { // // Summary: // Adding this attribute to a method within a NUnit.Framework.TestFixtureAttribute // class makes the method callable from the NUnit test runner. There is a property // called Description which is optional which you can provide a more detailed test // description. This class cannot be inherited. [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] public class TestAttribute : Attribute { public TestAttribute();

    //
    // Summary:
    //     Descriptive text for this test
    public string Description { get; set; }
}

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nunit/nunit-vs-adapter/issues/159#issuecomment-320450933, or mute the thread https://github.com/notifications/unsubscribe-auth/ACjfhSAvJtt-hV7fH7N1rVWs_oQ829nQks5sVI6dgaJpZM4OufNM .

CharliePoole commented 6 years ago

HI Michael,

Actually, TestAttribute and TestFixtureAttribute are propagating in exactly the same way. The difference is that override and new have no meaning for a class.

Charlie

On Sat, Aug 5, 2017 at 9:01 AM, Charlie Poole charliepoole@gmail.com wrote:

Hopefully, this info will help you present the issue in a light that gives it a bit of a chance to be accepted. :-)

Charlie

On Sat, Aug 5, 2017 at 8:58 AM, Charlie Poole charliepoole@gmail.com wrote:

See https://github.com/nunit/nunitv2/blob/master/src/NUnitCore/ core/Builders/NUnitTestCaseBuilder.cs#L46

On Sat, Aug 5, 2017 at 8:47 AM, Michael W Powell < notifications@github.com> wrote:

Just for sake of discussion, literally, this is the code that is exposed via inspection:

region Assembly nunit.framework, Version=2.6.4.14350, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77// \packages\NUnit.2.6.4\lib\nunit.framework.dll

endregion

using System; namespace NUnit.Framework { // // Summary: // Adding this attribute to a method within a NUnit.Framework.TestFixtureAttribute // class makes the method callable from the NUnit test runner. There is a property // called Description which is optional which you can provide a more detailed test // description. This class cannot be inherited. [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] public class TestAttribute : Attribute { public TestAttribute();

    //
    // Summary:
    //     Descriptive text for this test
    public string Description { get; set; }
}

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nunit/nunit-vs-adapter/issues/159#issuecomment-320450933, or mute the thread https://github.com/notifications/unsubscribe-auth/ACjfhSAvJtt-hV7fH7N1rVWs_oQ829nQks5sVI6dgaJpZM4OufNM .

CharliePoole commented 6 years ago

Based on a quick look at the code, I believe we maintained compatibility WRT the treatment of overrides when we went to NUnit 3. Any change we would make is breaking, but perhaps not seriously.

I'm going ahead and moving this to the right place. Seeing the code is actually confirmation enough for me.

CharliePoole commented 6 years ago

Issue moved to nunit/nunit #2349 via ZenHub