Bad Practice - having a SUT use a different execution path when in test mode?

Question

I recently ran across some code in a public repo that I'd thought... just plain wrong. The first thing I noticed was

/**
 * Sets a test mode status.
 *
 * @param boolean $mode Mode to set
 */
public function testMode(bool $mode = true)
{
    $this->testMode = $mode;
    return $this;
}

Then another method used the testMode property like this (mostly pseudo-code to protect the innocent).

public someDbFunc($table)
{
    $sql = makeQueryStatement($table);

    if ($this->testMode)
    {
        return $sql;
    }

   // code that really does database stuff
   //...
}

Doesn't this kind of thing make unit testing pointless? Which is worse, the testing code or the code being tested?

Edit

Adding an approximation of an actual test.

// 'reservations' is the name of a table
public function testSomeDbFuncMakesGoodSql)
{
    $generator = new QueryBuilder('reservations', $this->db);
    $generator->testMode();

    $expectedSQL = 'SELECT COUNT(*) AS "numrows" FROM "reservations"';

    $this->assertEquals($expectedSQL, $builder->someDbFunc('reservations));
}

That is pretty typical of how $this->testMode() is used. The test returns the query statement instead of carrying out the db operation. There are a few uses where it (for lack of a better term) mocks the return. These are all either write or delete type operations.

Answer 1

Yeah it is a bit bad.

However, let play devil's advocate. Unit testing code which interacts with a database is difficult.

I guess the most popular approach would be to add an in memory database to the test. But Databases are not all created equal. I've had cases where the sql ran fine against the in memory db but failed against the production db.

What a lot of developers want to unit test is

Does the function run the correct sql string?

But the actual generated string is often impossible to expose, and injecting a "database context" of some kind tends to fall down because they are so hard to mock. Hence this kind of work around.

I would always create integration tests for a repository and skip unit tests, at least for the "run the sql" bits

Answer 2

Doesn't this kind of thing make unit testing pointless?

Not necessarily. Unit tests -- especially the flavor of "developer tests" that tend to be run between edits in a code base -- are somewhat contrived anyway, being as they are typically being run within a nice simple carefully managed sand box, rather than in a multi threaded production environment.

In this specific case, you are in effect dealing with a test double - code that within a test environment is configured to use an immutable in memory implementation as a substitute for shared mutable state. It may look out of place, because we are accustomed to seeing that approach implemented via a testing framework, rather than in the production code.

The branch in the middle of the business logic is a bit odd -- instead of testing the condition here, we would normally test it somewhere closer to the composition root, selecting the correct behavior at that point, and within the business logic blindly invoking the selected behavior. In other words, during the construction phase we would branch to decide which collaborator to use.

(Note: "construction phase" can be a tricky thing -- if we are expecting to be able to run tests on the production appliance, then we're probably going to be setting "testMode" on a single session or request, rather than interrupting all of the traffic.)