6

Imagine a psedo-code class like this which does name comparisons:

public class NameComparer
{
    public bool DoNamesMatch(Name name1, Name name2)
    {
        if(WholeNameMatch(name1, name2))
        {
            return true;
        }

        return ProcessorIntensiveFuzzyMatcher(name1.Forename, name2.Forename)
                && ProcessorIntensiveFuzzyMatcher(name1.Surname, name2.Surname)
    }

    private bool WholeNameMatch(Name name1, Name name2)
    {
        return (name1.Forename == name2.Forename)
                && (name1.Surname == name2.Surname);
    }

    private bool ProcessorIntensiveFuzzyMatcher(string string1, string string2)
    {
        // complex fuzzy matching
    }
}

Best practice suggests that this class only ought to have one public method, and that we ought to unit test the private methods in this class through that public method.

While I can write various tests that ensure WholeNameMatch and ProcessorIntensiveFuzzyMatcher both do what they're supposed to do, the order of operation is important here. If a developer mistakenly moves one above the other, the program is going to take a massive performance hit.

Is it desirable - and possible - to test this order of operations via the exposed public method?

  • 2
    You're describing a functional requirement (public method must execute in x time) that can be tested with another unit test, given some assumptions about the hardware you're running the tests on. – Robert Harvey Nov 9 '17 at 17:27
  • 2
    This is also a good reason to put some comments in the public method that explain why the private methods are being called in a specific order. – Robert Harvey Nov 9 '17 at 17:29
  • 1
    Most mock frameworks have that functionality. For example: alastairchristian.com/… – Eternal21 Nov 9 '17 at 17:38
  • 1
    If you are concerned about performance, why not write a test for that instead? – Frank Hileman Nov 10 '17 at 0:32
  • @FrankHileman: unit tests for performance tend to become brittle, since they depend on the hardware involved. For this case, there are better alternatives. – Doc Brown Nov 10 '17 at 13:21
11

The premise of your question is incorrect. You don't want to test that first WholeNameMatch is called and then ProcessorIntensiveFuzzyMatcher is called.

You want to test

GIVEN WholeNameMatch returns true
WHEN NamesMatch is called
THEN ProcessorIntensiveFuzzyMatch is not called

This is easily done if the fuzzy matching is done by an injected dependency that is mocked by the unit test. Most mocking frameworks allow you to verify that a call did not occur.

If the fuzzy matcher is not an injected dependency, it should be.

  • 3
    That test is coupled to implementation details, since it checks if private methods are called. – JacquesB Nov 10 '17 at 17:10
6

@KevinCline gave an excellent answer, but I would like to add something one can do when injecting the fuzzy matcher does not seem worth the hassle, and to solve things in a more simple way.

One can simply add a public boolean read-only property LastMatchWasFuzzy to NameComparer, and initialize it this way:

public bool DoNamesMatch(Name name1, Name name2)
{
    LastMatchWasFuzzy=false;
    if(WholeNameMatch(name1, name2))
    {
        return true;
    }

    LastMatchWasFuzzy = ProcessorIntensiveFuzzyMatcher(name1.Forename, name2.Forename)
                     && ProcessorIntensiveFuzzyMatcher(name1.Surname, name2.Surname);
    return LastMatchWasFuzzy;
}

This gives the caller a chance to test how the last match was achieved, which can be beneficial not just for a unit test, but for other use cases as well.

In general, sometimes it is not the worst strategy to add some "maintenance hatches" to a class, which means properties or functions just for testing purposes (however, this is a always a tradeoff, these should not pollute the public API in a disturbing way).

  • 2
    This was enlightening. If the abstraction barriers get leaky clients will care so let the leak reach the public interface in a way that makes sense for them. – Goyo Nov 10 '17 at 12:59
  • 1
    A nice pragmatic solution, if used with caution. – user949300 Nov 10 '17 at 16:20
5

Best practice suggests that this class only ought to have one public method, and that we ought to unit test the private methods in this class through that public method.

Best practice suggests that tests do not care about implementation details like private methods. Public methods are a way to expose functionality to the outside world, not a way to test private methods.

While I can write various tests that ensure WholeNameMatch and ProcessorIntensiveFuzzyMatcher both do what they're supposed to do,

Maybe you can but that is not what you want. You want to ensure that the public interface does what it is supposed to do. How it achieves that is not relevant to clients of your class and should not be relevant to your tests.

the order of operation is important here. If a developer mistakenly moves one above the other, the program is going to take a massive performance hit.

The order of private operations is meaningless to clients of your class, including tests, because they are not even aware that those operations exists, let alone in which order they are executed. If performance is important then test performance. Alternatively, follow kevin's advice about using dependency injection and the whole issue about private methods will disappear.

Is it desirable - and possible - to test this order of operations via the exposed public method?

Anything about private methods are implementation details. If you test implementation details and later you change implementation details your tests will fail. That is not desiderable, as long as the public interface provides the same functionality your tests should not break.

  • IMHO "Best practice suggests that tests do not care about implementation details like private methods" is a cargo cult rule which misses the addendum as long as the abstraction barrier created by them does not get leaky. There is no universal "best practice" in software development which holds to each and every situaion. – Doc Brown Nov 10 '17 at 12:12
  • @DocBrown I disagree. Both Kevin's answer and yours are good ways to deal with those leaks without leting tests care for implementation details. Instead you embrace the leak and adjust the design so it is not an implementation detail anymore. – Goyo Nov 10 '17 at 12:45
  • Ok, that's a way to interpret this, but my main point is, in the current form at least the first half of your answer looks like you are questioning the need for testing what the OP wants to test, which gives the (IMHO wrong) impression the OP's testing requirements are unjustified. – Doc Brown Nov 10 '17 at 13:18
1

No.

Unit tests are supposed to let you safely refactor or rewrite code by showing everything still works as it is supposed to. But if the test code is coupled to the implementation details of the unit, you will have to modify the test at the same time when you change the code. Which means you could easily introduce errors, and the whole benefit of unit tests is lost.

In short, unit tests should verify the requirements of the unit are fulfilled, but not care about implementation details.

This is why it is bad practice to test private methods. Private methods are supposed to be implementation details. The same goes for verifying if a private method is called or not, or in what order.

In your case, the expected behavior is some level of fuzzy matching. What algorithm is used, and whether it is implemented in one or more methods is an implementation detail.

Imagine you find a better fuzzy matching algorithm which have the property that it is as fast or faster as a regular string match when there is an exact match. This allows you to skip the special casing of the exact match. This would be an improvement which preserved the expected behavior - but it would cause your test to fail! The test prevent you from improving the code.

Or you might realize that if Forename has an exact match but Surname does not, then there is no reason to perform a costly fuzzy match on Forename again. But preventing this will require a restructuring of the order of method calls, so most likely you test will again prevent this change, even though behavior remains correct and performance is improved. So the test have negative value.

But I understand you want to ensure that comparing strings which are fully equal should not be significantly slower due to the fuzzy algorithm. I see two possibilities:

  • the performance benefit for this optimization is insignificant
  • the performance benefit is significant

If it is insignificant, you should not waste time on testing it. If it is significant you should test it, but you should test the actual performance (time or processor cycles), not irrelevant implementation details.

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.