Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply attributes recursively for nested classes #3889

Merged
merged 3 commits into from
Oct 1, 2021

Conversation

mvdfugro
Copy link
Contributor

This resolves #3888 by applying all attributes recursively for nested classes.

Although this is a nice solution in terms of consistency, it also has a somewhat higher risk of breaking backwards compatibility (in all test cases in the world there is probably someone depending on the non-inheritance of attributes). So maybe this is an OK solution for 4.0+ but not for backporting to 3.x. An interesting example of this is the Ignore attribute:

    [TestFixture]
    [Ignore("testing ignore a fixture")]
    public class FixtureUsingIgnoreAttribute
    {
        [Test]
        public void Success()
        {
        }

        public class SubFixture
        {
            [Test]
            public void Success()
            {
                // this is currently a test that runs, but gets ignored with this change
            }
        }
    }

A safer (but less consistent) solution would filter the recursive application for specific attributes (maybe based on an explicit interface?)

Note that branch is 'on top of' #3843 as it depends (somewhat) on the new test system added there. The framework changes should apply to master without any changes.

@rprouse
Copy link
Member

rprouse commented Jul 15, 2021

It looks like this has the changes from your other PR mixed up in it. I am going to hold off reviewing until that is merged.

@mvdfugro
Copy link
Contributor Author

Indeed this is based on the other PR (as otherwise I had no failing tests to fix ;-)). I'll rebase this on master once the other request is merged back there.

This helps to clarify that this has no effect on 'inheritance'
of the lifecycle (this is determined while applying the attributes,
not at runtime).
@mvdfugro
Copy link
Contributor Author

Rebased on latest master. Now the review only contains the three relevant commits :-)

Fundamentally, two changes have been made:

  • Attributes are now applied recursively (and I've also added a test for the Ignore attribute),
  • HasLifeCycle has been simplified to just use the first TestFixture found. I believe this is what the actual behavior already was (as there only is a single TestFixture in the ITest.Parent hierarchy) but this more clearly communicates that this is not where the inheritance happens. See the discussion on FixtureLifeCycle not applied correctly to nested test classes #3888

Copy link
Member

@rprouse rprouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rprouse rprouse merged commit 40dcb26 into nunit:master Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FixtureLifeCycle not applied correctly to nested test classes
2 participants