v4.2.1: Received.InOrder ignores .Returns()

[Reris]

Describe the bug Received.InOrder ignores/bypasses any .Returns() configured call. All calls inside return their default result. This leads to failing Received tests, as the expected result isn't called.

This bug relates to 4.2.1 4.2.0 works as expected.

To Reproduce

    public class Operation_Tests
    {
        public Operation_Tests()
        {
            this._connection = Substitute.For<IDbConnection>();
            this._connector = Substitute.For<IConnector>();
            this._connector.OpenAsync().Returns(ci => this._connection);
        }

        private readonly IConnector _connector;
        private readonly IDbConnection _connection;

        public interface IConnector
        {
            Task<IDbConnection> OpenAsync();
        }

        private Operation CreateTestee()
        {
            return new Operation(this._connector);
        }

        private class Operation
        {
            private readonly IConnector _connector;

            public Operation(IConnector connector)
            {
                this._connector = connector;
            }

            public async Task ExecuteAsync(Action action)
            {
                using (var connection = await this._connector.OpenAsync())
                {
                    using (var transaction = connection.BeginTransaction())
                    {
                        action();
                        transaction.Commit();
                    }
                }
            }
        }

        [Fact]
        public async Task ExecuteAsync__ShouldCallInOrder()
        {
            //////////////////
            // Works in 4.2.0
            // Not   in 4.2.1
            //////////////////

            // Arrange
            var testee = this.CreateTestee();
            var act = Substitute.For<Action>();

            // Act
            await testee.ExecuteAsync(act);

            // Assert
            Received.InOrder(
                async () =>
                {
                    using (var connection = await this._connector.OpenAsync())
                    {
                        using (var transaction = connection.BeginTransaction())
                        {
                            act();
                            transaction.Commit();
                        }
                    }
                });
        }

        [Fact]
        public async Task ExecuteAsync_Alternative_ShouldCallInOrder()
        {
            // Arrange
            var testee = this.CreateTestee();
            var act = Substitute.For<Action>();

            this._connector.ClearSubstitute(); // remove .Returns()
            var connection = await this._connector.OpenAsync();
            var transaction = connection.BeginTransaction();
            this._connector.ClearReceivedCalls();
            connection.ClearReceivedCalls();

            // Act
            await testee.ExecuteAsync(act);

            // Assert
            Received.InOrder(
                async () =>
                {
                    using (await this._connector.OpenAsync())
                    {
                        using (connection.BeginTransaction())
                        {
                            act();
                            transaction.Commit();
                        }
                    }
                });
        }
    }

Expected behaviour Both tests work. Well... the second would not be preferrable :)

Environment:

• NSubstitute version: 4.2.1
• Platform: dotnetcore2.0 project on Windows 10

[dtchepak]

Hi @Reris, thanks a lot for raising this and for the excellent repro. Sorry for the breaking change and for the inconvenience caused!

@zvirja do you have any suggestions on how to handle this break? Re-release with version rev? Rollback? Keep release and update release notes/breaking changes? Longer term do we want to keep the 4.2.1 behaviour for future releases?

---

@Reris: bit off topic, but I have a suggestion for the test that I noticed when going through the repo steps.

We already know that _connector.OpenAsync() is called before connection.BeginTransaction() because without the former call we can't get the connection reference for the latter, so we don't need to assert on this. The test then becomes a bit simpler and no longer echoes the production code. Having test code match one-to-one with production can sometimes be a problem as we're just checking the code is exactly the same, rather than it behaves a certain way. If we calculate the result/assert in a different way in test than production code then it can gives us a bit more confidence our code is correct. A bit like checking basic mental arithmetic by checking a + b = r by working out r - b = a, rather than just repeating the a + b = r calculation in which we could make the same mistake twice.

[Fact]
public async Task ExecuteAsync__ShouldCallInOrder() {
    // Arrange
    var testee = this.CreateTestee();
    var act = Substitute.For<Action>();
    var transaction = _connection.BeginTransaction(); // ...or include this in test setup.

    // Act
    await testee.ExecuteAsync(act);

    // Assert
    Received.InOrder(async () => {
        act();
        transaction.Commit();
        transaction.Dispose();
        _connection.Dispose();
    });
}

Edit: just noticed that doesn't ensure the transaction is created before act() is invoked. This idea would probably work better if we had act(transaction) so we've enforced that transaction is created beforehand, but that may have negative side-effects like allowing act to commit or rollback transactions. We can add _connection.BeginTransaction() to the InOrder assertion before act(), but we'd need to explicitly setup the transaction with _transaction = _connection.BeginTransaction() in Operation_Tests() then (or clear calls) to avoid that counting as a call to the code under test. Hope this rambling has given some useful ideas though. 😄

[Reris]

@dtchepak Thanks for your suggestion. Of course you're right in your statement that as test shouldn't be a copy of the tested code. The act via parameter was only intended to be a placeholder for any other real acting scenario. So the whole code is just a simple replayable example which could help to fix this bug :)

A further impact in a more complex scenario inluding constructor injected substitutions would be the usage of another function result/property getter, which would ignore any previously defined results only during the replay. So I think it's clearly an unintended bug, which forces a developer to write less readable test code.

[dtchepak]

@Reris This was adjusted in 4.2.1 to fix https://github.com/nsubstitute/NSubstitute/issues/569 and to match standard .Received() behaviour (which also does not run .Returns). We'll have to work out which behaviour is preferable. Your example really helps here, thank you!

[zvirja]

Hi guys!

First of all I join David's apology for the caused inconveniences - I know that feeling when you update the dependency and all of a sudden passing tests start to fail due to changes in the testing library. It indeed sucks 😟

Regarding the change itself, as it has been mentioned above, it was a fix for another issue. If we roll back the change, the other issue will re-occur. It's impossible to fix both the issues altogether, so it's a choice between two possible behaviors.

From my experience, the Received.InOrder() check often leads to unintended assertions, simply because it's so easy to miss that any call you invoke inside the scope is in fact an assertion. That makes tests more fragile as it's an instantiation of the Structural inspection anti-pattern.

I reviewed the scenario you provided and in fact you are asserting that 5(!!!) different calls were invoked in the particular order:

    1@Operation_Tests+IConnector.OpenAsync()
    [email protected]()
    [email protected]()
    [email protected]()
    [email protected]()
    [email protected]()

I don't know logic of your code base too much, but it worth reviewing whether you indeed need to assert all 5 calls, or it's enough to assert only few of them (e.g. transaction only).

The change we applied indeed breaks the chains and makes it impossible to write statements like A.B.C(). Instead, it fosters you to assert only calls which you indeed want to verify. It makes your tests more readable, more robust to the refactoring and less messy.

A further impact in a more complex scenario inluding constructor injected substitutions would be the usage of another function result/property getter, which would ignore any previously defined results only during the replay.

That's a perfect demonstration of the concern I've mentioned above. Any call under the Received.InOrder() scope is asserted, which means that you also verify (most likely unintentionally) that properties/intermediate calls were invoked in the particular order. Is that indeed what you intend to verify in your test?

---

I'm very strong in my position that we should preserve the fix and I insist on the existing behavior. It forces people to write more robust and transparent tests. I deeply regret that it failed the existing code base, but believe that the code changes people apply will bring benefits to them.

@dtchepak Speaking of our actions, I suggest us to watch and see whether there are more people suffering from it. If they are - we could update our release notes to indicate that the change might lead to potentially broken tests. Otherwise, if that's the only case - we could keep it as-is.

[dtchepak]

@zvirja Thanks for posting your analysis of this. 👍

I think I should update the release notes and breaking changes to list this, as we now can confirm it has broken tests for people (in retrospect, I think it should have been minor version rev at least).

I rarely, if ever, use Received.InOrder so I have trouble commenting on what the behaviour should be, and am happy to go with your advice on keeping this behaviour. If we are going to keep this behaviour though I will update the Checking call order documentation and xmldoc to mention this design choice. It is unfortunate that we don't have a reliable way of determining whether #569 or this #582 is going to cause more troubles for people.

[zvirja]

(in retrospect, I think it should have been minor version rev at least).

Well, SemVer is about backward and forward API compatibility. From API perspective the change is both backward and forward compatible, so it's a patch number change. So to me for the cases like this we should either change major or patch version.

It is unfortunate that we don't have a reliable way of determining whether #569 or this #582 is going to cause more troubles for people.

We could also look at this from the aligning perspective. Current fix makes the library consistent in its behavior, which adds value.

I think I should update the release notes and breaking changes to list this If we are going to keep this behaviour though I will update the Checking call order documentation and xmldoc to mention this design choice.

Agree, would be awesome to do it! 👍

[Reris]

Okay, agreed. Its probably the best way to make a hard cut in Received.InOrder() to prevent code mimic scenarios. To go a step further, I would suggest to enforce this behaviour: Methods called in Received.InOrder() return their default(T) value instead of their default substitute. This would make a clear statement in the tests above where keywords like using and await hide eventual tail calls.

Example:

// fails with nullref
Received.InOrder(
async () =>
{
    await this._connector.OpenAsync();
});

// fails with nullref
Received.InOrder(
() =>
{
    connection.BeginTransaction().Dispose();
});

// Still okay
await this._connector.Received().OpenAsync();
await this._connector.Received().OpenConnectionStringAsync(Arg.Any<string>());

This would make a clear distinction between stacked/scoped execution and bit-by-bit Received.InOrder() and expose some false positives. What do you think? And would this be possible? 😃

[fteotini]

I've a piece of code that works like StringBuilder so it's imperative to assert the order of calls. Is there a way to do this?

[dtchepak]

Hi @Fed03 , do you have an example you can share for this? Received.InOrder generally works ok, there are just some edge cases like the one in this issue.

[fteotini]

@dtchepak sure, you are right. I didn't explain myself XD Here's a very simple example

class SomeBuilder {
    SomeBuilder AppendLine(string content)
}

//somewhere in code
builder
    .AppendLine("First line")
    .AppendLine("Second line")
    .AppendLine("Third line");


// in tests
_builderMock = Substitute.For<SomeBuilder>();
_builderMock.ReturnsForAll<SomeBuilder>(_builderMock)

Received.InOrder(() => {
    _builderMock.Received().AppendLine("First line");
    _builderMock.Received().AppendLine("Second line");
    _builderMock.Received().AppendLine("Third line");
});

[dtchepak]

Thanks @Fed03 . First, make sure AppendLine is virtual. Then you should be able to do:

Received.InOrder(() => {
    _builderMock.AppendLine("First line");
    _builderMock.AppendLine("Second line");
    _builderMock.AppendLine("Third line");
});

The .Received() doesn't get used here as the calls are already in the Received.InOrder(...) block.