berlindb / core

All of the required core code
MIT License
254 stars 28 forks source link

Getting mysql db table errors when dropping tables while unit testing #64

Closed alexstandiford closed 3 years ago

alexstandiford commented 4 years ago

If you try to drop a table using Table::drop inside of a WordPress unit test case, you will receive an error something like this:

WordPress database error: [Unknown table 'wordpress_test.wptests_table_name']
DROP TEMPORARY TABLE wptests_table_name

It's entirely possible this is just a bug in what I've built, but wanted to see if others are experiencing this.

When I add IF EXISTS to the end of the drop statement, these errors go away. I'm not certain this is the best way to handle this, however, since in my context I usually drop a table to reset the table back to the default settings. So I suppose my questions are:

  1. Is there a more-appropriate way to reset a database table?
  2. Is there any reason why we should not add IF EXISTS to the drop table syntax?
  3. Is there some logic we should introduce that works around MySQL transactions?

I did a small amount of digging, and found this: https://wordpress.stackexchange.com/a/220308/105777

You've just discovered an important feature of the core test suite: it forces any tables created during the test to be temporary tables.

If you look in the WP_UnitTestCase::setUp() method you'll see that it calls a method called start_transaction(). That start_transaction() method starts a MySQL database transaction:

    function start_transaction() {
            global $wpdb;
            $wpdb->query( 'SET autocommit = 0;' );
            $wpdb->query( 'START TRANSACTION;' );
            add_filter( 'query', array( $this, '_create_temporary_tables' ) );
            add_filter( 'query', array( $this, '_drop_temporary_tables' ) );
    }

It does this so that any changes that your test makes in the database can just be rolled back afterward in the tearDown() method. This means that each test starts with a clean WordPress database, untainted by the prior tests.

However, you'll note that start_transaction() also hooks two methods to the 'query' filter: _create_temporary_tables and _drop_temporary_tables. If you look at the source of these methods, you'll see that they cause any CREATE or DROP table queries to be for temporary tables instead:

    function _create_temporary_tables( $query ) {
            if ( 'CREATE TABLE' === substr( trim( $query ), 0, 12 ) )
                    return substr_replace( trim( $query ), 'CREATE TEMPORARY TABLE', 0, 12 );
            return $query;
    }

    function _drop_temporary_tables( $query ) {
            if ( 'DROP TABLE' === substr( trim( $query ), 0, 10 ) )
                    return substr_replace( trim( $query ), 'DROP TEMPORARY TABLE', 0, 10 );
            return $query;
    }

The 'query' filter is applied to all database queries passed through $wpdb->query(), which dbDelta() uses. So that means when your tables are created, they are created as temporary tables.

So in order to list those tables, I think you'd have to show temporary tables instead: $sql = "SHOW TEMPORARY TABLES LIKE '%'";

Update: MySQL doesn't allow you to list the temporary tables like you can with the regular tables. You will have to use another method of checking if the table exists, like attempting to insert a row.

But why does the unit test case require temporary tables to be created in the first place? Remember that we are using MySQL transactions so that the database stays clean. We don't want to ever commit the transactions though, we want to always roll them back at the end of the test. But there are some MySQL statements that will cause an implicit commit. Among these are, you guessed it, CREATE TABLE and DROP TABLE. However, per the MySQL docs:

CREATE TABLE and DROP TABLE statements do not commit a transaction if the TEMPORARY keyword is used.

So to avoid an implicit commit, the test case forces any tables created or dropped to be temporary tables.

This is not a well documented feature, but once you understand what is going on it should be pretty easy to work with.

ashleyfae commented 4 years ago

Not sure if this is the best way, but I do this at the start of my unit tests and it works well for me:

foreach ( restrict_content_pro()->components as $component ) {
    $table = $component->get_interface( 'table' );

    if ( $table instanceof \RCP\Database\Table ) {
        if ( $table->exists() ) {
            $table->uninstall();
        }

        $table->install();
    }
}
alexstandiford commented 4 years ago

This is basically what in doing, too. Do you see the error I described above?

JJJ commented 4 years ago
  1. Is there a more-appropriate way to reset a database table?

How about truncate() or delete_all()? :zoidberg:

  1. Is there any reason why we should not add IF EXISTS to the drop table syntax?

IF EXISTS would not generate this error. 😇

  1. Is there some logic we should introduce that works around MySQL transactions?

Probably not.

Transactions could always happen anywhere outside of Berlin, so there is no way to know if it's wrapped in one or why. In the case that you've found, it's explicitly making sure tables are temporary.

alexstandiford commented 4 years ago

How about truncate() or delete_all()? :zoidberg:

I suppose my reasoning for drop and create is that I wanted the table to completely reset instead of simply removing all of the data.

Judging by what @ashleyfae said above, it seems like this is probably specific to my use case, and is probably indicative of something amiss with my specific setup.

If nobody else is experiencing this, we can probably close this.

ashleyfae commented 4 years ago

I've never gotten the error before. I can double check again later but pretty confident I haven't seen it.

JJJ commented 3 years ago

No movement in a few months, so I'm closing just to keep open issues to a minimum.

Please reopen anytime if needed, and thank you everyone for the discussion 🥳