SetUp/TearDown not called recursively for PropertyAttribute

[simendsjo]

When using PropertyAttribute, I would expect all SetUpAttribute and TearDownAttribute to be run as would be the case for TestAttribute.

We have an attribute with a similar use-case, and does the following:

                    // We need to manually clean up the test before trying again.
                    // I was unable to reuse existing NUnit functionality for this.
                    //
                    // See also NUnits SimpleWorkItem and RetryCommand
                    TestFixture fixture;
                    {
                        var p = context.CurrentTest.Parent;
                        while (!(p is TestFixture)) p = p.Parent;
                        fixture = (TestFixture) p;
                    }

                    var teardownMethods = fixture.TearDownMethods.OrderByDescending(GetTypeDepth);
                    foreach (var m in teardownMethods)
                    {
                        try
                        {
                            m.Invoke(context.TestObject, new object[] { });
                        }
                        catch
                        {
                            // Has teardown already completed?
                            // We just ignore all errors and assume a full setup will solve all problems
                        }
                    }

                    var setupMethods = fixture.SetUpMethods.OrderBy(GetTypeDepth);
                    foreach (var m in setupMethods)
                    {
                        m.Invoke(context.TestObject, new object[] { });
                    }

where

        private static int GetTypeDepth(MethodInfo x)
        {
            var d = 0;
            for (var ty = x.DeclaringType; ty != null; ty = ty.BaseType)
            {
                ++d;
            }

            return d;
        }

[kurtschelfthout]

Ah, it looks like you almost have a pull request, hint hint.

Is the crux here the recursive part? We already seem to be calling setup and teardown here: https://github.com/fscheck/FsCheck/blob/master/src/FsCheck.NUnit/FsCheckPropertyAttribute.fs#L119

Also do you have an example that doesn't work? I'm just trying to understand what recursive really means in this context...I'm not super-familiar with NUnit.

[simendsjo]

Ah, it looks like you almost have a pull request, hint hint.

:) I might have time to look at this earlier next week

Is the crux here the recursive part?

Yes. Each class in the hierarchy is allowed to have SetUp and TearDown methods, which NUnit calls correctly. I looked at the NUnit implementation, but it seemed overly complex for what I needed, which is why I opted for the simple "depth" based implementation. But note that I have the case where it seems like TearDown is already called -- I don't know enough about NUnit internals to really say what goes on here, but the code I pasted works for us; ~800 tests that might run this code (a retry on deadlock mechanism).

Also do you have an example that doesn't work?

Some pseudocode

class Parent {
  int setupParent;
  int teardownParent;

  [SetUp] void SetupParent() { ++setupParent; }
  [TearDown] void TeardownParent() { ++teardownParent; }
}

class Child : Parent {
  int setupChild;
  int teardownChild;

  [SetUp] void SetupChild() { ++setupChild; }
  [TearDown] void TeardownChild() { ++teardownChild; }
}

With the naive implementation, this we get (completely unverified)

  setupParent = 0
  teardownParent = 0
  setupChild = 1
  teardownChild = 1

So we need to call SetUp starting at the lowest level, Parent, and TearDown starting at the top, Child.

Not sure why people haven't noticed, but people might be using virtual/override instead -- we used this ourselves until it became a problem when trying to restart the tests.

[kurtschelfthout]

That clarifies things, thank you. A few more things:

but people might be using virtual/override instead -- we used this ourselves until it became a problem when trying to restart the tests.

If you're careful about when you call base (at the beginning in setup and at the end in teardown) doesn't that work?

Also, wouldn't the code have to check whether the setup/teardown isn't already virtual, otherwise we may be calling the parent's setup/teardown more than once?

[simendsjo]

If you're careful about when you call base (at the beginning in setup and at the end in teardown) doesn't that work?
Also, wouldn't the code have to check whether the setup/teardown isn't already virtual, otherwise we may be calling the parent's setup/teardown more than once?

I think our problem was it was being called twice (without double checking this)

class Base {
  [SetUp] virtual void Setup() {}
}
class Child : Base {
  [SetUp] override void Setup() { base.Setup(); }
}

I think having [SetUp] twice would case it to be called twice. Not sure about this though. I also found a method hiding when converting the tests, so it might have been that too.

I should probably experiment to see exactly what went on here -- I just got a bit eager to get it working that I looked an the NUnit implementation and found out we're probably using it "wrong".