TestContext.CurrentContext.Test.Properties does not contain value(s) from PropertyAttribute when using TestCaseAttribute

[corsairmarks]

NUnit version: 3.2.0 (from NuGet)

Test execution: Visual Studio adapter

Steps to reproduce:

1. Create a test method marked with [TestCase] and [Property("key", "value")]
2. Executed the test
3. Inspect the value of TestContext.Current.Test.Properties - it will be an empty collection, where it should have 1 value

Code example (failing test):

[TestCase(5)]
[Property("Answer", 42)]
public void TestCaseCanAccessItsOwnProperties(int x)
{
    Assert.That(TestContext.CurrentContext.Test.Properties.Get("Answer"), Is.EqualTo(42));
}

Previously reported on LaunchPad

This does not appear to be the same bug as #548 or #796.

[corsairmarks]

I am working on this issue, my work can be previewed here: corsairmarks/nunit@a2baec0d138ff151f4a9041ae6041f482cdf4de1

I'm not sure this is the right approach, so I am currently looking through the NUnit source and learning how and build-up of a [TestCase] differs from a [Test].

[corsairmarks]

I have now also tested this with the console runner.

nunit3-console.exe version: 3.2.0 (installed using with Windows msi installer)
Command: nunit3-console.exe .\Sample.Test.dll
Code that was executed: corsairmarks/sample-test-framework@1e30e2b

Output (note how test-suite "TestMethod4_TestCaseAttribute" shows the properties at the suite level, but they do not show for the test-case "TestMethod4_TestCaseAttribute()"):

<?xml version="1.0" encoding="utf-8" standalone="no"?>
<test-run id="2" testcasecount="6" result="Passed" total="6" passed="6" failed="0" inconclusive="0" skipped="0" asserts="6" engine-version="3.2.0.0" clr-version="4.0.30319.42000" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.128479">
  <command-line><![CDATA["C:\Program Files (x86)\NUnit.org\nunit-console\nunit3-console.exe" .\Sample.Test.dll]]></command-line>
  <test-suite type="Assembly" id="0-1010" name="Sample.Test.dll" fullname="C:\Source\git\sample-test-framework\src\Sample.Test\bin\Debug\Sample.Test.dll" runstate="Runnable" testcasecount="6" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.039621" total="6" passed="6" failed="0" inconclusive="0" skipped="0" asserts="6">
    <environment framework-version="3.2.0.0" clr-version="4.0.30319.42000" os-version="Microsoft Windows NT 10.0.10586.0" platform="Win32NT" cwd="C:\Source\git\sample-test-framework\src\Sample.Test\bin\Debug" machine-name="DURANDAL" user="butchern" user-domain="DURANDAL" culture="en-US" uiculture="en-US" os-architecture="x64" />
    <settings>
      <setting name="WorkDirectory" value="C:\Source\git\sample-test-framework\src\Sample.Test\bin\Debug" />
      <setting name="ImageRuntimeVersion" value="4.0.30319" />
      <setting name="ImageTargetFrameworkName" value=".NETFramework,Version=v4.5" />
      <setting name="ImageRequiresX86" value="False" />
      <setting name="ImageRequiresDefaultAppDomainAssemblyResolver" value="False" />
      <setting name="NumberOfTestWorkers" value="8" />
    </settings>
    <properties>
      <property name="_PID" value="26364" />
      <property name="_APPDOMAIN" value="test-domain-" />
    </properties>
    <test-suite type="TestSuite" id="0-1011" name="Sample" fullname="Sample" runstate="Runnable" testcasecount="6" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.029111" total="6" passed="6" failed="0" inconclusive="0" skipped="0" asserts="6">
      <test-suite type="TestSuite" id="0-1012" name="Test" fullname="Sample.Test" runstate="Runnable" testcasecount="6" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.028970" total="6" passed="6" failed="0" inconclusive="0" skipped="0" asserts="6">
        <test-suite type="TestFixture" id="0-1000" name="NUnitAttributeExample" fullname="Sample.Test.NUnitAttributeExample" classname="Sample.Test.NUnitAttributeExample" runstate="Runnable" testcasecount="4" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.024032" total="4" passed="4" failed="0" inconclusive="0" skipped="0" asserts="4">
          <properties>
            <property name="TestOf" value="Sample.Foo" />
          </properties>
          <test-case id="0-1001" name="TestMethod1" fullname="Sample.Test.NUnitAttributeExample.TestMethod1" methodname="TestMethod1" classname="Sample.Test.NUnitAttributeExample" runstate="Runnable" seed="798259755" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.009839" asserts="1">
            <properties>
              <property name="TestIdPropertyKey" value="WorkItem1" />
            </properties>
            <output><![CDATA[Prepare() TestMethod1 (TestMethod1): WorkItem1
Complete()
]]></output>
          </test-case>
          <test-case id="0-1002" name="TestMethod2" fullname="Sample.Test.NUnitAttributeExample.TestMethod2" methodname="TestMethod2" classname="Sample.Test.NUnitAttributeExample" runstate="Runnable" seed="1630670862" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.000263" asserts="1">
            <properties>
              <property name="TestIdPropertyKey" value="WorkItem2" />
            </properties>
            <output><![CDATA[Prepare() TestMethod2 (TestMethod2): WorkItem2
Complete()
]]></output>
          </test-case>
          <test-case id="0-1003" name="TestMethod3_TestAttribute" fullname="Sample.Test.NUnitAttributeExample.TestMethod3_TestAttribute" methodname="TestMethod3_TestAttribute" classname="Sample.Test.NUnitAttributeExample" runstate="Runnable" seed="993609143" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.000279" asserts="1">
            <properties>
              <property name="TestIdPropertyKey" value="WorkItem3" />
            </properties>
            <output><![CDATA[Prepare() TestMethod3_TestAttribute (TestMethod3_TestAttribute): WorkItem3
Complete()
]]></output>
          </test-case>
          <test-suite type="ParameterizedMethod" id="0-1005" name="TestMethod3_TestCaseAttribute" fullname="Sample.Test.NUnitAttributeExample.TestMethod3_TestCaseAttribute" runstate="Runnable" testcasecount="1" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.002440" total="1" passed="1" failed="0" inconclusive="0" skipped="0" asserts="1">
            <properties>
              <property name="TestIdPropertyKey" value="WorkItem3" />
            </properties>
            <test-case id="0-1004" name="TestMethod3_TestCaseAttribute()" fullname="Sample.Test.NUnitAttributeExample.TestMethod3_TestCaseAttribute()" methodname="TestMethod3_TestCaseAttribute" classname="Sample.Test.NUnitAttributeExample" runstate="Runnable" seed="277322916" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.000283" asserts="1">
              <output><![CDATA[Prepare() TestMethod3_TestCaseAttribute() (TestMethod3_TestCaseAttribute): 
Complete()
]]></output>
            </test-case>
          </test-suite>
        </test-suite>
        <test-suite type="TestFixture" id="0-1006" name="NUnitSetUpTearDownExample" fullname="Sample.Test.NUnitSetUpTearDownExample" classname="Sample.Test.NUnitSetUpTearDownExample" runstate="Runnable" testcasecount="2" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.002920" total="2" passed="2" failed="0" inconclusive="0" skipped="0" asserts="2">
          <properties>
            <property name="TestOf" value="Sample.Foo" />
          </properties>
          <test-case id="0-1007" name="TestMethod4_TestAttribute" fullname="Sample.Test.NUnitSetUpTearDownExample.TestMethod4_TestAttribute" methodname="TestMethod4_TestAttribute" classname="Sample.Test.NUnitSetUpTearDownExample" runstate="Runnable" seed="994105559" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.001717" asserts="1">
            <properties>
              <property name="TestIdPropertyKey" value="WorkItem4" />
            </properties>
            <output><![CDATA[Prepare() TestMethod4_TestAttribute (TestMethod4_TestAttribute): WorkItem4
Complete()
]]></output>
          </test-case>
          <test-suite type="ParameterizedMethod" id="0-1009" name="TestMethod4_TestCaseAttribute" fullname="Sample.Test.NUnitSetUpTearDownExample.TestMethod4_TestCaseAttribute" runstate="Runnable" testcasecount="1" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.000477" total="1" passed="1" failed="0" inconclusive="0" skipped="0" asserts="1">
            <properties>
              <property name="TestIdPropertyKey" value="WorkItem4" />
            </properties>
            <test-case id="0-1008" name="TestMethod4_TestCaseAttribute()" fullname="Sample.Test.NUnitSetUpTearDownExample.TestMethod4_TestCaseAttribute()" methodname="TestMethod4_TestCaseAttribute" classname="Sample.Test.NUnitSetUpTearDownExample" runstate="Runnable" seed="1553621128" result="Passed" start-time="2016-03-18 17:42:44Z" end-time="2016-03-18 17:42:44Z" duration="0.000215" asserts="1">
              <output><![CDATA[Prepare() TestMethod4_TestCaseAttribute() (TestMethod4_TestCaseAttribute): 
Complete()
]]></output>
            </test-case>
          </test-suite>
        </test-suite>
      </test-suite>
    </test-suite>
  </test-suite>
</test-run>

[CharliePoole]

First off, could you explain why you don't feel this is the same as #548?

Also, in terms of the approach, note that your code introduces the Properties of the test into DefaultNUnitTestCaseBuilder, which previously didn't know about properties. That's because it's the attributes themselves that are responsible for applying values to tests.

So, if we were to take your approach of "updating" each test with it's parent attributes, we would be more likely to do something like

foreach (TestMethod test in tests)
{
    test.ApplyAttributesToTest(method.MethodInfo);
}

The question is whether this should be in addition to or in place of the existing line of code that precedes your addition. If we keep that line, the each attribute will have to decide how to handle collisions of values. I think most of them already do that, but it will require a lot of testing.

I've given this some thought and it seems to me that this is a fairly broad issue. If we have a bunch of test cases (TestCaseAttribute, TestCaseSourceAttribute) along with other property-related attributes on a method. Are those properties intended to apply to the containing suite or to each of the individual tests?

Currently, we follow the literal approach and apply them to the test suite that corresponds to the method - not to the individual cases that correspond to the method plus its arguments. In many cases, we may be wrong about this choice. So should we go the other way?

I would say... Maybe! I think it's probably a case for examining each individual attribute. However, applying them to the test case will probably break somebody's code. We will look into it further, for sure and I'd be happy for you to keep working with us on it.

[CharliePoole]

In general, all attributes that modify the test work the same way. They implement IApplyToTest, which is the interface called at the point in time when we have created a test and want to apply attributes to it. The difference with test cases is that they are grouped in a suite. It's a question of how the interface is called, which determines what test they apply themselves to.

[corsairmarks]

It ultimately seems to be related to the same code and hierarchical issues raised in #548. I considered this separate because the Google group discussion for #548 seemed to be desiring attributes from a fixture to be inherited by its member tests. In this case, I'm interested in ensuring the inheritance of attributes from a TestCase to all of its parameterized children that are constructed as tests (which is an implementation detail from my view as a consumer). I viewed the difference between a TestFixture and TestCase as significant.

My (potentially flawed) understanding is that a TestAttribute and TestCaseAttribute are both supposed to signal a test execution, they both live inside a fixture, and the significant difference is parameterization. Thus the correct level to add properties for all TestCases using a given method seemed to be on the method itself.

Thanks for the tip on IApplyToTest - I originally took the approach you noted (test.ApplyAttributesToTest(method.MethodInfo)) and all the NUnit tests still passed. However, I was very leery that it could cause a lot of unintended side-effects by applying everything from a TestCase so I opted to propose Property copying only. This does introduce knowledge of property inheritance, which I also dislike, but it was a targeted change with less change of unexpected side-effects.

Your reasoning for not jumping to make the change is strong - it would change existing behavior pretty significantly. What can I do to help test this? Look at the potential impacts of applying IApplyToTest to any descendant tests? Would making the code change you indicate have implications for building a fixture? I spent some time in the code, but it wasn't apparent to me whether the same code would be called building a fixture or a test suite for a TestCase.

Or should this be sidestepped by an approach similar to previous proposals to add a "ParentContext" to TestContext? I saw on your notes for #548 and you seemed to reject this approach because of the nesting implications and tests potentially querying up the chain.

[CharliePoole]

Actually, #548 deals with the ParameterizedMethodSuite that is created when you use a parameterized test case. It's a suite without a fixture that represents the method and all it's individual cases. Issue #796 addresses the issue of properties set on a test fixture.

You're right to say that the two issues are analogous. They need to be solved in the same way to avoid confusing users. The key difference is that properties on a test fixture are available to your tests in the OneTImeSetUp method while those on a ParameterizedMethodSuite are not user-accessible at all, which makes the matter somewhat more difficult for the user!

I don't see this as an entirely simple problem in the general case. Any property that influences how a test is executed should really not be copied to it's children. If the test case builder were to contain the knowledge of which properties are in this category it would be a violation of the SRP. It's the attributes themselves that are supposed to know what they do and how they are applied, not some external builder. On a practical level, this would mean that the builder would have to be modified as new attributes were introduced. Of course, this isn't possible in the case of custom attributes created by the user. So we need a way for attributes to apply to the child tests either instead of or in addition to the test on which they are placed. Probably, this calls for the introduction of a new interface similar to IApplyToTest. In some ways it's a more complex situation, but if it preserves NUnit's capacity to be extended by users it's worth the extra complexity. Plus, it makes the builder simpler!

Since NUnit 3.0 is relatively new, we can probably focus most heavily on the behavior of our existing attributes. Basically, we need to do some design grudge work: listing all the attributes that use IApplyToTest and deciding if it's appropriate to apply that attribute to children when on a fixture or a parameterized test method. In the case of fixtures, we have to consider simple, parameterized and generic fixtures as well.

Example (very easy one): DescriptionAttribute. IMO...

• It should apply to a simple TestFixture
• It should apply to every instance of a parameterized or generic fixture
• It should not apply to the test methods contained in the fixture

Once we have examined all this, we will be able to generalize categories of behavior. Of course, many devs would start out by generalizing. I call this generalizing from no cases. :-)

One way to examine this is to just do it on (e-)paper. Another way, since we are coders after all, would be to add tests for how we think it should be done to ApplyToTestTests.

[corsairmarks]

I'll plan to get started on some unit tests to indicate how I think the various IApplyToTest implementations should (or should not) be inherited. With the code in hand, it should be easy for me to do a little write-up here too.

As for a new interface, I would initially consider something like IApplyToChildTests that inherits from IApplyToTest with a method like ApplyToChildTests(ITest) or ApplyToChildTests(IList<ITest>). I'll think on it as I am going through all the current attributes.

[CharliePoole]

Sounds like a good start with the tests. Even if some currently fail, they will provide a spec for how this should work.

As for interfaces, I've been thinking in the same terms. We'll see what we need.