[BUG] race conditions & bugs due to connecting twice to database with WPTestCase and WPLoader | v3

[calvinalkan]

We're still using v3 (a fork actually) but the issue described below is also present in the latest commit on v3.

https://github.com/lucatume/wp-browser/blob/334cfebd01afe4db317b6f4c99c90fdcfd2d0d18/src/Codeception/TestCase/WPTestCase.php#L92

WPLoader connects to the database after installing WordPress (it calls wp-settings.php), then in the setUpBeforeClass method of WPTestCase we connect to the database again (see link above).

I'm not quite sure why it's done that way, but apart from the overhead of connecting twice, it never seemed to cause issues.

However, today I faced a situation where the transactions of the WPTestCase were entirely being ignored and data was committed straight away.

It took me forever to track this down, but the issue is because dataProviders are resolved before setUpBeforeClass is called, and I had a class that took wpdb as a constructor dependency to do some lower level operations on the actual dbh (mysqli connection).

So the dataProvider gets mysqliA (from wp-settings.php), and the WPTestCase manages transactions for mysqliB (from the second connection in setupBeforeClass).

---

This issue would probably not have surfaced if I did not operate on the dbh directly (since wpdb is not overwritten globally) but it still seems unneeded to connect twice to the DB and there are probably other edge cases where this might cause issues.

Basically, if any kind of setup work is performed anywhere between the bootstrap.php wp-settings call and the setupBeforeClass this issue will surface.

Proposed Solution:

I'm assuming there is a good reason for trying to connect again to the db in WPTestCase, but it should probably be wrapped with an if in case somebody already used WPLoader.

  static::setupForSeparateProcessBeforeClass();

        $wpdb->suppress_errors = false;
        $wpdb->show_errors = true;
        global $wpdb;

        if ($wpdb->check_connection(false)){
            $wpdb->db_connect();
        }

        // ....

        }

[calvinalkan]

Apart from dataProviders, this seems to be a bug in general, since a couple of lines after we try so commit some transaction which should never have any changes to commit because we just connected.

public static function _setUpBeforeClass()
    {
        static::setupForSeparateProcessBeforeClass();

        global $wpdb;

        $wpdb->suppress_errors = false;
        $wpdb->show_errors = true;
        $wpdb->db_connect();
        ini_set('display_errors', 1);

        $c = self::get_called_class();
        if (!method_exists($c, 'wpSetUpBeforeClass')) {
           //   THIS IS A BRAND NEW CONNECTION, nothing to be commited?
            self::commit_transaction();
            return;
        }

        call_user_func(array($c, 'wpSetUpBeforeClass'), self::factory());
           //   THIS IS A BRAND NEW CONNECTION, nothing to be commited?
        self::commit_transaction();
    }

And

[lucatume]

Hi @calvinalkan,

This is not a bug, but something that works like that in the Core PHPUnit suite as well, and has always been working like this. Your observations are correct: the WPTestCase class will start a transaction at the start of tests methods, not during set up of the test case or before data providers. Any database change made to before the start of a test method will not happen in a transaction, but will become part of the database fixture.

The calls to commit transactions are there for back-compatibility with some implementations extending the set up methods, in your specific application, they might not be committing anything to the database, but it's not a huge overhead.

Data providers: PHPUnit will run and collect the output of all the data-providers before any test method runs, this is how PHPUnit works to this day.

• WPLoader loads WordPress
• PHPunit runs all data providers, including database queries that will become part of the fixture
• WPTestCase runs set up methods following PHPUnit setup flow, including database queries that will become part of the fixture - let's call this state "fixture"
• now each test case is executed, its setUp methods run and WPTestCase::setUp starts the db transaction
• the test method runs, it will use and make changes to the database state
• the test case tearDown method rolls back the database changes - the db state goes back to "fixture"
• ...

One approach is the Core suite one where tests are written to be very conscious of a mixed db state. E.g. if a test creates a post and other data providers (thus part of the fixture) created posts before, one will not run this:

self::factory()->post->create();
$this->assertCount(1, get_posts());

But this:

$id = self::factory()->post->create();
$this->assertInstanceOf(WP_Post::class, get_post($id));

Or some other form of very discerning and conscious assertion.

What I, personally, prefer is:

• Alter the db only in the setUp method, after the transaction started, i.e. after the parent::setUp() call
• Alter the db in the test methods, after the transaction started
• Use data providers with Closures

While the first two are obvious, the third one might not.

Say I want to snapshot test the HTML of a component when there are no posts, one post, three posts; the following code will not work as I might expect:

<?php

use lucatume\WPBrowser\TestCase\WPTestCase;

class Component_Test extends WPTestCase
{
    public function postsProvider(): array
    {
        return [
            'no posts' => [[]],
            'one post' => [
                [
                    self::factory()->post->create()
                ]
            ],
            'three posts' => [
                [
                    self::factory()->post->create(),
                    self::factory()->post->create(),
                    self::factory()->post->create(),
                ]
            ],
        ];
    }

    /**
     * @dataProvider postsProvider
     */
    public function test_fixture(array $ids): void
    {
        $this->assertCount(count($ids), get_posts());
    }
}

All tests will fail as the fixture contains 4 posts.

I might go the route of creating a test for each number of post, e.g. test_with_no_posts, test_with_one_post and so one, but I like using data providers.

So I just return Closures that will run in the test method, hence a transaction:

<?php

use lucatume\WPBrowser\TestCase\WPTestCase;

class Component_Test extends WPTestCase
{
    public function postsProvider(): array
    {
        return [
            'no posts' => [fn() => []],
            'one post' => [
                fn() => [
                    self::factory()->post->create()
                ]
            ],
            'three posts' => [
                fn() => [
                    self::factory()->post->create(),
                    self::factory()->post->create(),
                    self::factory()->post->create(),
                ]
            ],
        ];
    }

    /**
     * @dataProvider postsProvider
     */
    public function test_fixture(\Closure $fixture): void
    {
        $ids = $fixture();
        $this->assertCount(count($ids), get_posts());
    }
}

Hope this helps you.

[calvinalkan]

Thanks, Luca. That all make/made sense and is how it expect it to work anyways.

Any database change made to before the start of a test method will not happen in a transaction, but will become part of the database fixture.

Correct, this is the correct/expected behavior. I don't know why we need two distinct DB connections for that, though.

It would work the same with one DB connection.

Anything before setUp() is a fixture, anything between setUp() and tearDown() is wrapped in a transaction.

I've fixed this in my BaseIntegrationTestCase and it works like so:

1. Once per process(!), in setUpBeforeClass, I drop all (custom) tables and re-create them.
2. Every test is wrapped in a transaction (setUp/tearDown), all with one DB connection.

I don't ever use dataproviders to create data, just to create classes (sometimes with closures).

The issue that I had is quite clear with the following example.

<?php

use Codeception\TestCase\WPTestCase;

interface CanCountUsers {
    public function countItems() :int;
}

class RepositoryDatabaseWithWPDB implements CanCountUsers {

    private wpdb $db;

    public function __construct(wpdb $db) {
        $this->db = $db;
    }

    public function countItems() :int
    {
        return (int) $this->db->get_var('select count(*) from wp_users');
    }
}

class SomeTestCase extends WPTestCase {

    /**
     * @test
     */
    public function test_that_count_works(CanCountUsers $test_subject) :void
    {
        $this->assertSame(0, $test_subject->countItems());

        wp_insert_user(['user_login' => 'foo', 'user_pass' => 'bar']);

        // THIS WILL ALWAYS FAIL. wp_insert_user was called inside a transaction.
        // BUT IN A DIFFERENT DB CONNECTION.
        $this->assertSame(1, $test_subject->countItems());

        wp_insert_user(['user_login' => 'foo', 'user_pass' => 'bar']);

        $this->assertSame(2, $test_subject->countItems());
    }

    public function implementation () :Generator {
        yield 'db' => [new RepositoryDatabaseWithWPDB($GLOBALS['wpdb'])];
        // more implementations here.
    }

}

I guess it would have worked if I return a closure in the Generator.

But I'd have never thought that the wpdb instance would be different/re-created.

[lucatume]

What version of wp-browser are you using? Or: what version is your fork based on?

The database does not connect twice or reconnect by design or intention.

[calvinalkan]

Based on 3.X

But I checked that this is still present in the latest 3.5.

1. Connection to mysqli is done inside wploader by requiring wp-settings.php. that sets the global wpdb with dbh1.
2. Connection is done inside the WPTestCase Setup. This will now assign dbh2 to the global wpdb instance

https://github.com/lucatume/wp-browser/blob/334cfebd01afe4db317b6f4c99c90fdcfd2d0d18/src/Codeception/TestCase/WPTestCase.php#L92

[lucatume]

Thanks @calvinalkan for the detailed report: I've gone through it a second time and understood where I had understood the issue.

My apologies for dev-splaining: thank you for being graceful in receiving it.

I've applied your suggested change and released it on both main branch (version 4) and v3.5 branch.

[calvinalkan]

No worries.

TBH, I'd have just thrown an exception if the connection went away, because the bug's still there.

Now the error case is just hidden, but the success case works.

[lucatume]

I'd have just thrown an exception if the connection went away, because the bug's still there.

So, change to this:

-        if ( ! $wpdb->check_connection() ) {
+        if ( ! $wpdb->check_connection(false) ) {
            $wpdb->db_connect();
        }

Yours is a fair point. I'm reopening the issue and following up next.

[calvinalkan]

I'd do this:

 -        if ( ! $wpdb->check_connection() ) {
 +        if ( ! $wpdb->check_connection(false) ) {
 +           $this->failTest('The database connection went away. Tests might give false positives/negatives.');
         }

[lucatume]

The wpdb::check_connection method will re-connect to the database if it can; the return value will then be true. That check will pass, no exception/error will be thrown, and the tests will move on.

I've experimented changing the line to

        if ( ! $wpdb->check_connection(false) ) {
            self::fail('The database connection went away. Tests might give false positives/negatives.');
        }

and running this test:

<?php

use lucatume\WPBrowser\TestCase\WPTestCase;

class BreakingTest extends WPTestCase
{
    public static function setUpBeforeClass():void
    {
        global $wpdb;
        $wpdb->close();

        parent::set_up_before_class();
    }

    public function test_something():void{
        $this->assertTrue(true);
    }
}

It will pass without issues.

The thing to check would is the dbh property of the global wpdb object, I can do that with reflection since it's protected.

Changing the code to this:

        if ( empty(Property::readPrivate($wpdb, 'dbh')) ) {
            self::fail('The database connection went away. A `setUpBeforeClassMethod` likely closed the connection.');
        }

throws an error since the connection was closed. I think this might be a good solution to make sure the connection is never dropped during set up of the tests.

[calvinalkan]

What I did is query for the connection ID in before lass, and setup to ensure it does not change during one test run.

That way, you don't have to mess with reflection.

https://dev.mysql.com/doc/refman/8.0/en/information-functions.html#function_connection-id

[lucatume]

I've settled for checking the connection id as suggested (thank you for the suggestion) and adding a new, true by default, beStrictAboutWpdbConnectionId configuration parameter to the WPLoader module configuration.

[calvinalkan]

I've settled for checking the connection id as suggested (thank you for the suggestion) and adding a new, true by default, beStrictAboutWpdbConnectionId configuration parameter to the WPLoader module configuration.

That sounds reasonable. Cheers.