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

introduce FixtureLifeCycle #3427

Open
wants to merge 13 commits into
base: master
from
Open

introduce FixtureLifeCycle #3427

wants to merge 13 commits into from

Conversation

@avilv
Copy link

avilv commented Nov 16, 2019

see issue #2574

@dnfclas

This comment has been minimized.

Copy link

dnfclas commented Nov 16, 2019

CLA assistant check
All CLA requirements met.

@avilv avilv marked this pull request as ready for review Nov 16, 2019
Copy link
Member

CharliePoole left a comment

Nice work! I especially like the tests. As you can see, my comments are more from an architectural point of view than a functional one. I suggest you not make a major change until one or two others comment, since this is something that we could disagree about. OTOH, if you like the approach of "atomicizing" the commands and putting all the intelligence in the generation, go to it!

Summary...

  1. Decide on the overall approach
  2. If you keep the logic within the commands, then investigate using the test context rather than the tests themselves to drive that logic. (If that's not clear, I can give more detail)
src/NUnitFramework/testdata/LifeCycleFixture.cs Outdated Show resolved Hide resolved
/// Specify the lifecycle of a Fixture
/// </summary>
[AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
public sealed class FixtureLifeCycleAttribute : PropertyAttribute

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 5, 2019

Member

It seems to me that making this a PropertyAttribute rather than setting the context complicates things more than is necessary. Commands, which are intended to be as independent of the tests as reasonably possible, have to examine the test, sometimes even the parent tests, in order to do different things based on life cycle. In the original design, which has admittedly changed on an adhoc basis as problems arose, all the "intelligence" about tests was incorporated in the point where the command chains were generated, not internally in the commands themselves. Each command was intended to do one thing. What we have here is a new "thing" to do, so maybe we need a new command.

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 5, 2019

Member

To clarify... I'm OK with a PropertyAtribute if the property is interrogated when the command chains are created, to control what commands are in the chain. Less OK with adding another branch inside an existing command. I'd like to hear what others think about this before deciding, however.

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 5, 2019

Member

Alternative to a PropertyAttribute is one that implements IApplyToContext. Then the commands would still have branches, but they would be context-based and would not require interrogating the test that created the command.

This comment has been minimized.

Copy link
@avilv

avilv Dec 14, 2019

Author

this makes sense, i switched to a simple NUnitAttribute + IApplyToContext, but im having trouble with two parts

1)
right now i'm using:

var fixture = TestBuilder.MakeFixture(typeof(LifeCycleFixtureTestCountIsAlwaysOne));
var attr = new FixtureLifeCycleAttribute(LifeCycle.SingleInstance);

and attr.ApplyToTest(fixture); to apply the attribute to the fixture, but now im not sure how i can apply a context attribute to a fixture.. i'm probably missing something since i haven't seen it done anywhere in the test code.

2)
i still have to change the command chain construction, in SimpleWorkItem MakeTestCommand() since the IApplyToContext is already executed and i can identify the LifeCycle attribute. (via the Context property)
but i still need to make the same change (i assume) in CompositeWorkItem's MakeOneTimeSetUpCommand, but the context is not applied there yet.. so i have no way to know if the fixture is marked as an InstancePerTestCase fixture.

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 14, 2019

Member

As you know, IApplyToContext has one method, ApplyToContext. That method modifies the context of the test where it's applied. NUnit's standard command chain call ApplyToContext at the appropriate place, when you have used an attribute that implements the interface.

I assume that your attribute will set a new property in the TestExecutionContext. Once that is set, all the contexts of any descendant test cases will have the property set. That leaves nothing for you to do except to recognize the context in your own commands.

Many of NUnit's own attributes work this way. See, for example, SetCultureAttribute and SingleThreadedAttribute. Our tests for such attributes are all in AppllyToContextTests.

This comment has been minimized.

Copy link
@avilv

avilv Dec 14, 2019

Author

right now there's code in in CompositeWorkItem MakeOneTimeSetUpCommand() that adds ConstructFixtureCommand to the chain:

private TestCommand MakeOneTimeSetUpCommand(List<SetUpTearDownItem> setUpTearDown, List<TestActionItem> actions)
{
            TestCommand command = new EmptyTestCommand(Test);

            // Add Action Commands
            int index = actions.Count;
            while (--index >= 0)
                command = new BeforeTestActionCommand(command, actions[index]);

            if (Test.TypeInfo != null)
            {
                // Build the OneTimeSetUpCommands
                foreach (SetUpTearDownItem item in setUpTearDown)
                    command = new OneTimeSetUpCommand(command, item);

                // Construct the fixture if necessary
                if (!Test.TypeInfo.IsStaticClass)
                    command = new ConstructFixtureCommand(command); 
            }

            // Prefix with any IApplyToContext items from attributes
            foreach (var attr in Test.GetCustomAttributes<IApplyToContext>(true))
                command = new ApplyChangesToContextCommand(command, attr); 

            return command;
}

this is called from PerformWork() InitializeSetUpAndTearDownCommands

        protected override void PerformWork()
        {
            if (!CheckForCancellation())
                if (Test.RunState == RunState.Explicit && !Filter.IsExplicitMatch(Test))
                    SkipFixture(ResultState.Explicit, GetSkipReason(), null);
                else
                    switch (Test.RunState)
                    {
                        default:
                        case RunState.Runnable:
                        case RunState.Explicit:
                            // Assume success, since the result will otherwise
                            // default to inconclusive.
                            Result.SetResult(ResultState.Success);

                            if (Children.Count > 0)
                            {
                                InitializeSetUpAndTearDownCommands();

                                PerformOneTimeSetUp();

all the IApplyToContext's attributes ApplyToContext are being executed by PerformOneTimeSetUp which happens after InitializeSetUpAndTearDownCommands
so anywhere before PerformOneTimeSetUp(), Context.LifeCycle (my new property - added to TestExecutionContext) is not applied yet - so i cant use it to change the command chain

This comment has been minimized.

Copy link
@rprouse

rprouse Dec 16, 2019

Member

@CharliePoole, you've been following this PR more closely than I have. Do you think that the new instance per fixture command should be created in the MakeOneTimeSetUpCommand rather than down in the SimpleWorkItem?

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 27, 2019

Member

@rprouse - back to looking at this now that Christmas is over!

I think it kind of depends on exactly what instance-per-test-case means to us. Thinking about it just now, I realize we have not stated whether any of the following should happen just once or once per instance...

  • OneTimeSetUp and OneTimeTearDown
  • Before or After actions specified on the class
  • ApplyToContext attributes specified on the class

If only construction is to be per-instance, then I'd skip adding the ConstructFixtureCommand in the above code.

Bui if all of the above are to be per-instnce, I'd just change when MakeOneTimeSetUpCommand gets called in the first place.

To put the question another way... is instance-per-fixture intended to work as if we had selected the tests, one at a time, and run them separately? That's the second approach and FWIW it's what makes sense to me.

This comment has been minimized.

Copy link
@avilv

avilv Dec 27, 2019

Author

this.. is a good question, i agree on the second approach, as soon as we come to an agreement ill add the test cases and implementation for review if needed.

@@ -37,20 +38,40 @@ public class ConstructFixtureCommand : BeforeTestCommand
public ConstructFixtureCommand(TestCommand innerCommand)
: base(innerCommand)
{
Guard.ArgumentValid(Test is TestSuite, "ConstructFixtureCommand must reference a TestSuite", nameof(innerCommand));

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 5, 2019

Member

This line is what triggered my comments above... ConstructFixtureCommand was designed to work on suites, represented by a CompositeWorkItem. That's in part reflected in the fact that it's a BeforeTestCommand rather than a BeforeAndAfterTestCommand. Repurposing it to work on both suites and test cases introduces a bit more complexity.

I recognize that it's a bit hard to see that we have two different sorts of commands, since they all work against Tests and dynamically determine if the test is a suite or not. That's because of the history... we first tried to make one type of command work in both environments, but it turned out that CompositeWorkItems needed two chains of commands, whereas SimpleWorkItems only need one. It was my hope to come back and refactor this at some point, but it never happened. 😞

@CharliePoole CharliePoole requested a review from jnm2 Dec 5, 2019
Copy link
Member

rprouse left a comment

@mikkelbu I'd appreciate your opinions and guidance on this PR if you have time. @CharliePoole is doing a good job guiding this, but it is a big change, so we should have more eyes on it if possible.

/// Specify the lifecycle of a Fixture
/// </summary>
[AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
public sealed class FixtureLifeCycleAttribute : PropertyAttribute

This comment has been minimized.

Copy link
@rprouse

rprouse Dec 16, 2019

Member

@CharliePoole, you've been following this PR more closely than I have. Do you think that the new instance per fixture command should be created in the MakeOneTimeSetUpCommand rather than down in the SimpleWorkItem?

@CharliePoole

This comment has been minimized.

Copy link
Member

CharliePoole commented Dec 16, 2019

@avilv @rprouse I'm in the middle of some other work, but I'll come back again on this tomorrow. I think I want to take a step back and look at the entire flow of control through the command chains.

as added 3 commits Dec 20, 2019
as
as
…structFixturePerTestCaseCommand
@avilv

This comment has been minimized.

Copy link
Author

avilv commented Dec 20, 2019

hey @CharliePoole @rprouse , i cleaned up some of the minor issues and separated the logic to a ConstructFixturePerTestCaseCommand command - until i get some feedback about a way of implementing this via an IApplyContext approach

aviel
@avilv avilv requested a review from CharliePoole Dec 23, 2019
Count++;
Assert.AreEqual(1, Count);
}

This comment has been minimized.

Copy link
@jnm2

jnm2 Dec 27, 2019

Contributor

@rprouse What do you think about adding a warning with a Ctrl+. fix when there is a blank line before a closing brace or after an opening brace?

src/NUnitFramework/testdata/LifeCycleFixture.cs Outdated Show resolved Hide resolved
src/NUnitFramework/testdata/LifeCycleFixture.cs Outdated Show resolved Hide resolved
@avilv

This comment has been minimized.

Copy link
Author

avilv commented Dec 27, 2019

@jnm2 - done, i went through the code again to make sure i didn't miss anything.

Copy link
Member

CharliePoole left a comment

I've made this a comment review, but I don't think we should merge until the question of what exactly a per-instance test case is supposed to do per-instance... just construction? construction plus onetime setup? More?

/// <summary>
/// ConstructFixtureCommand constructs the user test object if necessary.
/// </summary>
public class ConstructFixturePerTestCaseCommand : BeforeTestCommand

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 27, 2019

Member

Agree in the longer term, but it could be confusing to have one command that's internal while others are public. If I were doing it, I'd make the change in a series of PRs scoped to about the same size as this... i.e. all the commands. I'd favor having team members simply commit such a change once the tests passed. Just my 2 cents. 😄

/// Specify the lifecycle of a Fixture
/// </summary>
[AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
public sealed class FixtureLifeCycleAttribute : PropertyAttribute

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 27, 2019

Member

@rprouse - back to looking at this now that Christmas is over!

I think it kind of depends on exactly what instance-per-test-case means to us. Thinking about it just now, I realize we have not stated whether any of the following should happen just once or once per instance...

  • OneTimeSetUp and OneTimeTearDown
  • Before or After actions specified on the class
  • ApplyToContext attributes specified on the class

If only construction is to be per-instance, then I'd skip adding the ConstructFixtureCommand in the above code.

Bui if all of the above are to be per-instnce, I'd just change when MakeOneTimeSetUpCommand gets called in the first place.

To put the question another way... is instance-per-fixture intended to work as if we had selected the tests, one at a time, and run them separately? That's the second approach and FWIW it's what makes sense to me.

/// <summary>
/// Overridden to set a TestFixture's <see cref="LifeCycle"/>.
/// </summary>
public override void ApplyToTest(Test test)

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 27, 2019

Member

So does the test get a C# LifeCycle property as well as an NUnit Property named FixtureLifeCycle, based on the inheritance from PropertyAttribute? I can see that the life cycle is sufficiently important to make it a C# property on the fixture, but in that case why use a PropertyAttribute, which creates the other kind of property?

This comment has been minimized.

Copy link
@avilv

avilv Dec 27, 2019

Author

my bad, i was using ParallelizeAttribute as an example for my changes at first - so PropertyAttribute looked like the right choice then, not anymore now :)
i changed it to NUnitAttribute, IApplyToTest with my latest commit


Guard.ArgumentValid(testSuite != null, "ConstructFixturePerTestCaseCommand must reference a TestSuite", nameof(innerCommand));

BeforeTest = (context) =>

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 27, 2019

Member

@avilv Is there some reason that you didn't use a BeforeAndAfter command? That way, the cleanup of the fixture instance could be handled automatically without needing a separate command to do it.

This comment has been minimized.

Copy link
@avilv

avilv Dec 27, 2019

Author

i thought about it but, but i figured since the convention right now is to have a construct and a dispose command i should try and stick to it.. i don't mind changing it though

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 27, 2019

Member

That's the architecture for commands that relate to classes, because classes have two different command sequences, one for before and one for after.

But for commands that relate to methods, we generally use beforeandafter commands when there are symmetric things to do because there is only one command sequence constructed. I think this is a bit confusing because we are constructing a class as part of the execution of a method.

This comment has been minimized.

Copy link
@avilv

avilv Dec 27, 2019

Author

ah.. that makes sense, so say i add the dispose logic in AfterTest, now that its doing two things should it still be called ConstructFixturePerTestCaseCommand ?
or perhaps HandleFixturePerTestCaseCommand

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 27, 2019

Member

Or just FixturePerTestCaseCommand?

Anyway, the advantage of Before + After is that the system keeps track for you. After doesn't run unless before ran. If you use two commands, you're supposed to do that yourself.

This comment has been minimized.

Copy link
@avilv

avilv Dec 27, 2019

Author

done

…PerTestCaseComamnd
Copy link
Member

CharliePoole left a comment

See my inline comment regarding OneTimeSetUp. Everything else looks great now and I'd be ready to approve once that problem is fixed.

Thanks for your perserverance.

@@ -243,7 +243,8 @@ private TestCommand MakeOneTimeTearDownCommand(List<SetUpTearDownItem> setUpTear
command = new OneTimeTearDownCommand(command, item);

// Dispose of fixture if necessary
if (Test is IDisposableFixture && typeof(IDisposable).IsAssignableFrom(Test.TypeInfo.Type))
if (Test is IDisposableFixture && typeof(IDisposable).IsAssignableFrom(Test.TypeInfo.Type) &&
(Test as TestFixture)?.LifeCycle != LifeCycle.InstancePerTestCase)
command = new DisposeFixtureCommand(command);

This comment has been minimized.

Copy link
@CharliePoole

CharliePoole Dec 28, 2019

Member

Here's a problem I didn't see at first. Let's say the fixture is LifeCycle.InstancePerTestCase. In that case, we won't construct it here. Now, let's say that the OneTimeSetUp is an instance method that sets some property in the fixture. It's going to throw an NRE.

So, it's really not a matter of choice whether we run OneTimeSetUp and OneTimeTearDown for each instance. It has to be done that way... unless there is something I'm missing.

I suggest adding a test case that does exactly that. Let it set some property of the fixture to a value. You should see the crash. Then you should fix it. 😈

This comment has been minimized.

Copy link
@avilv

avilv Dec 28, 2019

Author

i would really like to expand the test coverage on this
right now i'm basically hard-coding the LifeCycle flag to InstancePerTestCase then running the entire suite to check for bugs in this mode
since this affects everything, it needs to be tested, well - everywhere
i need to find an approach of testing this without duplicating every test code that was written
any ideas? like a global way to run all the tests with single then instance-per-test-case
what i'm going to do right now is simply find all the tests that fail and shouldn't with the InstancePerTestCase life cycle flag and add them to LifeCycleAttributeTests (basically duplicating the code.. )

but i know that's not ideal

This comment has been minimized.

Copy link
@avilv

avilv Jan 10, 2020

Author

so i just had some free time and i pushed the latest change-set , i added more test cases went through the entire suite (with instance-per-test-case forced on) and made sure it makes sense on all test cases.

the only thing i would like to suggest is.. maybe we should throw an exception when InstancePerTestCase is used with an instance OneTimeSetup/TearDown method - this can lead to unexpected results since the instance is no longer shared

what do you think ?

as
@avilv avilv requested a review from CharliePoole Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.