Faster and phpunit runner-compatible unit tests

[nbachiyski]

Currently the WordPress unit-tests use a custom test runner and most of the test cases replace all the data in the database tables before they run.

Since we are using PHPUnit, we should use the phpunit test runner. This gives us: in-editor test running, CI, easier creation of more fine-grained suites. All these will make running the testt easier, which is a good step towards more people running them.

Replacing the data before each testcase run is often slow. The data isn't replaced after each test withing the case, though, which is highly error-prone, because we need to account for any changes of the data made during the tests.

Here is my take on solving these problems:

• Create a very simple PHPUnit test suite, which loads all test classes from the tests directory.
• Install WordPress once, before running any tests.
• Make all the tables InnoDB.
• In the default WordPress test case class start a transaction on setUp.
• ROLLBACK all the changes on tearDown.
• Reinitalize some global variables and clean cache on setUp.

This way we install WordPress only once (fast) and at the same time we don't need to care about the changes we make during each test (not error prone).

This approach, however, doesn't favor having prepared datasets in advance (shared fixtures). Which isn't bad. I strongly believe that shared fixtures more harm than good. They create unneeded dependencies between data, make test unclear, because the data lives far away, and are very sensitive to change, because many tests use the same data. If the tests are small enough (they should) fresh fixtures won't slow the test, the data will be in the test itself and no external data change would break them.

In order to implement the test class and suite, a couple of changes to core were needed:

• WordPress is loaded in a function call (that's how phpunit loads tests), so some variables need to be explictly initialized as globals (I guess there are more, which will be revaled after more test coverage).
• Some filters were needed in order to be able to make the tables InnoDB (easier to implement than changing the enging after creation).

The patch is attached.

The test case class and runner code is at ​https://github.com/nb/wordpress-tests (the current repo doesn't have branch structure, otherwise I would have put it there).

[nacin]

Loading WordPress in a function call will break plugins and themes. Eventually we may want them to be able to leverage the test suite. Or maybe not. But I'm not comfortable with it as it may make some tests more difficult.

Can we move this over to there UT Trac and close this?

[nbachiyski]

Replying to nacin:

Loading WordPress in a function call will break plugins and themes. Eventually we may want them to be able to leverage the test suite. Or maybe not. But I'm not comfortable with it as it may make some tests more difficult.

The patch only explicitly states what is already implied by WordPress – that some of the variables are global. It doesn't force WordPress to load inside a function call all the time.

What aren't you comfortable with? Which test will be made more difficult?

[ocean90]

If still relevant, someone should open a new ticket ​here.

[nbachiyski]

In [18531]:

Add filters for install/upgrade queries, so that unit tests installer can force creating InnoDB tables, so that we can use transactions to revert the database to its initial state after each test. See #17749

[nbachiyski]

In [18532]:

Explicitly globalize some variables, so that unit tests can run WordPress inside a function. Fixes #17749

[scribu]

explicitly globalize wp_locale

[scribu]

Getting an error when running phpunit for a plugin:

PHP Fatal error:  Call to a member function is_rtl() on a non-object in /home/cristi/www/localhost/wp/core/wp-includes/locale.php on line 348

Fixed in 17749.diff​ by explicitly globalizing $wp_locale.

[nacin]

In [18756]:

Explicitly globalize wp_locale for the UT framework. props scribu, fixes #17749.

[nbachiyski]

We should globalize all variables in wp-settings.php. See:

​http://glotpress.trac.wordpress.org/browser/trunk/gp-settings.php#L239

[mrtorrent]

A couple other issues that cause breakages when including Wordpress a scope other than global:

• $table_prefix in wp-config.php is not explicitly globalised, resulting in queries on non-existent tables if a prefix is used
• $PHP_SELF is used as a global in load.php and vars.php, but not explicitly globalised, resulting in errors

I am attaching a patch to fix these.

[scribu]

Changing wp-config-sample.php just won't cut it. We have to find an alternative that doesn't involve everyone modifying their wp-config.php file.

[scribu]

We can probably live with plugins using the PHP_SELF global not work when running unit tests.

[mrtorrent]

@scribu good point. The only alternative that I can see would be adding $GLOBALS['table_prefix'] = $table_prefix; before wp_set_wpdb_vars(); in wp-settings.php. How does that sound?

[mrtorrent]

Forgot to say, here's a Git branch in case it's useful to anyone: ​https://github.com/mrtorrent/WordPress/tree/explicitly_set_globals

[scribu]

If it works, it's good enough.

One more thing: why wouldn't $PHP_SELF work? it's explicitly globalized in wp_fix_server_vars().

[nacin]

Replying to scribu:

If it works, it's good enough.

One more thing: why wouldn't $PHP_SELF work? it's explicitly globalized in wp_fix_server_vars().

Because in vars.php, it's used in global (therefore local) scope.

I'm fine with just no longer using $PHP_SELF. I imagine that's some weird back compatible thing, but no worries if it's not available when running WP inside of a function.

[mrtorrent]

Replying to nacin:

I'm fine with just no longer using $PHP_SELF. I imagine that's some weird back compatible thing, but no worries if it's not available when running WP inside of a function.

Yeah, $PHP_SELF has been deprecated for years, and since Wordpress now states that it requires PHP 5.2 at a minimum, I thought this would be a safe switch.

[scribu]

Ok, I'm convinced.

[GaryJ]

The patch continues to use $_SERVER["REQUEST_URI"] when it would seem a good opportunity to switch to $_SERVER['REQUEST_URI'] for general consistency, as when using single- vs double-quoted strings.

[mrtorrent]

Explicitly globalise $table_prefix, fix globalisation of $PHP_SELF by using $_SERVER['PHP_SELF'] instead. Fixes issues with including Wordpress outside the global scope.

[mrtorrent]

Replying to GaryJ:

The patch continues to use $_SERVER["REQUEST_URI"] when it would seem a good opportunity to switch to $_SERVER['REQUEST_URI'] for general consistency, as when using single- vs double-quoted strings.

Done.

May we have a commit? :)

[ryan]

wp_fix_server_vars() should still fix $PHP_SELF for plugins that use it.

[mrtorrent]

$PHP_SELF is deprecated and doesn't usually exist (i.e. if register_globals is off, as it should be). Plugins shouldn't be using it. I don't think it's reasonable for Wordpress to try to provide a compatibility layer for plugins using old PHP code, do you?

[ryan]

Depends on how many plugins it breaks.

[mrtorrent]

I think it's silly and detrimental to WordPress to try to extend the life of PHP4-based plugins, but I don't really care, so if you want to keep $PHP_SELF in, by all means go for it. You'd just need to change line 89 back:

$_SERVER['PHP_SELF'] = $PHP_SELF = preg_replace( '/(\?.*)?$/', '', $_SERVER["REQUEST_URI"] );

The important thing is to get the rest of the patch committed, because it makes a big difference: globalising these variables properly means that not only can WordPress be run in PHPUnit tests, but also that the API can be loaded and used by external applications to integrate with WordPress. Surely that's a hell of a win?

I've written ​an integration with Symfony 2's authentication layer, for example, but it can't work if WordPress just assumes that it is in the global scope and bombs out otherwise.

[ryan]

$PHP_SELF is used a lot in plugins. Therefore it must stay.

[nacin]

In [18993]:

Explicitly globalize $table_prefix in wp-settings.php in case WordPress isn't loaded in global scope. Use $_SERVERPHP_SELF?, not $PHP_SELF. We need to keep $PHP_SELF for backwards compatibility reasons (many, many plugins rely on it). props mrtorrent, fixes #17749.

[nacin]

In [18994]:

Globalize everything in vars.php. props duck_. fixes #17749.

[mrtorrent]

Explicitly globalise $wp_rewrite

[mrtorrent]

$wp_rewrite needs to be globalised; here's the patch

[westi]

Replying to mrtorrent:

$wp_rewrite needs to be globalised; here's the patch

Please open a new ticket for this as it is too late for 3.3 and this is the ticket for this issue in 3.3

[mrtorrent]

Why? This is an issue in something that is targeted for 3.3, doesn't it make sense to fix it? Not only that, but it is zero-impact in terms of the rest of WordPress.

[nacin]

Replying to mrtorrent:

Not only that, but it is zero-impact in terms of the rest of WordPress.

I think you just made his argument for him. :)

[mrtorrent]

I understand what you're saying, but this is bad code that should be fixed -- should have been fixed a long time ago. I'm really disappointed that the first contributions I've made to WordPress have been met with such reluctance, when I thought I was just chipping in a few small, non-controversial fixes that would help up the standard of the code.

It seems like a fear of breaking anything, which comes from a lack of thorough test coverage to give people peace of mind when they make a change, and which would make WordPress a more robust piece of software. But that is being prevented by bad code like this which has been hanging around for a long time and was written by people who didn't know better at the time. And that was the point of this ticket in the first place -- taking a significant step towards improving testing of WordPress by making some simple but important, non-breaking changes.

If this isn't a top goal for WordPress at this stage in its life, then I don't know where your priorities could be. Manual testing and clinging to an aged codebase is not healthy.

[dd32]

I understand what you're saying, but this is bad code that should be fixed

There's no arguments that the patch isn't worthy. However, In order to get WordPress releases out the door, there are certain stages which are followed - and we're in the RC(Release Candidate) stage right now. Put simply: Unless it's a bug in a new functionality, or introduced in 3.3 (ie. a regression from 3.2), It can wait for the next release.

The priorities here, are simply, to follow the commit guidelines which the developers are working by - Start making exceptions, and it's a fast question to "What do we make exceptions for" - and that question has already been answered by the commit guidelines: Nothing.

As WordPress works as it is, and this ticket is "completed" (as far as possible) for 3.3, Any further work can be done in a future release, which itself, requires a new ticket - Please feel free to make one and reference this ticket (If someone else doesn't do it first)

[nacin]

Replying to mrtorrent:

I'm really disappointed that the first contributions I've made to WordPress have been met with such reluctance, when I thought I was just chipping in a few small, non-controversial fixes that would help up the standard of the code.

I'm sorry your first contribution fell by the wayside. We'd love to see more of them. But once we hit release candidate stage, as dd32 has explained, we have strict rules about what can/should and cannot/shouldn't be committed to core. I opened #19456 which will be committed once development for 3.4 opens.

It seems like a fear of breaking anything, which comes from a lack of thorough test coverage to give people peace of mind when they make a change, and which would make WordPress a more robust piece of software.

But that is being prevented by bad code like this which has been hanging around for a long time and was written by people who didn't know better at the time.

I think there are plenty of examples of bad code in WordPress that bind our hands for future development, but this line is not one of them.

Manual testing and clinging to an aged codebase is not healthy.

You're preaching to the choir.

[mrtorrent]

I understand the release process and that you have one that works for you, and I do see where you're coming from, as I'm sure you see where I'm coming from in my frustration that such an obviously non-breaking, one-line fix could be turned away at any time. Anyway, hopefully this and other improvements will go in next time.

I think there are plenty of examples of bad code in WordPress that bind our hands for future development, but this line is not one of them.

It's symptomatic of bigger issues -- of course any old, crusty code like this holds you back.

You're preaching to the choir.

Then why is the code still in the state it's in?

[nacin]

In [19603]:

Globalize wp_rewrite in wp-settings. props mrtorrent, fixes #17749.

[nbachiyski]

Here are some more. I hit those, because in a plugin I was checking the WordPress version and it kept saying it's too old.

[dd32]

Attachment globalize-version-vars.diff​ added

We can't do that, as we use version.php in "local" scope in certain functions.

Since this ticket was closed on a completed milestone, please open a new ticket and reference this one instead, including exactly what's breaking and requiring the globalisation.

[nbachiyski]

dd32, you're right on both points. I will open a ticket and I will fix the local $wp_version usage.

For context on what's breaking, it's in the ticket description above:

WordPress is loaded in a function call (that's how phpunit loads tests), so some variables need to be explictly initialized as globals