WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#39327 closed defect (bug) (fixed)

Database connection errors in unit tests on 4.7

Reported by: rmccue Owned by: pento
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: Build/Test Tools Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Between 4.6 and 4.7, wpdb somehow broke, with "Couldn't fetch mysqli" being thrown.

Tracked this down a bit with @pento; it turns out to be a problem with PHPUnit's global backups, and can be fixed by setting protected $backupGlobals = true in the test class. The key is that PHPUnit serialises the data and unserialises it to restore it. Adding this to wpdb also fixes it:

public function __wakeup() {
	$this->db_connect();
}

As @pento mentioned, this is somewhat known behaviour; wpdb isn't designed to be serialized and unserialized. The question is not why this error occurs, but rather: what changed between 4.6 and 4.7?

There were no relevant changes to wpdb, and running 4.6 with a 4.7-ified wp-db.php does not cause the error, so the error is somewhere else.

Will try and track this down. I'm experiencing it, and it has been independently reported on the forums too.

Attachments (1)

39327.diff (781 bytes) - added by pento 3 months ago.

Download all attachments as: .zip

Change History (10)

This ticket was mentioned in Slack in #core by pento. View the logs.


3 months ago

@pento
3 months ago

#2 @pento
3 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.7.1
  • Owner set to pento
  • Status changed from new to assigned

In Core tests, phpunit.xml.dist has backupGlobals="false" defined, hence why we never see this in Core.

For plugin tests that don't have this option set to false, $wpdb was always being serialised/unserialised between tests, but WP_UnitTestCase::setUp() took care of reconnecting. The changes to WP_UnitTestCase::setUpBeforeClass() in [38398] introduced DB interactions before the database was reconnected, however, causing this warning to appear during plugin tests, but not Core tests.

For an immediate plugin fix, the best option is to add backupGlobals="false" to your phpunit.xms.dist.

@rmccue: Could you also confirm that 39327.diff fixes the warning for you? If so, you'll need to remove the blame-gary tag on this one. ;-)

#3 @rmccue
3 months ago

  • Keywords commit added; blame-gary removed

Confirmed; this fix changes it, and that also explains why core doesn't run into it.

Also confirming this is not a blame-gary situation, but it continues to make sense to assume that in the future.

#4 @pento
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39626:

Tests: Restore the database connection earlier when switching test groups.

When plugins don't disable the backupGlobals PHPUnit option in their own tests, $wpdb is backed up and restored between classes of tests. The serialisation process used for this broke the database connection. This previously wasn't a problem, as it was reconnecting before each test.

[38398] introduced some changes that required the connection to be available in setUpBeforeClass(), earlier than in was previously reconnecting. This didn't cause warnings in Core, but it did cause warnings for plugins that don't disable the backupGlobals option.

The database connection now reconnects in setUpBeforeClass(). This change also fixes a few Core tests that weren't calling parent::setUpBeforeClass() or parent::tearDown() correctly.

Fixes #39327.

#5 @pento
3 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 4.7 branch.

#6 @pento
3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 39627:

Tests: Restore the database connection earlier when switching test groups.

When plugins don't disable the backupGlobals PHPUnit option in their own tests, $wpdb is backed up and restored between classes of tests. The serialisation process used for this broke the database connection. This previously wasn't a problem, as it was reconnecting before each test.

[38398] introduced some changes that required the connection to be available in setUpBeforeClass(), earlier than in was previously reconnecting. This didn't cause warnings in Core, but it did cause warnings for plugins that don't disable the backupGlobals option.

The database connection now reconnects in setUpBeforeClass(). This change also fixes a few Core tests that weren't calling parent::setUpBeforeClass() or parent::tearDown() correctly.

Merges [39626] to the 4.7 branch.

Fixes #39327.

#7 @mnelson4
3 months ago

This is causing some of our plugin's phpunit tests to fail. We were already setting backupGlobals="false" in our phpunit.xml file. Haven't nailed down the issue yet though, so still investigating.

Reverting the first commit on this ticket fixes our plugin's unit tests.

Also, adding

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

to the start of our test classes' setUp() method fixese it too. Not sure why those changes resolve the issue yet

Last edited 3 months ago by mnelson4 (previous) (diff)

#8 @jdgrimes
3 months ago

I think this also caused issues in one of my plugin's unit tests. I have backupGlobals set to false, too. The errors were like:

WordPress Database Error: Table 'wptests_test' already exists [CREATE TEMPORARY TABLE `wptests_test` ( `id` BIGINT );]

Which was coming from some tests where a table gets created. Previously, the table would be automatically dropped after the test was done, because it was temporary. Then it could be created again as part of the next test. However, now apparently a single DB session is being used throughout the tests, which means that the temporary tables persist between tests now. The fix is to either drop the tables manually, or else reset the connection in tearDown() ($wpdb->close(); $wpdb->db_connect()). (Calling $wpdb->db_connect() in setUp() as @mnelson4 is doing would probably also work.)

#9 @mnelson4
3 months ago

+1 to @jdgrimes 's fix.
That turned out to be exactly our issue too. We had temporary tables that weren't getting dropped explicitly after each test (only implicitly by the database session ending). So now we're explicitly dropping those temporary tables after each test during tearDown and it works.

Note: See TracTickets for help on using tickets.