Unit testing anti-patterns: Structural Inspection

By Vladimir Khorikov

This post is about the practice of Structural Inspection in unit testing and why I personally consider it an anti-pattern.

Structural Inspection

But first, let me explain Structural Inspection itself. Structural Inspection is generally about writing tests verifying that the system under test (SUT) has some particular structure. Let’s say we’ve got the following code base:

public class Message

{

    public string Header { get; set; }

    public string Body { get; set; }

    public string Metadata { get; set; }

}

 

public interface IProcessor

{

    string Process(Message message);

}

 

public class MessageProcessor : IProcessor

{

    private readonly List<IProcessor> _subProcessors;

    public IReadOnlyList<IProcessor> SubProcessors => _subProcessors;

 

    public MessageProcessor()

    {

        _subProcessors = new List<IProcessor>

        {

            new HeaderProcessor(),

            new BodyProcessor(),

            new MetadataProcessor()

        };

    }

 

    // IProcessor implementation

}

Following the Structural Inspection unit testing technique, you could write a test that makes sure the MessageProcessor class implements the IProcessor interface, like this:

[Fact]

public void MessageProcessor_implements_IProcessor()

{

    var processor = new MessageProcessor();

    Assert.IsAssignableFrom<IProcessor>(processor);

}

And to verify that the routine the processor follows is also correct, you could add a test similar to the following:

[Fact]

public void MessageProcessor_uses_correct_sub_processors()

{

    var processor = new MessageProcessor();

 

    IReadOnlyList<IProcessor> processors = processor.SubProcessors;

 

    Assert.Equal(3, processors.Count);

    Assert.IsAssignableFrom<HeaderProcessor>(processors[0]);

    Assert.IsAssignableFrom<BodyProcessor>(processors[1]);

    Assert.IsAssignableFrom<MetadataProcessor>(processors[2]);

}

It checks to see if the sub-processors are all of the expected types and appear in a correct order which means the way MessageProcessor processes messages must also be correct.

Structural Inspection is an anti-pattern

Alright, so why do I think this is a bad practice? Let’s take a look at it from the value proposition perspective I wrote about in an earlier post. Here are 3 components that comprise a valuable unit test:

  • High chance of catching a regression error.
  • Low chance of producing a false positive.
  • Fast feedback.

The first and the third attributes are good here. Such tests would most likely reveal an error assuming the existing implementation is correct and they run fast. However, they fall short of protection against false positives.

What if you change the way you arrange the sub-processors? Or remove the IProcessor interface implementation because you’ve come to realize that it’s not necessary in this particular case? The tests will turn red regardless of whether or not the functionality itself is broken.

Structural Inspection encourages you to couple your tests to the SUT’s implementation details which is never a good idea. The problem is essentially the same as with the excessive use of mocks: such tests are not able to distinguish a bug from a legit refactoring.

Structural Inspection is sometimes claimed to be able to prove the code base’s correctness. In reality, though, it sets your code in stone. The only thing it proves is that the SUT is implemented in one particular way. And that way may or may not be correct, you will need to introduce additional tests to verify that anyway.

When I think of inspecting the structure of the SUT, this is what comes to mind:

[Fact]

public void MessageProcessor_is_implemented_correctly()

{

    string sourceCode = File.ReadAllText(@”<project path>\MessageProcessor.cs”);

    Assert.Equal(

@”public class MessageProcessor : IProcessor

{

    private readonly List<IProcessor> _subProcessors;

    public IReadOnlyList<IProcessor> SubProcessors => _subProcessors;

 

    public MessageProcessor()

    {

        _subProcessors = new List<IProcessor>

        {

            new HeaderProcessor(),

            new BodyProcessor(),

            new MetadataProcessor()

        };

    }

}”, sourceCode);

}

This test is just plain ridiculous. At the same time, it’s not that different from the two tests I brought earlier. All 3 insist on a particular implementation of the SUT without taking into account the outcome it produces. And all 3 will break each time you change that implementation. Admittedly, though, this test will turn red more often.

Structural Inspection: alternatives

So what are the alternatives for the Structural Inspection technique? The alternative is to verify the end result the SUT generates and to distance yourself from the implementation details as much as possible.

So, in the example above, what is it that the MessageProcessor class actually does? Here’s its full source code:

public class MessageProcessor : IProcessor

{

    private readonly List<IProcessor> _subProcessors;

    public IReadOnlyList<IProcessor> SubProcessors => _subProcessors;

 

    public MessageProcessor()

    {

        _subProcessors = new List<IProcessor>

        {

            new HeaderProcessor(),

            new BodyProcessor(),

            new MetadataProcessor()

        };

    }

 

    public string Process(Message message)

    {

        return _subProcessors

            .Select(x => x.Process(message))

            .Aggregate(string.Empty, (str1, str2) => str1 + str2);

    }

}

The only thing that makes sense to check here is the string the Process method returns as it’s the only observable result we get out of it. As long as the output stays the same, we shouldn’t worry about how exactly that output was generated. These implementation details are simply irrelevant.

By the way, getting rid of inspection tests will allow us to remove the SubProcessors property. Which is a good thing because the only purpose of it is to expose the internal structure of the class in order to exercise it.

Overall, try to constantly ask yourself a question: does this test verify some business requirement? If the answer is no, remove it. The most valuable tests are always the tests that have at least some connection to the business requirements your code base is ought to address. Not surprisingly, such tests also fall into the formal definition I brought in the beginning of this article: they are likely to give a protection against regression errors and they are unlikely to turn red without a good reason.

If you enjoyed this article, check out my Pragmatic Unit Testing Pluralsight course, you’ll love it too.

Summary

Tests that employ the Structural Inspection technique couple to the SUT’s implementation details and thus are fragile. Because of that, they don’t contribute to your confidence in the software correctness.

The most valuable tests are tests that verify the observable behavior as it seems to appear from the end user’s perspective. The closer you can get to this kind of verification, the better.

Other articles in the series





  • https://github.com/Mykezero Mykezero

    The ‘MessageProcessor_implements_IProcessor’ test is useful for creating the first MessageProcessor class without having to worry about all the details at that moment.

    Currently, that is the first test I write when creating a new class. Afterwards, if I wanted to test MessageProcessor.Process(Message), that’s when I would start testing the methods. I do agree that, that test provides no value in of itself.

    Hopefully this provides some insight as to why someone would choose to write a test like that in the first place.

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      I agree, such tests can be useful in the process of writing code when you want to adhere to the TDD cycle. However, I would recommend to remove them after you’ve written “real” tests that cover the SUT’s observable behavior.

  • Szymon Ko

    I really like your blog cause it shows usefull design tactics in KISS’s way. I read all articles and observed that you (in case of design) describe mainly design principles, ddd, functional practices in c# and automatic tests. There are no articles concerning GoF design patterns. What’s the reason – are they no use today in enterprise development (cause of violating KISS often) or you just haven’t written about them yet or…? All in all I would love one / series of articles about them – in context of your programming experience in real world problems, are they helpful for you if so which of them…

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      Thanks! Really impressed that you’ve read all the articles.

      No particular reason actually. I do find them useful, but haven’t yet found an story to write about that would involve GoF patterns specifically. (Except this one: http://enterprisecraftsmanship.com/2016/05/03/singleton-vs-dependency-injection/ ).

      I think this might be related to the fact that the GoF design patterns are mostly a tool to describe a design. They don’t impose a particular way of designing, and they don’t provide a set of guidelines to how to do that. They are mostly a communication tool.

      Also, with the latest language features (at least in C#), most of these patterns have silently merged to our day-to-day work. For example, passing a simple delegate to a method can be interpreted as an implementation of the Strategy pattern. But few people perceive it as such, it’s become too common.

      However, if you have some particular topics in mind, I’ll be glad to consider them as post ideas and express my views on them in the future articles.

      • Szymon Ko

        “However, if you have some particular topics in mind, I’ll be glad to
        consider them as post ideas and express my views on them in the future
        articles.”
        – I understand you put your design toolbox mainly in domain model, so I’ll be interested in any use cases in which you incorporated design pattern(s). The most obvious is factory (which produce entity / value object in proper state) I think.
        – Use cases of usages of design patterns in higher level places (application services, domain services, repositories) would also be nice.

        Another topics (not strictly connected with design pattern) I’ll would like to read about are:
        – your subjective comparison between application / domain / infrastructure services
        – domain events with practical implementation of them

        • http://enterprisecraftsmanship.com/ Vladimir Khorikov

          Great topic ideas! Added them to my todo list, will try to address them soon. Thanks.

  • Sergey Tachenov

    What about tests that don’t verify some behavior, but rather verify that a class does NOT provide certain behavior? For example, when implementing a ViewModel, one might want to write a test that ensures that some “Items” property is a “ReadOnlyObservableCollection”. For a regular collection, you can test that it’s read-only by verifying that its ‘Add’ method throws an appropriate exception. However, “ReadOnlyObservableCollection” simply does not have an “Add” method, and there is no (reasonable) way to verify that “this code doesn’t compile”. Testing that this particular property has the right type seems to be the easiest way to test this kind of thing. Of course, you can just skip this test altogether, but then someone may accidentally change the type to “ObservableCollection” and then someone else may start adding items to that collection directly instead of via the “AddItem” method, thus breaking encapsulation and causing subtle bugs.

    This may sound as stupid as testing that fields are private. And indeed, it’s almost the same kind of thing. Except that people rarely make fields non-private nowadays, whereas accidentally using “ObservableCollection” as property type is something that may very well happen in real life.

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      That’s an interesting angle to view this problem from. I would personally skip those tests altogether. View models shouldn’t contain business logic anyway, they are more of a glue that combine your domain model with the UI, so covering them with unit tests often doesn’t pay off.

      Another option I would recommend if you are really into solid end-to-end verification is writing UI tests. They provide better protection as they go through a more code (both the one you wrote and the one you didn’t – the code of the UI framework itself).

      Verifying that some behavior doesn’t exist is a complicated way to ensure code correctness. There are simply too many negative scenarios that might happen and you can’t possibly account for all of them.

      So, while agree that structural inspection is the only way to do the kind of verification you are referring to without involving UI tests, I would argue that this type of verification itself is often not worth being implemented. It tends to cement your code (resulting in lots of false positives in case you want to refactor your view models) and doesn’t speak the language of business requirements.

      • Sergey Tachenov

        Hmm… I never thought of unit tests as verifying business logic. I thought that the job for acceptance tests, and unit tests should verify all kinds of logic. And VMs are full of presentation logic. And I’ve found TDD to be very useful for VM development. And isn’t one of the points of MVVM to enable VM testing without UI?

        Still, I don’t do structural inspection for VMs either. I find them boring, and coding guidelines and code review is just as good for preventing that kind of silly mistakes.

        • http://enterprisecraftsmanship.com/ Vladimir Khorikov

          And isn’t one of the points of MVVM to enable VM testing without UI?

          It is.

          However, my standpoint on unit testing is that we need to approach it in a pragmatic manner, meaning that we need to write only those tests that provide the best return on investment. In that sense, unit tests covering view models don’t provide enough value and thus don’t justify the effort you put into them. Of course, that is true only when MVVM is implemented keeping a good separation of concerns, when there’s no domain logic leaking to view models.

          Here I wrote more about that: http://enterprisecraftsmanship.com/2015/07/06/how-to-do-painless-tdd/
          And here’s a great article from Steve Sanderson on the same topic: http://blog.stevensanderson.com/2009/11/04/selective-unit-testing-costs-and-benefits/

          Hmm… I never thought of unit tests as verifying business logic. I thought that the job for acceptance tests, and unit tests should verify all kinds of logic.

          I don’t think there’s this kind of separation between the two. While it’s true that you can verify any kind of logic in unit tests, the best ROI yield the tests covering the business logic. Moreover, it’s a good idea to approach that from the end user’s perspective and work with the SUT in as black-box manner as possible. In my experience, white-box testing (and structural inspection is part of that) is rarely justified.

          • Sergey Tachenov

            Oh, I see. I work in space industry, where we have a lot of time, very high reliability requirements and don’t bother with investment efficiency, so I never thought about it in this way. I guess things are different in business where developers don’t have all day to write a lot of unit tests that don’t provide enough return to justify their precious time.

            Great articles, by the way! I’ve read a couple now, and going to read them all because they look very useful.

            One last thing: is structural inspection really a part of white-box testing? If you’re checking for implemented interfaces and public member types, how is it white-box? You’re not looking inside.

          • http://enterprisecraftsmanship.com/ Vladimir Khorikov

            Thanks, appreciate the compliment!

            Things indeed are very different when you’ve got strong reliability requirements. The approach I advocate for is applicable for typical line of business applications where the cost of error isn’t as high.

            One last thing: is structural inspection really a part of white-box testing? If you’re checking for implemented interfaces and public member types, how is it white-box? You’re not looking inside.

            That’s an interesting point. Now, as you provided the MVVM example, I no longer think it is always the case. I think it depends on what members of the SUT you inspect. If they have a meaning to the outside layer, that indeed shouldn’t be considered white-box testing. If not, it means you are validating implementation details and follow the white-box approach. Here I wrote more on the subject of public API vs implementation detail: http://enterprisecraftsmanship.com/2016/07/27/what-is-an-implementation-detail/

            In the example you provided earlier, the Items property is part of the VM layer’s public API as it is used by the external framework, hence inspecting it isn’t white-box. In the example from the article, the fact that MessageProcessor implements IProcessor is an implementation detail, it doesn’t have any meaning to the outer layer. Therefore, verifying that fact means you treat the SUT as a white box.

          • Sergey Tachenov

            But in your example, IProcessor is still a part of the public API. And it may be important that it should implement this interface because if it just has all the necessary methods, then someone may try to cast it to IProcessor and get an exception. This only applies if you’re developing a library for public use, though. If the class is used internally by your application, then you either don’t cast it or, if you do, then the exception would fail some other test, unit or integration.

            Not that I think it’s worth testing even in the case of a public library, though. Better to actually cast it to IProcessor in some other test. But funny thing is, in case of a public library, this kind of structural inspection actually does verify your business requirement (assuming there is a requirement that this class should implement that interface)!

          • http://enterprisecraftsmanship.com/ Vladimir Khorikov

            I’d say that even when being part of some redistributable library, not everything about MessageProcessor would be part of its published API (I’m referring to this article here: http://www.martinfowler.com/ieeeSoftware/published.pdf ). For such libraries, structural inspection is a black-box testing only when it verifies the published part of the API. Clients should couple to non-published part of the API at their own peril, keeping in mind that that API is actually an implementation detail and may change in the future.

            For internal code, the “rules” are roughly the same except the way you identify an implementation detail. While in a public library, there’s no way to see how exactly the library is used and you need to somehow convey the correct usage pattern to the clients, either formally or informally (here comes the term “published interface”), in internal code, you can check the usage patterns directly, by searching for the actual usages.

            I think I can summarize my opinion on the subject in the following way: structural inspection is a bad practice only when it’s aimed at verifying an implementation detail. In this case, it encourages you to couple tests to those details and thus make them fragile. But even if you verify some public contract, such tests don’t provide a lot of value, and I’d recommend to omit them (in a typical enterprise application).

          • Sergey Tachenov

            Ah, I see what you’re getting at! I agree completely, but I would like to add that we should strive to avoid making non-published parts of our API public (for a published library). If IProcessor is an implementation detail, then MessageProcessor (which I assume to be a part of published API) has no business implementing it in the first place! If it needs to be implemented for the purpose of MessageProcessor, then this interface should be implemented by some other class (perhaps a nested one) and that class should be internal (or even private), and MessageProcessor should use that class.

            I think we’ve come to an agreement about structural inspection. I’d say it’s either harmful (if it verifies implementation details) or just plain useless (if it just checks that the API is what it’s supposed to be) because you’re going to test that API anyway and if you do it right, those tests will break anyway if the API is wrong.

          • http://enterprisecraftsmanship.com/ Vladimir Khorikov

            Absolutely! Ideally, public and published parts of API should be aligned to avoid leaking implementation details.

            And I agree with your conclusion about structural inspection.

            you’re going to test that API anyway

            This part is hard to implement, however, if you are developing for an external framework (example – WPF calling methods on your view models), as opposed to when you are writing a public library methods on which you can call yourself in tests. With a framework, you either need to write tests that capture that framework along with your own code (UI tests), or just forgo on verifying this part of public API altogether.

          • Sergey Tachenov

            Oh, I didn’t mean developing for frameworks. I meant developing published libraries. Like, if you’re developing a library that has a class implementing, say, INotifyCollectionChanged, then you’re obviously going to have a test that upcasts an instance of your class to INotifyCollectionChanged and tests that the methods of that interface work as intended. Therefore, structural inspection testing that that class actually implements INotifyCollectionChanged becomes redundant.

            If you’re developing for a framework, that’s your call. I’m happily testing my VMs right now, and I’ve found it very helpful in fact. Just today I had a subtle bug when one VM subscribed to changes in another VM and made some changes in one its collections, but that other VM turned out to be in the middle of a foreach loop over that very same collection, which resulted to an exception which was happily swallowed by the framework, causing the initial command to silently stop halfway. This was instantly detected by the appropriate test and fixed in a few minutes.

            But on the other hand, VM tests are rather boring and repetitive. I’d definitely not do them when facing a tight deadline or some other kind of pressure.

            P. S. Oh, and I must say, even though I do VM testing, it’s not unit testing at all. It’s integration testing with full domain model and even an in-memory SQLite database.