How to unit test a method calling a function that has a side-effect by changing an argument passed by reference?

Question

I have a method that makes a call to a builtin PHP function, openssl_random_pseudo_bytes.

public function generateRandomBytes()
{
    $crypto_secure = TRUE;

    // $crypto_secure is passed by reference and will be set to FALSE by
    // openssl_random_pseudo_bytes if it uses an insecure algorithm
    $random_bytes = openssl_random_pseudo_bytes(16, $crypto_secure);
    if (!$crypto_secure)
    {
        throw new Security_Exception('Random bytes not generated by a cryptographically secure PRNG algorithm');
    }
    return $random_bytes;
}

I have one PHPUnit test case to test this method (all it does is verify that the randomly generated string is 16 bytes long).

public function testRandomBytesLength()
{
    $myclass = new MyClass();

    $this->assertEquals(16, strlen($myclass->generateRandomBytes()));
}

My question is, how do I test the case in which $crypto_secure is FALSE and an exception must be thrown? Since this value is passed in and modified as a reference to openssl_random_pseudo_bytes, I'm not sure how I could get test coverage for this execution path. My first thought was that maybe there's a php.ini configuration that I could use to force openssl_random_pseudo_bytes to use a cryptographically insecure algorithm (via ini_set in the test case). Any suggestions?

Answer 1

One option is to abstract your code out so you can mock the return value of the openssl method:

public function generateRandomBytes()
{
    $crypto_secure = TRUE;
    $random_bytes = $this->randomPseudoBytes(16, $crypto_secure);
    if (!$crypto_secure)
    {
        throw new Security_Exception('Random bytes not generated by a cryptographically secure PRNG algorithm');
    }
    return $random_bytes;
}

protected function randomPseudoBytes($length, &$crypto_secure)
{
    return openssl_random_pseudo_bytes(16, $crypto_secure);
}

Then you can control the wrapper around the core function to test how your code reacts to it changing:

/**
 * @expectedException Security_Exception
 * @expectedExceptionMessage Random bytes not generated by a cryptographically secure PRNG algorithm
 */
public function testCryptoIsNotSecure()
{
    $myclass = $this->getMockBuilder(MyClass::class)->setMethods(['randomPseudoBytes'])->getMock();

    $myclass->expects($this->once())
        ->method('randomPseudoBytes')
        ->will($this->returnCallback(function ($length, &$secure) {
            // Mock variable assignment via reference
            $secure = false;
        });

    $myclass->generateRandomBytes();
}

Answer 2

This answer below was written under the assumption that openssl-random-pseudo-bytes takes two immubtable arguments. Instead, the second argument is passed by reference and gives feedback if the random bytes were created by a strong algorithm. Given that information, the answer provided by Robbie Averill is a valid approach as one has to deal with basically two return-statements and a side-effect, which inherently complicate unit-testing.

---

You don't need your Security exception in your case.

You want to wrap openssl_random_pseudo_bytes into your own custom function and you want to hardcode the length to 16 chars and to always call openssl_random_pseudo_bytes with true. Hence you can write your class as:

class MyClass
{
    public function generateRandomBytes()
    {
        return openssl_random_pseudo_bytes(16, true);
    }
}

The only meaninful test here is to check that the returned string length is of 16 chars. And you have covered that case.

---

In order to show the needlessness of the exception throwing, you rather would inject the flag in the constructor or as a parameter:

class MyClass
{
    /**
     * @var bool
     */
    private $beSecure;

    public function __construct(bool $beSecure)
    {
        $this->beSecure = $beSecure;
    }


    /**
     * @return string
     * @throws Exception
     */
    public function generateRandomBytes(): string
    {
        if (!$this->beSecure) {
            // will always throw if false is injected, why would we do that?
            throw new Exception("I AM NOT SECURE!");
        }

        return openssl_random_pseudo_bytes(16, true);
    }
}

In your unit tests you can now create two test, one for the secure case, and one for the insecure case, yet why would you ever want to inject false to that class? It would then always fail.