Is a check like isInUnitTest() an antipattern?

Question

I'm working on a personal project (meaning clean source code, no legacy dependencies) and attempting to follow best practices regarding unit testing, dependency management, etc.

My company's codebase is littered with code like this:

public Response chargeCard(CreditCard card, Money amount) {
  if(Config.isInUnitTests()) {
      throw new IllegalStateException("Cannot make credit card API calls in unit tests!");
  }
  return CreditCardPOS.connect().charge(card, amount);
}

The goal here is to actively prevent dangerous code / code with external dependencies from being executed during testing. I like the notion of failing fast if unit tests do something bad, but I don't like this implementation for a few reasons:

• It leaves hidden dependencies to the static Config class scattered throughout our codebase.
• It changes the control flow between testing and live behavior, meaning we're not necessarily testing the same code.
• It adds an external dependency to a configuration file, or some other state-holding utility.
• It looks ugly :)

A fair number of the places where we use this in my company's codebase could be avoided by better dependency awareness, which I'm attempting to do, but there are still some places where I'm still struggling to see away around implementing an isInUnitTests() method.

Using the credit card example above, I can avoid the isInUnitTests() check on every charge by properly wrapping this up inside a mock-able CardCharger class, or something similar, but while cleaner I feel like I've only moved the problem up one level - how do I prevent a unit test from constructing a real instance of CardCharger without putting a check in the constructor / factory method that creates it?

• Is isInUnitTests() a code smell?
• If so, how can I still enforce that unit tests don't reach external dependencies?
• If not, what's the best way to implement such a method, and what are good practices for when to use / avoid it?

---

To clarify, I'm trying to prevent unit tests from accessing unacceptable resources, like the database or network. I'm all for test-friendly patterns like dependency injection, but good patterns are useless if they can be violated by a careless developer (i.e. me) - it seems important to me to fail-fast in cases where unit tests do things they aren't supposed to, but I'm not sure the best way to do that.

Answer 1

Is isInUnitTests() a code smell?

Yes, definitively, you have many ways to avoid coupling your code to unit tests. There is no valid reason to have something like that.

If so, how can I still enforce that unit tests don't reach external dependencies?

Your tests must only validate a unit of code and create mock or stubs for external dependencies.

Your code seems to be Java, which it's plenty of mature mock frameworks. Research a little about existing ones and pick the one that likes you more.

EDIT

how do I prevent a unit test from constructing a real instance of HTTPRequest without putting a check in the constructor / factory method that creates it

You're supposed to use a dependency injector to resolve your instance dependencies so you will never need to use a if to test if you're on a test or not since on your "real" code you inject full functional dependencies and on your test you inject mock or stubs.

Take a more serious example, like a credit card charger (a classic unit testing example) - it would seem like a very poor design if it were even possible for test code to access the real card charger without triggering a deluge of exceptions. I don't think it's enough in a case like that to trust the developer will always do the right thing

Again, you're supposed to inject external dependencies as a credit card charger so on your test you would inject a fake one. If some developer don't know this, I think the first thing your company needs is training for that developer and some pair programming to guide the process.

In any case, kind of I get your point and let me tell you a similar situation I experienced.

There was an application that sent a bunch of emails after some processing. These emails were not to be send on any other environment except live, otherwise it would be a big problem.

Before I started working on this application, it happened several times that developers "forgot" about this rule and emails were sent on test environments, causing a lot of problems. It's not a good strategy to depends on human memory to avoid this kind of problem.

What we did to avoid this to happen again was adding a config setting to indicate if send real emails or not. The problem was broader than executing things or not on unit tests as you can see.

Nothing replace communication though, a developer could set the incorrect value for this setting on his development environment. You're never 100% safe.

So in short:

• First line of defense is communication and training.
• You second line of defense should be dependency injection.
• If you feel that this is not enough, you could add a third line of defense: a configuration setting to avoid executing real logic on test/development environments. Nothing wrong about that. But please don't name it IsInUnitTest as the problem is broader than that (you want also o avoid this logic to be executed on developer machine)

Answer 2

The AngularJS Unit Testing Documentation actually does a really fantastic job of describing the different ways that developers handle unit testing.

Of the four different methods they outline, the one they recommend is one that involves using dependency injection so that your production code flow is the same as your testing flow. The only differences are that the objects you pass into your code would be different. Note that Claudio used the term "Stub" to refer to objects you pass into a method to act as placeholders for those you'd use in production:

public Document getPage(String url, AbstractJsoup jsoup) {
  return jsoup.connect(url).get();
}

My Java is a little rusty, so consider that you may need to actually do some funky stuff to make this work, like check the type of the jsoup object or cast it to your test Jsoup object or your production Jsoup object, which might just defeat the entire purpose of using dependency injection. Thus, if you're talking about a dynamic language like JavaScript or some other loosely-typed functional programming language, dependency injection would keep things really clean.

But in Java, unless you control the dependencies and can use a design pattern, like the strategy pattern, to pass in different concrete implementations, doing what you're doing may be easier, especially since you don't control the Jsoup code. However, you might check to see if they have available stubs you can use as placeholders, as some library developers do write stubs.

If you don't own the code, another option could be to use a Factory class to obtain the desired objects, depending on a flag you set when first instantiating it. This seems like a less desirable solution, since you're still sort of setting a global flag in that Factory object that may have an effect on things you might not be trying to test. The advantage of dependency injection for unit testing is that you must explicitly pass in a test stub when testing and explicitly pass in the production object when you wish the method to do what it was written to do. Once the method completes its execution, the test is over, and any production process that invokes it will automatically run it in production mode because it will inject production objects.

Answer 3

I've never seen a system where steps were taken to actively prevent code running under unit tests from accessing external resources. The problem just never came up. You have my deepest sympathy for working somewhere where it has.

Is there a way you could control the classpath used for unit tests, to make the libraries needed to access external resources unavailable? If there is no JSoup and no JDBC driver on the classpath, tests for code which attempts to use them will fail. You won't be able to exclude JDK-level classes like Socket and URLConnection this way, but it might still be useful.

If you were running tests using Gradle, that would be fairly straightforward. If you are using Maven, maybe not. I don't think there's any way to have different classpaths for compilation and testing in Eclipse.

Answer 4

Well, you can achieve the same clean way using Abstract Factory Pattern though (maybe not suitable to call it Abstract Factory Pattern).

Example in C#:

public class CardChargerWrapper{
    public CardChargerWrapper(
        NullCardCharger nullCharger
        , TestUnitConfig config){
        // assign local value
        this.charger = new CardCharger();
    }
    CardCharger charger;
    NullCardCharger nullCharger;
    TestUnitConfig config;

    public Response Charge(CreditCard card, Money amount){
        if(!config.IsUnitTest()) { return charger.connect().charge(card, amount); }
        else { return NullCardCharger.charge(card, amount); }
    }
}

EDIT: Changed CardChargerWrapper to use hard-coded instance of CardCharger instead of injecting it.

Note: You can change NullCardCharger to something like MockCardCharger or OfflineCardCharger for logging purpose.

Note again: You can change the CardChargerWrapper's constructor to fit. Example, instead of constructor injecting the NullCardCharger, you can make it property injected. Same with TestUnitConfig.

EDIT: Regarding calling IsUnitTest() a good idea or not:

It really depends on your business perspective and how you are doing testings. As many people said, a code that has not yet been tested is not trusted for their correctness. It cannot be reliabled. On side note, I prefer IsChargeRealCard() will be more suitable than IsUnitTest().

Say that we take out the unit test in our context, at least you will still need to do integration testing in test environment. You probably want to test something like:

1. I want to test the credit card validation (is it real or not, etc).
2. I want to test the payment method and see whether the card being charged. As a process, but not as a real credit card payment.

For 2nd point, I think the best thing to do is to create a mock credit card charger to log the transaction. This is, to ensure that the charging is correct. And it will be conducted in both test and dev server.

So, How can the CardChargerWrapper help such situation?

Now with CardChargerWrapper, you can:

1. Switch the NullCardCharger to any mock card chargers to enhance your unit testing.
2. All class using CardChargerWrapper can be ensured that they are checking IsUnitTest first before charging real card.
3. Your developer need to use CardChargerWrapper instead of CardCharger, preventing development error for unit tests.
4. During code review, you can find whether CardCharger being used in other class instead of CardChargerWrapper. This is to ensure no leak of code.
5. I'm unsure, but seems like you can hide references of your main project to your real CardCharger. This will protect your code further.

Answer 5

If [isInUnitTest() is an antipattern], how can I still enforce that unit tests don't reach external dependencies?

I now have a solution I'm fairly satisfied with, which will ensure external dependencies cannot be used in test environments without explicitly enabling them. All classes which depend on external resources (HTTP requests, databases, credit card processors, etc.) take as one of their arguments a configuration class which contains the necessary settings to initialize these objects. In a real environment, a real Config object is passed in containing the data they need. In a test environment, a mock is passed in, and without explicitly configuring the mock, the object will fail to construct/connect.

For instance, I have an Http connection utility:

public class HttpOp {
  public HttpOp(Config conf) {
    if(!conf.isHttpAllowed()) {
      throw new UnsupportedOperationException("Cannot execute HTTP requests in "+
          getClass()+" if HTTP is disabled.  Did you mean to mock this class?");
    }
  }
  ....
}

In a unit test, if you attempted to run code that constructs an HttpOp object, you'd raise an exception as the mocked Config will not return true unless explicitly set to do so. In a functional test where this is desired, you can do so explicitly:

@Test
public void connect() {
  State.Http httpState = mock(State.Http.class);
  when(httpState.isEnabled()).thenReturn(true);
  RemoteDataProcessor rdp = new RemoteDataProcessor(new HttpOp(httpState));
  ...

}

Of course, this still depends on Config being properly mocked in test environments, but now we have exactly one danger point to look for and reviewers can quickly verify the Config object is mocked and trust that only explicitly enabled utilities will be accessible. Similarly, there's now only one gotcha new team-members need to be told ("always mock Config in tests") and they can now be confident they won't accidentally charge a credit card or send emails to clients.

Answer 6

Update

This was an idea I had shortly after posting this question, however I'm now convinced it's not a good plan. Leaving it here for posterity, but see my newer answer for what I ended up doing.

---

I'm far from certain this is the right thing to do, but one thought I had, which at least addresses my first and third objections, comes from Determine if code is running as part of a unit test:

You could avoid storing external state about whether you're in a unit test or not by directly examining the execution stack, like so:

/**
 * Determines at runtime and caches per-thread if we're in unit tests.
 */
public static class IsInUnitTest extends ThreadLocal<Boolean> {
    private static final ImmutableSet<String> TEST_PACKAGES = 
                                       ImmutableSet.of("org.testng");

    @Override
    protected Boolean initialValue() {
        for(StackTraceElement ste : Thread.currentThread().getStackTrace()) {
            for(String pkg : TEST_PACKAGES) {
                if(ste.getClassName().startsWith(pkg)) {
                    return true;
                }
            }
        }
        return false;
    }
}

The primary advantage here is we don't store any state; we simply check the stack trace - if the trace contains a test-framework package, we're in a unit test, otherwise we're not. It's not perfect - in particular it could potentially lead to false positives if you use the same testing framework for integration or other more permissive tests - but avoiding external state seems like at least a slight win.

Curious what others think of this idea.