10
\$\begingroup\$

I have the following unit test that is testing predicate logic in a repository (Note: mockOpportunityRepository.Setup):

[Test]
public void GetFollowedByUserIdShouldGetTheUsersFollowedOpportunitiesFromOpportunityFollowRepositoryAndThenGetThoseFollowedOpportunities()
{
    const int ExpectedOpportunityId = 9;

    var testOpportunities = new List<Domain.EF.Opportunity>()
    {
        OpportunityBuilder.Default().WithId(1).Build(),
        OpportunityBuilder.Default().WithId(2).Build(),
        OpportunityBuilder.Default().WithId(ExpectedOpportunityId).Build(),
    };

    var actualOpportunities = new List<Opportunity>();

    var expectedOpportunityFollows = new List<OpportunityFollow> { new OpportunityFollow { OpportunityFollowId = 1, OpportunityId = ExpectedOpportunityId, UserId = UserId } };
    mockOpportunityFollowRepository.Setup(x => x.GetByUserId(UserId)).ReturnsAsync(expectedOpportunityFollows);

    mockOpportunityRepository.Setup(
        s =>
            s.GetAsync(
                It.IsAny<Expression<Func<Domain.EF.Opportunity, bool>>>(),
                It.IsAny<Func<IQueryable<Domain.EF.Opportunity>, IOrderedQueryable<Domain.EF.Opportunity>>>(),
                It.IsAny<int?>(),
                It.IsAny<int>()))
        .Callback<
            Expression<Func<Domain.EF.Opportunity, bool>>,
            Func<IQueryable<Domain.EF.Opportunity>,
            IOrderedQueryable<Domain.EF.Opportunity>>,
            int?,
            int>((queryParam, sortParam, takeParam, skipParam) =>
            {
                var func = queryParam.Compile();
                actualOpportunities = testOpportunities.Where(func).ToList();
            });

    var results = sut.GetFollowedByUserId(UserId);

    mockOpportunityFollowRepository.Verify(x => x.GetByUserId(UserId), Times.Once);
    actualOpportunities.Count.Should().Equals(1);
    actualOpportunities[0].OpportunityId.Should().Equals(ExpectedOpportunityId);
}

I have several tests like this all utilising the same bulky mockOpportunityRepository.Setup and wanted to refactor it into one re-useable place. Essentially the setup has a callback which runs an anonymous function. The anon function compiles the query parameters and executes them against a test list of opportunities. The outer test function needs the result of this filter to Assert the predicate logic.

If I try to move the mockOpportunityRepository.Setup into a separate function and try to output the actualOpportunities by reference I get a compiler error that ref and out parameters are not allowed in anonymous method body. I'm not very knowledgeable on functions and am sure it can be done.

Can anyone refactor this and suggest a way of moving the mockOpportunityRepository.Setup out of the test to make it reusable yet somehow get the results of the anonymous function (actualOpportunities) back to the calling test?

\$\endgroup\$
  • 4
    \$\begingroup\$ It really feels like you're testing the implementation instead of the result. I understand that is usually the case when testing repository implementations, but I can't help but feel like this would be more straight forward to test by creating & seeding a test database for integration testing. Just a thought. \$\endgroup\$ – RubberDuck Sep 4 '16 at 14:50
  • \$\begingroup\$ I'm finished on the project now, but In retrospect I agree. Perhaps an integration test would have better suited this. It should be easier to test repository logic however \$\endgroup\$ – sarin Sep 4 '16 at 20:40
  • 5
    \$\begingroup\$ Upvoted simply for that method name. \$\endgroup\$ – Der Kommissar Apr 1 '17 at 20:52
1
\$\begingroup\$

I think, for what it's worth, you have mostly only succeeded in testing your mocks here. Even if you moved these mocks out of the test, I think you'd still only be testing your mock implementation and not much production code. So these tests can pass all day long without reflecting the condition of your production implementations. (If I'm understanding every properly :)

[Test]
public void GetFollowedByUserIdShouldGetTheUsersFollowedOpportunitiesFromOpportunityFollowRepositoryAndThenGetThoseFollowedOpportunities()
{
    const int ExpectedOpportunityId = 9;

    var testOpportunities = new List<Domain.EF.Opportunity>()
    {
        OpportunityBuilder.Default().WithId(1).Build(),
        OpportunityBuilder.Default().WithId(2).Build(),
        OpportunityBuilder.Default().WithId(ExpectedOpportunityId).Build(),
    };

    var actualOpportunities = new List<Opportunity>();

    var expectedOpportunityFollows = new List<OpportunityFollow> { new OpportunityFollow { OpportunityFollowId = 1, OpportunityId = ExpectedOpportunityId, UserId = UserId } };
    mockOpportunityFollowRepository.Setup(x => x.GetByUserId(UserId)).ReturnsAsync(expectedOpportunityFollows);

    mockOpportunityRepository.Setup(
        s =>
            s.GetAsync(
                It.IsAny<Expression<Func<Domain.EF.Opportunity, bool>>>(),
                It.IsAny<Func<IQueryable<Domain.EF.Opportunity>, IOrderedQueryable<Domain.EF.Opportunity>>>(),
                It.IsAny<int?>(),
                It.IsAny<int>()))
        .Callback<
            Expression<Func<Domain.EF.Opportunity, bool>>,
            Func<IQueryable<Domain.EF.Opportunity>,
            IOrderedQueryable<Domain.EF.Opportunity>>,
            int?,
            int>((queryParam, sortParam, takeParam, skipParam) =>
            {
                var func = queryParam.Compile();
                actualOpportunities = testOpportunities.Where(func).ToList();
            });

    // NOTE: not sure which guy your sut variable is here.  
    // NOTE: no assertions on the results is suspect.
    var results = sut.GetFollowedByUserId(UserId);

    // NOTE: this is testing the plumbing of a function.  that my thing called another thing.  in general, 
    //       i have found this kind of testing to be less valuable and brittle.  if the function
    //       can do the job without calling the other thing in the future, this test will break,
    //       even though the function works properly. ymmv.
    mockOpportunityFollowRepository.Verify(x => x.GetByUserId(UserId), Times.Once);

    // NOTE: unfortunately, all thats tested here is that the mock implementation returns
    //       the right result.  no production code is exercised for these.
    actualOpportunities.Count.Should().Equals(1);
    actualOpportunities[0].OpportunityId.Should().Equals(ExpectedOpportunityId);
}

For all the mocking etc... I believe the above code is functionally equivalent as what I have below (or close to it). And almost all of it is still mock implementation. What production code we are testing is hard to know.

[Test]
public void GetFollowedByUserIdShouldGetTheUsersFollowedOpportunitiesFromOpportunityFollowRepositoryAndThenGetThoseFollowedOpportunities()
{
    const int ExpectedOpportunityId = 9;

    var testOpportunities = new List<Domain.EF.Opportunity>()
    {
        OpportunityBuilder.Default().WithId(1).Build(),
        OpportunityBuilder.Default().WithId(2).Build(),
        OpportunityBuilder.Default().WithId(ExpectedOpportunityId).Build(),
    };

    Expression<Func<Domain.EF.Opportunity, bool>> queryParam; // = ???

    var actualOpportunities = new List<Opportunity>();
    var func = queryParam.Compile();

    actualOpportunities = testOpportunities.Where(func).ToList();

    actualOpportunities.Count.Should().Equals(1);
    actualOpportunities[0].OpportunityId.Should().Equals(ExpectedOpportunityId);
}   

You could consider this to be more of an integration test. But then what's being tested? That entity framework works? Or SQL? If so, these fall significantly outside of the boundaries of what a unit test can do well for you.

For my money it's an integration test and needs to exercise production code. I think you can safely remove this test as a unit test.

\$\endgroup\$
  • \$\begingroup\$ As per the first line of the post I was "testing predicate logic in a repository". This logic is in "var results = sut.GetFollowedByUserId(UserId);" You will notice that the mockOpportunityRepository has a callback. So, when the repository encounters the entityframework GetAsync method, instead of calling the underlying implementation, our callback is called to extract the predicate and test it. As per previous comment, I agree it should be an integration test in retrospect, but i disagree this is testing the mocks. Your example has refactored out the SUT, which is needed to test against! \$\endgroup\$ – sarin Oct 20 '17 at 17:12
  • 1
    \$\begingroup\$ @sarin ok. I think i see what you are going for. could you make the predicate more available and test it directly without having to mock a framework around it? That would simplify your tests greatly and get straight to the heart of what you want to test. \$\endgroup\$ – TBone Oct 20 '17 at 17:29
  • \$\begingroup\$ Probably yes. This was a problem a while ago and have since moved on. Who knows, by the time I get back to EntityFramework EF Core might have evolved and support basic querying (sarcasm...) as well as being testable... \$\endgroup\$ – sarin Oct 20 '17 at 17:37
-1
\$\begingroup\$

How about putting parameters on your test method, and then using the [TestCase] attribute to specify the different tests?

\$\endgroup\$
  • \$\begingroup\$ In this case I am testing several repository methods so would need to vary the test method. var results = sut.MethodToTest(Params). Not sure how to go about that. Agree if I was testing the same method this would be a good solution. \$\endgroup\$ – sarin May 7 '16 at 21:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.