Test Smell: Mystery Guest

This is a series of short blog posts on the topic of test architecture. Most topics evolved from my talk "Dealing with Testing Fatigue" which highlights typical anti-patterns in test code, why they are problematic and how to improve them.

The "mystery guest" is test data from outside the test method. This can be some external resource like a file or a database record, or data from a general fixture, i.e. centralized test setup for multiple tests. As such, this test data is disconnected from the rest of the test.

Problem

A bigger problem is when this external resource is shared by several tests. Now there is the risk of somebody else editing it without knowing how it's used, and break our tests. Actually it doesn't have to be somebody else, just you working on another feature that uses the same resource.

Possible Solutions

For example to create a CSV file with newsletter subscribers that needs name, email and status columns but we are only interested in the "email" column:

private function createSubscriberCsv(string $filename, string ...$emails): void
{
    $file = fopen($filename, 'w');
    foreach ($emails as $email) {
        fputcsv($file, ['John Doe', $email, 'SUBSCRIBED']);
    }
    fclose($file);
}

Now a test for importing this CSV can look like this:

public function testImportEmails()
{
    // setup
    $filename = tempnam('/tmp', '');
    $emailsToImport = ['user1@example.com', 'user2@example.com'];
    $this->createSubscriberCsv($filename, ...$emailsToImport);

    // exercise
    $this->subscribers->importFromCsvFile($filename);

    // verify
    $this->assertTrue(
        $this->subscribers->isSubscribed($emailsToImport[0]),
        "Email {$emailsToImport[0]} should be subscribed"
    );
    $this->assertTrue(
        $this->subscribers->isSubscribed($emailsToImport[1]),
        "Email {$emailsToImport[1]} should be subscribed"
    );

    // teardown
    unlink($filename);
}

For the sake of a short example, I chose a very small and simple file format. In this case it would be perfectly fine to write the file without an additional method and have a setup like this:

    // setup
    $filename = tempname('/tmp', '');
    $emailsToImport = ['user1@example.com', 'user2@example.com'];
    file_put_contents(

<<

If the test uses a general fixture, i.e. one test setup that is reused by multiple tests, set up a fresh fixture for each test instead. Or if you want to keep the general fixture, at least write accurately named finder methods. For example, if you test behavior of recently added products and $this->product happens to be a recently added product, loaded from a general fixture, introduce a finder method like:

private function getRecentlyAddedProduct(): Product
{
    return $this->product;
}

Now instead of using $this->product directly you can use this method to make clear why you are using that product and which aspects of it you are interested in for the test.

Examples from Magento

public function testBundleImport()
{
    // import data from CSV file
    $pathToFile = __DIR__ . '/../../_files/import_bundle.csv';
    // ...
}

Since it is testing an import, that choice seems natural. But it comes with all the disadvantages of separating fixture (test setup) and verification (assertions) as described above. A minor improvement would be to move the file next to the test, so that it's easier to navigate between the two. But it does not beat creating a temporary file in the test itself.

Unfortunately, one mystery guest is deeply ingrained into the Magento 2 integration test framework: The fixture scripts:

/**
 * @magentoAppArea adminhtml
 * @magentoDataFixture Magento/Reports/_files/viewed_products.php
 */
public function testExecute()
{
    $this->dispatch('backend/admin/dashboard/productsViewed/');
    $this->assertEquals(200, $this->getResponse()->getHttpResponseCode());
    $actual = $this->getResponse()->getBody();
    $this->assertContains('Simple Product', $actual);
}

The @magentoDataFixture annotation tells the test framework to execute the given PHP file before the test. It contains procedural code to set up a database fixture. In this case: creates some products and "viewed product" stats.

Then in the test, an assertion comes out of nowhere: The admin dashboard should contain the words "Simple Product". If you read the fixture script, you will eventually find this as a product name, 200 lines deep into a second included script.

How could this test be more clear? Let's introduce methods that create a product and register a "viewed" event:

/**
 * @magentoAppArea adminhtml
 */
public function testExecute()
{
    $productName = 'Simple Product';
    $product = $this->createProductWithName($productName);
    $this->countView($product);
    $this->dispatch('backend/admin/dashboard/productsViewed/');
    $this->assertEquals(200, $this->getResponse()->getHttpResponseCode());
    $actual = $this->getResponse()->getBody();
    $this->assertContains($productName, $actual);
}

The test now is more verbose, but there's a clear connection between fixture setup and verification. We immediately see that there is a product named "Simple Product" that has been viewed once. The $productName variable in the assertion makes it obvious, that we are looking for this product name.

There's more potential to improve this test, but getting rid of the mystery guest smell already helps a lot. By the way, for this test it does not matter, how createProductWithName() and countView() are implemented. For what it's worth the methods could include procedural fixture files, as long as the product name parameter gets passed along, being the relevant information that is used in the verification, and the high level test method expresses its intent clearly.

By the way, if you ever worked with EcomDev_PHPUnit for Magento 1, the YAML fixtures are another example of the same smell. Sometimes I still have to work with them, or with core fixture scripts from Magento 2, in that case I add constants for clarity, like this:

    /*
     * Values from fixture:
     */
    private const VIEWED_PRODUCT_NAME = 'Simple Product';
    ...
    

This does not mitigate all the problems, but at least makes the connection a bit more explicit.

Conclusion

Keep interdependent test data and verification closely together, so that cause and effect is easily understood while reading the test. It should not be necessary to look into external resources to understand why a certain outcome is expected.

Further reading

Read more on the "Mystery Guest" test smell in the XUnit Patterns directory: Obscure Test: Mystery Guest