WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 14 months ago

Last modified 14 months ago

#17749 closed feature request (fixed)

Faster and phpunit runner-compatible unit tests

Reported by: nbachiyski Owned by: nbachiyski
Milestone: 3.3 Priority: normal
Severity: normal Version:
Component: Unit Tests Keywords: has-patch commit
Focuses: Cc:

Description

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).

Attachments (6)

new-unit-tests.diff (2.2 KB) - added by nbachiyski 3 years ago.
17749.diff (469 bytes) - added by scribu 3 years ago.
explicitly globalize wp_locale
fix-table_prefix-php_self-globalisation.diff (2.4 KB) - added by mrtorrent 3 years ago.
Explicitly globalise $table_prefix, fix globalisation of $PHP_SELF by using $_SERVER['PHP_SELF'] instead. Fixes issues with including Wordpress outside the global scope.
17749.2.diff (787 bytes) - added by ryan 3 years ago.
globalise_wp_rewrite.diff (319 bytes) - added by mrtorrent 2 years ago.
Explicitly globalise $wp_rewrite
globalize-version-vars.diff (1.0 KB) - added by nbachiyski 14 months ago.

Download all attachments as: .zip

Change History (48)

nbachiyski3 years ago

comment:1 westi3 years ago

  • Cc westi added

comment:2 follow-up: nacin3 years ago

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?

comment:3 in reply to: ↑ 2 nbachiyski3 years ago

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?

comment:4 ocean903 years ago

  • Keywords close added

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

comment:5 nbachiyski3 years ago

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

comment:6 nbachiyski3 years ago

  • Owner set to nbachiyski
  • Resolution set to fixed
  • Status changed from new to closed

In [18532]:

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

comment:7 scribu3 years ago

  • Milestone changed from Awaiting Review to 3.3

scribu3 years ago

explicitly globalize wp_locale

comment:8 scribu3 years ago

  • Keywords close removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

Last edited 3 years ago by scribu (previous) (diff)

comment:9 nacin3 years ago

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

In [18756]:

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

comment:10 nbachiyski3 years ago

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

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

comment:11 mrtorrent3 years ago

  • Cc mrtorrent added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:12 scribu3 years ago

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.

comment:13 scribu3 years ago

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

comment:14 mrtorrent3 years ago

@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?

comment:15 mrtorrent3 years ago

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

comment:16 follow-up: scribu3 years ago

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().

comment:17 in reply to: ↑ 16 ; follow-up: nacin3 years ago

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.

comment:18 in reply to: ↑ 17 mrtorrent3 years ago

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.

comment:19 scribu3 years ago

  • Keywords commit added

Ok, I'm convinced.

comment:20 follow-up: GaryJ3 years ago

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.

mrtorrent3 years ago

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

comment:21 in reply to: ↑ 20 mrtorrent3 years ago

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? :)

comment:22 ryan3 years ago

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

comment:23 mrtorrent3 years ago

$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?

comment:24 ryan3 years ago

Depends on how many plugins it breaks.

comment:25 mrtorrent3 years ago

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.

comment:26 ryan3 years ago

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

comment:27 nacin3 years ago

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

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.

ryan3 years ago

comment:28 nacin3 years ago

In [18994]:

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

mrtorrent2 years ago

Explicitly globalise $wp_rewrite

comment:29 follow-up: mrtorrent2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:30 in reply to: ↑ 29 westi2 years ago

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

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

comment:31 follow-up: mrtorrent2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:32 in reply to: ↑ 31 nacin2 years ago

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. :)

comment:33 nacin2 years ago

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

comment:34 follow-up: mrtorrent2 years ago

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.

comment:35 dd322 years ago

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)

comment:36 in reply to: ↑ 34 nacin2 years ago

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.

comment:37 mrtorrent2 years ago

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?

comment:38 nacin2 years ago

In [19603]:

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

comment:39 mbijon2 years ago

  • Cc mike@… added

comment:40 nbachiyski14 months ago

  • Milestone changed from 3.3 to 3.6
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:41 dd3214 months ago

  • Milestone changed from 3.6 to 3.3
  • Resolution set to fixed
  • Status changed from reopened to closed

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.

comment:42 nbachiyski14 months ago

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

Note: See TracTickets for help on using tickets.