WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#35249 new defect (bug)

Unit tests should pass

Reported by: jrf Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: Cc:

Description

The unit tests are no longer passing on the master and by the looks of it, haven't been for a while.

Ran the tests on two different setups with similar results, see summary screenshots below.

I haven't look into the various issues in depth, though one set of errors is clearly to do with either the core function(s) or the test(s) not taking MySql 5.0 into account (which is still the minimum requirement).

Additional info:

  • Tests run with the standard phpunit settings (+ logging enabled).
  • Before I ran the tests on either setup, I cleared out the database and fetched the latest commits.

Log files attached In the attached zip file are the phpunit log files for both test runs, including a copy of the phpunit.xml file used.

To view the log files in human readable format, just open the html file (only the errors and failures will be shown).

http://snag.gy/yg74g.jpg http://snag.gy/VFvyG.jpg

Attachments (4)

WP-Unit-tests-failing-logfiles.zip (161.5 KB) - added by jrf 2 years ago.
WP-Unit-tests-failing-logfiles-rev-36116.zip (347.2 KB) - added by jrf 2 years ago.
Renewed logfiles
mysql5.6.17 my.ini (5.3 KB) - added by jrf 2 years ago.
mysql5.6.17 show_variables.txt (13.3 KB) - added by jrf 2 years ago.
Output of SHOW VARIABLES

Download all attachments as: .zip

Change History (25)

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


2 years ago

#2 @pento
2 years ago

In 36116:

Tests: When testing the utf8mb4 charset, ensure that the current MySQL server has utf8mb4 support.

See #35249.

@jrf
2 years ago

Renewed logfiles

#3 @jrf
2 years ago

Ran the tests again against trunk on commit https://core.trac.wordpress.org/browser/trunk?rev=36116 - thanks @pento - with some additional log formats turned on for @dossy ;-)

Attached screenshots & the newly uploaded zip file give the status after @pento's commit.

The MySql 5.0 errors are gone, but in the PHP5.6 run there is one new failure (maybe to do with commit 36115 ?).

http://snag.gy/mZFjD.jpg http://snag.gy/XuW4m.jpg

#4 @dd32
2 years ago

In 36118:

Tests: After [36100] use an object style which is compatible with PHP5 get_object_vars().

Merges [36117] to the 4.4 branch.
See #35249.
Fixes #35058.

#5 @pento
2 years ago

I'm unable to reproduce the Tests_dbDelta failures against my own install of MySQL 5.6.17. @jrf, could you please post your my.cnf file (with any private information redacted, of course)? There may be a MySQL setting that is changing the behaviour.

@jrf
2 years ago

#6 follow-up: @jrf
2 years ago

@pento Uploaded. AFAIR I never changed anything, left it as it came with WampServer, so should be quite standard.

#7 in reply to: ↑ 6 @dossy
2 years ago

Replying to jrf:

@pento Uploaded. AFAIR I never changed anything, left it as it came with WampServer, so should be quite standard.

@jrf, like @pento, I couldn't reproduce the failures on out-of-the-box generic MySQL 5.6.17, PHP 5.5.9, PHPUnit 4.3.5.

Could you stop MySQL, delete your MySQL error log, restart MySQL, then re-run your tests, then attach a copy of the error log for us?

You can find the path to your error log by asking MySQL:

SHOW VARIABLES LIKE 'log_error';

#8 @jrf
2 years ago

@pento Will do, though there hasn't been anything in the MySql error logs so far. Any particular error logging option you want me to turn on ? My current my.cnf file is attached here.

#9 follow-up: @jrf
2 years ago

@dossy Sorry, the previous message was meant for you.

Anyway, I'd been running the tests in the background without keeping an eye on it, but now noticed some errors scrolling on the screen. So I had a quick look at the PHP error log and kind of think we have a winner:

Don't know why the messages are showing up in french, but it basically translates to the key being too long, max key length 1000 and then the second one saying the table doesn't exist

[29-Dec-2015 18:16:18 UTC] WordPress database error La clé est trop longue. Longueur maximale: 1000 for query 
			CREATE TABLE wptests_dbdelta_test (
				id bigint(20) NOT NULL AUTO_INCREMENT,
				column_1 varchar(255) NOT NULL,
				PRIMARY KEY  (id),
				KEY key_1 (column_1),
				KEY compoud_key (id,column_1),
				FULLTEXT KEY fulltext_key (column_1)
			) ENGINE=MyISAM
			 made by PHPUnit_TextUI_Command::main, PHPUnit_TextUI_Command->run, PHPUnit_TextUI_TestRunner->doRun, PHPUnit_Framework_TestSuite->run, PHPUnit_Framework_TestSuite->run, PHPUnit_Framework_TestCase->run, PHPUnit_Framework_TestResult->run, PHPUnit_Framework_TestCase->runBare, Tests_dbDelta->setUp

[29-Dec-2015 18:16:18 UTC] WordPress database error La table 'wptestsuite.wptests_dbdelta_test' n'existe pas for query select column_1 from wptests_dbdelta_test where column_1 = 'wcphilly2015' made by PHPUnit_TextUI_Command::main, PHPUnit_TextUI_Command->run, PHPUnit_TextUI_TestRunner->doRun, PHPUnit_Framework_TestSuite->run, PHPUnit_Framework_TestSuite->run, PHPUnit_Framework_TestCase->run, PHPUnit_Framework_TestResult->run, PHPUnit_Framework_TestCase->runBare, PHPUnit_Framework_TestCase->runTest, ReflectionMethod->invokeArgs, Tests_dbDelta->test_insert_into_table, Tests_dbDelta->assertTableRowHasValue

/cc @pento

Last edited 2 years ago by jrf (previous) (diff)

#10 in reply to: ↑ 9 @dossy
2 years ago

@jrf, the errors are in French because that's the locale you set in your my.cnf on line 43:

lc-messages=fr_FR

The corresponding error string in English for the one you showed us:

"Specified key was too long. Max key length is %d",

I still don't know how you're getting that error based on your my.cnf and other environment specifics, though.

Could you attach a copy of the output of SHOW VARIABLES so I can take a look and see if there's a setting that's causing this?

@jrf
2 years ago

Output of SHOW VARIABLES

#11 @jrf
2 years ago

the errors are in French because that's the locale you set in your my.cnf on line 43

@dossy Just goes to show that I never changed anything in the file. That's clearly how it came with WAMPServer in that case.

I've uploaded a text file with the results. Had a quick look through it and can't see anything obvious myself, it all looks quite standard to me. Hope your expert eye will see more ;-)

#12 @dossy
2 years ago

@jrf - I see version_compile_machine = x86 not x86_64 -- are you running a 32-bit OS and/or 32-bit version of MySQL?

#13 @jrf
2 years ago

@dossy That's correct. I am on a 64-bit machine, but for backward compatibility/version-switching installed the WAMP suite & numerous extra PHP/MySQL versions as 32-bit.

#14 @dossy
2 years ago

I'm using the MySQL-provided binaries for 5.6.17, but looking at the source it defines MI_MAX_KEY_LENGTH to be 1000 ... and the comment in the source says "The following defines can be increased if necessary." - unfortunately there's no way at runtime to find out what the compile-time value was set to, so I can't know if the binaries I'm using have it set larger, or if there's something else at play here.

Last edited 2 years ago by dossy (previous) (diff)

#15 @dossy
2 years ago

Actually, I figured out exactly how to determine what the value is at runtime, just trigger the error... duh:

mysql> CREATE TABLE t (a VARCHAR(1000), b VARCHAR(1000), c VARCHAR(1000), d VARCHAR(1000), KEY (a, b, c, d)) ENGINE=MyISAM;
ERROR 1071 (42000): Specified key was too long; max key length is 1000 bytes

So, interesting:

> DROP TABLE IF EXISTS t; CREATE TABLE t (a VARCHAR(1), b VARCHAR(1001), KEY (b)) ENGINE=MyISAM;
Query OK, 0 rows affected (0.00 sec)

Query OK, 0 rows affected, 1 warning (0.00 sec)

> SHOW WARNINGS;
+---------+------+----------------------------------------------------------+
| Level   | Code | Message                                                  |
+---------+------+----------------------------------------------------------+
| Warning | 1071 | Specified key was too long; max key length is 1000 bytes |
+---------+------+----------------------------------------------------------+
1 row in set (0.00 sec)

> DROP TABLE IF EXISTS t; CREATE TABLE t (a BIGINT, b VARCHAR(1000), KEY (a, b)) ENGINE=MyISAM;
Query OK, 0 rows affected, 1 warning (0.00 sec)

ERROR 1071 (42000): Specified key was too long; max key length is 1000 bytes

So, for simple (single-column) indexes, MySQL treats exceeding the MI_MAX_KEY_LENGTH as a warning, and only uses the max length as a prefix index. But, for compound (multi-column) indexes, it elevates it to an error which prevents the table from being created.

What I don't get is why your MySQL croaked when 255 + 20 = 275, nowhere near the 1000 limit. On my system, it does the math right:

> DROP TABLE IF EXISTS t; CREATE TABLE t (a BIGINT(20), b VARCHAR(992), KEY (a, b)) ENGINE=MyISAM;
Query OK, 0 rows affected, 1 warning (0.00 sec)

Query OK, 0 rows affected (0.00 sec)

> DROP TABLE IF EXISTS t; CREATE TABLE t (a BIGINT(20), b VARCHAR(993), KEY (a, b)) ENGINE=MyISAM;
Query OK, 0 rows affected (0.00 sec)

ERROR 1071 (42000): Specified key was too long; max key length is 1000 bytes

BIGINT is 8 bytes, so 992 + 8 = 1000, as expected. Oh, wait ...

> DROP TABLE IF EXISTS t; CREATE TABLE t (a BIGINT(20), b VARCHAR(992) CHARACTER SET utf8mb4, KEY (a, b)) ENGINE=MyISAM;
Query OK, 0 rows affected, 1 warning (0.00 sec)

ERROR 1071 (42000): Specified key was too long; max key length is 1000 bytes

> DROP TABLE IF EXISTS t; CREATE TABLE t (a BIGINT(20), b VARCHAR(255) CHARACTER SET utf8mb4, KEY (a, b)) ENGINE=MyISAM;
Query OK, 0 rows affected, 1 warning (0.00 sec)

ERROR 1071 (42000): Specified key was too long; max key length is 1000 bytes

> DROP TABLE IF EXISTS t; CREATE TABLE t (a BIGINT(20), b VARCHAR(248) CHARACTER SET utf8mb4, KEY (a, b)) ENGINE=MyISAM;
Query OK, 0 rows affected, 1 warning (0.00 sec)

Query OK, 0 rows affected (0.00 sec)

> DROP TABLE IF EXISTS t; CREATE TABLE t (a BIGINT(20), b VARCHAR(249) CHARACTER SET utf8mb4, KEY (a, b)) ENGINE=MyISAM;
Query OK, 0 rows affected (0.00 sec)

ERROR 1071 (42000): Specified key was too long; max key length is 1000 bytes

And, there it is. My character_set_database = latin1 and I bet yours is set to utf8mb4. You can confirm with SHOW CREATE DATABASE <dbname>.

#16 @dossy
2 years ago

Oh, and to confirm: if I change the default character set of my wordpress_unit_tests database to utf8mb4 ... tests/phpunit/tests/dbdelta.php winds up with:

FAILURES!
Tests: 9, Assertions: 11, Failures: 7.

which matches the test results you observed.

#17 @jrf
2 years ago

@dossy Sounds like you've figured it out. Your explanation makes total sense. And yes, on the MySql 5.6.17 install my default charset is utf8mb4.

As the 1000 limit is standard and the utf8mb4 charset quite common, I guess the DbDelta functions can do with some reviewing ? Or is this really something which could only occur in the tests ? If so, I'd say that those tests are testing a non-realistic situation and it makes me wonder if the realistic situation is being tested properly.

#18 @dossy
2 years ago

@jrf - I suppose the question is whether or not it's a good idea to have the default character set on a database set to utf8mb4 - do all of the text columns really need to use that character set? Or, if we're going to say "it's 2015, we should be able to shove utf8mb4 in any text column" then we probably should be defining our database schema to use more conservative prefix indexes on text columns that aren't FULLTEXT indexes.

While InnoDB allows prefix indexes of up to 3072 bytes using the innodb_large_prefix setting, it was only added to MySQL in 5.5, and FULLTEXT indexes since 5.6, but WordPress supports MySQL back to 5.0, so using 5.5+ and 5.6+ only features are probably a no-no.

I'm curious, how did your test database get its default character set changed to utf8mb4 anyway? That's definitely unusual, otherwise these tests would have failed for more people ...

#19 @dossy
2 years ago

It would be dirty, but we could force the column's character set in tests/phpunit/tests/dbdelta.php for the sake of the test:

 28     // Forcing MyISAM, because InnoDB only started supporting FULLTEXT indexes in MySQL 5.7.
 29     // Forcing latin1, in case character_set_database is multi-byte.
 30     $wpdb->query(
 31       "
 32       CREATE TABLE {$wpdb->prefix}dbdelta_test (
 33         id bigint(20) NOT NULL AUTO_INCREMENT,
 34         column_1 varchar(255) CHARACTER SET latin1 NOT NULL,
 35         PRIMARY KEY  (id),
 36         KEY key_1 (column_1),
 37         KEY compoud_key (id,column_1),
 38         FULLTEXT KEY fulltext_key (column_1)
 39       ) ENGINE=MyISAM
 40       "
 41     );

But, that might lead to some subtle false positives (test that pass, but should fail).

Maybe just adding a check to the setUpBeforeClass() that checks the character_set_database (using SHOW CHARACTER SET LIKE 'name''s Maxlen value, if it's greater than 1) and raises an exception that the tests WILL fail because of the database default character set? This way, if and when others get these same test failures, they aren't left scratching their heads as to why the tests fail for them but not everyone else ...

#20 follow-up: @jrf
2 years ago

@dossy

I suppose the question is whether or not it's a good idea to have the default character set on a database set to utf8mb4 - do all of the text columns really need to use that character set? Or, if we're going to say "it's 2015, we should be able to shove utf8mb4 in any text column" then we probably should be defining our database schema to use more conservative prefix indexes on text columns that aren't FULLTEXT indexes.

I think the *real* question is: does WP want to be compatible with all possible configurations out there ? So far in my experience, the answer to this has always been "Yes". If so, IMHO, it's not an issue of the test failing, but of WP failing to deal correctly with the possibility that there are databases with utf8mb4-like collations out there.

I'm curious, how did your test database get its default character set changed to utf8mb4 anyway? That's definitely unusual, otherwise these tests would have failed for more people ...

I'm not sure.

It would be dirty, but we could force the column's character set in tests/phpunit/tests/dbdelta.php for the sake of the test: Maybe just adding a check to the setUpBeforeClass() that checks the character_set_database (using SHOW CHARACTER SET LIKE 'name''s Maxlen value ...

I don't think fixing the test is the way to go.

What about this: if the DB_CHARSET constant is set to utf-8, in the WP_INSTALL routine - before adding any tables -, check the DB charset with:

USE `database_name`
SELECT @@character_set_database, @@collation_database

If the charset is not utf8, run:

ALTER DATABASE `database_name` CHARACTER SET `utf8` COLLATE `utf8_general_ci`

That would prevent the issue altogether without causing issues for people who have the DB_CHARSET set to something other than utf8.

#21 in reply to: ↑ 20 @dossy
2 years ago

Replying to jrf:

What about this: if the DB_CHARSET constant is set to utf-8, in the WP_INSTALL routine - before adding any tables -, check the DB charset with:

USE `database_name`
SELECT @@character_set_database, @@collation_database

If the charset is not utf8, run:

ALTER DATABASE `database_name` CHARACTER SET `utf8` COLLATE `utf8_general_ci`

That would prevent the issue altogether without causing issues for people who have the DB_CHARSET set to something other than utf8.

I suppose this should be filed as a new ticket, referencing this ticket for detailed background rationale, and let the community decide if they collectively think it's a good idea?

I'm not keen on the idea of the installer changing the character set and collation on an already existing database that's being installed into. DB_CHARSET controlling the charset/collation of a database that's being created as part of the WP installation, I'm all for. But, if I intentionally created a database OR (regardless of whether it's a bad idea or not) want to install WP into a previously existing database, and it quietly changed the default charset/collation of the database out from underneath me? That would be really bad, and would break assumptions made by other users of that database.

But, if everyone else thinks it's a good idea, I guess that's what really matters ... so, put it up to the community with a new ticket?

Last edited 2 years ago by dossy (previous) (diff)
Note: See TracTickets for help on using tickets.