Skip to content

Add tests for substituted properties that used as argument matchers#367

Merged
dtchepak merged 4 commits intonsubstitute:masterfrom
alexandrnikitin:property-tests
Mar 6, 2018
Merged

Add tests for substituted properties that used as argument matchers#367
dtchepak merged 4 commits intonsubstitute:masterfrom
alexandrnikitin:property-tests

Conversation

@alexandrnikitin
Copy link
Member

@alexandrnikitin alexandrnikitin commented Mar 5, 2018

Adds tests for substituted properties used as argument matchers.

@alexandrnikitin alexandrnikitin changed the title [WIP] Add property tests for RedundantArgumentMatcherException Add tests for substituted properties that used as argument mathchers Mar 6, 2018
@alexandrnikitin
Copy link
Member Author

Following questions, should these tests fail:

        [Test]
        public void Return_result_for_property_argument()
        {
            _something.SomeProperty = 2;
            Arg.Any<int>()
            _something.Echo(_something.SomeProperty);

            Assert.That(_something.Echo(1), Is.EqualTo(""), "First return");
            Assert.That(_something.Echo(2), Is.EqualTo("two"), "Second return");
        }

        [Test]
        public void Return_result_for_property_argument()
        {
            _something.SomeProperty = 2;
            Arg.Any<int>()
            // same but with Returns
            _something.Echo(_something.SomeProperty).Returns("two");

            Assert.That(_something.Echo(1), Is.EqualTo(""), "First return");
            Assert.That(_something.Echo(2), Is.EqualTo("two"), "Second return");
        }

@zvirja
Copy link
Contributor

zvirja commented Mar 6, 2018

Well, Returns() shouldn't matter, as fail is expected to happen during the call dispatch.

I'd say that both the scenarios should fail on the _something.Echo(...) line.

@alexandrnikitin
Copy link
Member Author

Yes, I would expect that. But now it fails for _something.Echo(...) see https://travis-ci.org/nsubstitute/NSubstitute/jobs/349729578#L793 but works for _something.Echo(...).Returns()

@zvirja
Copy link
Contributor

zvirja commented Mar 6, 2018

Ok, probably we should align, because I see a bit different results.
Both the scenarios provided here work fine for me. For both the tests I see that they fail due to redundant specifier - expected behavior.

Now about the reason of the CI failure. The following test failed:

        [Test]
        public void Should_fail_with_redundant_exception_if_more_specifications_than_arguments_scenario_4()
        {
            // This spec will be ignored, however it's good to let user know that test might not work how he expects.
            Arg.Is(42);

            var ex = Assert.Throws<RedundantArgumentMatcherException>(() =>
            {
                _something.Echo(_something.SomeProperty);
            });
            Assert.That(ex.Message, Contains.Substring("42"));
        }

The scenario is a bit tricky. If you add _something.SomeProperty = 2; before Arg.Is(42), the test will pass (i.e. the expected exception will be thrown). The reason why it fails in the current shape is following:

  1. Arg.Is(42) - specifier is pushed to stack.
  2. _something.SomeProperty - property getter is called. Method doesn't have arguments, so queued spec remains. Property returns 0, as value is not configured and the default one is returned.
  3. _something.Echo(..) is invoked with 0 as a parameter value. It consumes the queued Arg.Is(42) and mistakenly match that specifier with the first parameter value.

From NSubstitute perspective there is no way to handle this scenario (currently). Probably, we could find those ways, but it's a matter of a separate discussion.

I'd suggest to transform this test and add _something.SomeProperty = 2; before Arg.Is(42). In this case it will have a chance to fail and the newly added feature will shine 😉

@alexandrnikitin
Copy link
Member Author

@zvirja You are right! Thanks. I fixed the test to reflect the supported case.

@zvirja
Copy link
Contributor

zvirja commented Mar 6, 2018

@alexandrnikitin LGTM ☺️

@dtchepak dtchepak changed the title Add tests for substituted properties that used as argument mathchers Add tests for substituted properties that used as argument matchers Mar 6, 2018
@dtchepak dtchepak merged commit aca4675 into nsubstitute:master Mar 6, 2018
@dtchepak
Copy link
Member

dtchepak commented Mar 6, 2018

Thanks everyone 😄👍

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.

3 participants