WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#31491 closed task (blessed) (fixed)

Enhance unit tests for persistent object caching

Reported by: ocean90 Owned by: ocean90
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch
Focuses: Cc:
PR Number:

Description (last modified by ocean90)

While running the tests with an object-cache.php drop-in (in this case Memcached) I got some failing tests:

There was 1 error:

1) Tests_Cache::test_switch_to_blog
Undefined variable: table_prefix

/srv/www/wp-develop/svn/src/wp-content/object-cache.php:316
/srv/www/wp-develop/svn/tests/phpunit/tests/cache.php:246

--

There were 6 failures:

1) Tests_Cache::test_wp_cache_init
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
         'default' => Memcache Object (
-            'connection' => resource(765) of type (memcache connection)
+            'connection' => resource(764) of type (memcache connection)
             '_failureCallback' => Array (...)
         )
     )
     'stats' => Array (...)
     'group_ops' => Array ()
     'cache_enabled' => true
     'default_expiration' => 0
     'global_prefix' => 'wptests_'
     'blog_prefix' => 'wptests_:'
     'cache_hits' => null
     'cache_misses' => null
 )

/srv/www/wp-develop/svn/tests/phpunit/tests/cache.php:284

2) Tests_Import_Import::test_small_import
Failed asserting that 0 matches expected 6.

/srv/www/wp-develop/svn/tests/phpunit/tests/import/import.php:175

3) Tests_Option_SiteTransient::test_the_basics
Failed asserting that true is false.

/srv/www/wp-develop/svn/tests/phpunit/tests/option/siteTransient.php:16

4) Tests_Option_Transient::test_the_basics
Failed asserting that true is false.

/srv/www/wp-develop/svn/tests/phpunit/tests/option/transient.php:16

5) Tests_Option_UpdateOption::test_should_set_autoload_yes_for_nonexistent_option_when_autoload_param_is_no
Failed asserting that 42579 matches expected 42580.

/srv/www/wp-develop/svn/tests/phpunit/tests/option/updateOption.php:88

6) Tests_Option_UpdateOption::test_should_set_autoload_yes_for_nonexistent_option_when_autoload_param_is_false
Failed asserting that 42586 matches expected 42587.

/srv/www/wp-develop/svn/tests/phpunit/tests/option/updateOption.php:113

FAILURES!
Tests: 3938, Assertions: 15029, Failures: 6, Errors: 1, Skipped: 16.

We should enhance these tests to make them work with a persistent object cache.

From @pento: "It'd be worth having a memcache variant of our Travis tests.". Sounds like a good idea to me.

Attachments (7)

31491.diff (1.0 KB) - added by pento 5 years ago.
31491.2.diff (973 bytes) - added by pento 5 years ago.
31491.wordpress-importer.patch (709 bytes) - added by ocean90 4 years ago.
31491.cache-flush.patch (2.7 KB) - added by ocean90 4 years ago.
31491-memcached.patch (503 bytes) - added by ocean90 4 years ago.
31491-skip-all-transient-as-options-tests.patch (2.4 KB) - added by ocean90 4 years ago.
31491.patch (4.1 KB) - added by ocean90 4 years ago.

Download all attachments as: .zip

Change History (41)

#1 @nacin
5 years ago

Yes please!

We should be able to just use PHP 5.6 with https://plugins.svn.wordpress.org/memcached/trunk/object-cache.php placed in the right place, plus echo "extension = memcache.so" >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini as suggested by http://docs.travis-ci.com/user/languages/php/#Preinstalled-PHP-extensions.

@pento
5 years ago

#2 @pento
5 years ago

31491.diff adds a memcache test (though it causes a warning in object-cache.php, along with the failures found by @ocean90).

I wanted to double check the use of the WP_MEMCACHE environment variable, is that the appropriate style for how we want our .travis.yml to evolve?

@pento
5 years ago

#4 @pento
5 years ago

31491.2.diff tweaks the patch a little bit based on @netweb's feedback in Slack.

I haven't committed it, as I assume we don't want to commit things while tests are failing. :-)

#5 @ocean90
4 years ago

  • Milestone changed from Awaiting Review to 4.4
  • Owner set to ocean90
  • Status changed from new to assigned

This needs to be done.

#6 @ocean90
4 years ago

In 33702:

Tests: Skip some tests for the Transients API when an external object cache is in use.

test_transient_data_with_timeout(), test_transient_add_timeout(), test_nonexistent_key_dont_delete_if_false(), and test_nonexistent_key_old_timeout are testing option values which aren't available with an an external object cache like memcache.

see #31491.

#7 @ocean90
4 years ago

1) Tests_Import_Import::test_small_import
Failed asserting that 0 matches expected 6.

/srv/www/wp-develop/svn/tests/phpunit/tests/import/import.php:175

The importer uses a query to update post_parent and not wp_update_post() which clears the cache. 31491.wordpress-importer.patch fixes that.

#8 @ocean90
4 years ago

  • Description modified (diff)

#9 @ocean90
4 years ago

  • Description modified (diff)

#10 @ocean90
4 years ago

5) Tests_Option_UpdateOption::test_should_set_autoload_yes_for_nonexistent_option_when_autoload_param_is_no
Failed asserting that 42579 matches expected 42580.

/srv/www/wp-develop/svn/tests/phpunit/tests/option/updateOption.php:88

6) Tests_Option_UpdateOption::test_should_set_autoload_yes_for_nonexistent_option_when_autoload_param_is_false
Failed asserting that 42586 matches expected 42587.

/srv/www/wp-develop/svn/tests/phpunit/tests/option/updateOption.php:113

These are because the cache doesn't get flushed correctly. 31491.cache-flush.patch uses WP_UnitTestCase::flush() to fix this.
But this seems to be a bug in the memcache drop-in because it should clear the $cache property by itself, see 31491-memcached.patch. @ryan, can you take a look at this and maybe commit?


@Otto42 or @nacin: Can you please commit 31491.wordpress-importer.patch to wordpress-importer/trunk?

#11 @ocean90
4 years ago

31491-skip-all-transient-as-options-tests.patch skips all current transient tests if an external object cache is in use. This will _fix_ the test_the_basics tests because the cache API has different return values, WP_Object_Cache::set() returns always true.

#12 @ryan
4 years ago

I handed the memcached backend over to @markjaquith and @sivel. I don't maintain it anymore.

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


4 years ago

#14 @wonderboymusic
4 years ago

In 34765:

Unit Tests: in Tests_Cache::test_wp_cache_init(), when wp_using_ext_object_cache(), just check that the global is an instance of WP_Object_Cache. External object cache instances of WP_Object_Cache will contain resources as props that will always have differing internal IDs, so strict comparison won't work.

See #31491.

#15 @wonderboymusic
4 years ago

In 34766:

Unit Tests: tests that want to flush the cache should use their instance method instead of calling wp_cache_flush() - more properties need to be wiped out in between tests.

Props ocean90.
See #31491.

#16 @wonderboymusic
4 years ago

In 34767:

Unit Tests: when wp_using_ext_object_cache(), mark some transient tests as skipped.

Props ocean90.
See #31491.

#17 @DrewAPicture
4 years ago

@ocean90 What's left here?

#18 @ocean90
4 years ago

  • Type changed from enhancement to task (blessed)

There are still some failing tests.

There were 7 failures:

1) Tests_Pluggable::testPluggableFunctionSignaturesMatch with data set #47 ('wp_cache_decr')
Parameter: n
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-offset
+n

/srv/www/wp-develop/svn/tests/phpunit/tests/pluggable.php:43

2) Tests_Pluggable::testPluggableFunctionSignaturesMatch with data set #50 ('wp_cache_get')
Failed asserting that 3 is identical to 4.

/srv/www/wp-develop/svn/tests/phpunit/tests/pluggable.php:26

3) Tests_Pluggable::testPluggableFunctionSignaturesMatch with data set #51 ('wp_cache_incr')
Parameter: n
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-offset
+n

/srv/www/wp-develop/svn/tests/phpunit/tests/pluggable.php:43

4) Tests_Pluggable::testPluggableFunctionSignaturesMatch with data set #58 ('wp_cache_reset')
Failed asserting that false is true.

/srv/www/wp-develop/svn/tests/phpunit/tests/pluggable.php:20

5) Tests_Pluggable::testAllPluggableFunctionsExist
Function: wp_cache_reset()
Failed asserting that false is true.

/srv/www/wp-develop/svn/tests/phpunit/tests/pluggable.php:64

6) Tests_Term_Meta::test_term_meta_should_be_lazy_loaded_for_all_terms_in_wp_query_loop
Failed asserting that 116548 is identical to 116546.

/srv/www/wp-develop/svn/tests/phpunit/tests/term/meta.php:139

7) Tests_Term_Meta::test_term_meta_should_be_lazy_loaded_only_for_the_queries_in_which_the_term_has_posts
Failed asserting that 116711 is identical to 116712.

/srv/www/wp-develop/svn/tests/phpunit/tests/term/meta.php:183
Last edited 4 years ago by ocean90 (previous) (diff)

#19 @boonebgorges
4 years ago

In 35108:

In cache tests, determine cache class name dynamically.

Some cache backends may use a class name other than WP_Object_Cache for their
cache drop-in. For example, certain versions of the APC Object Cache plugin
have a shim called APC_Object_Cache.

See #31491.

#20 @boonebgorges
4 years ago

In 35112:

In term meta lazy-loading tests, force WP_Query to cache results.

By default, WP_Query will not cache query results when using a persistent
object cache. The lazyload tests, however, depend on the cache being set during
each WP_Query, because the object cache is cleared between tests.

See #31491.

#21 follow-up: @ocean90
4 years ago

@johnbillion What do you think about skipping the pluggable function signatures checks for cache functions when wp_using_ext_object_cache()?

#22 @johnbillion
4 years ago

In 35145:

Remove wp_cache_reset() from the pluggable functions signature tests, as the function is deprecated and no longer used.

See #31491, #33867

#23 in reply to: ↑ 21 @johnbillion
4 years ago

Replying to ocean90:

@johnbillion What do you think about skipping the pluggable function signatures checks for cache functions when wp_using_ext_object_cache()?

That makes sense. Core doesn't have control over the parameter names in the pluggable functions that cache drop-ins implement. Although it does highlight the fact that the wp_cache_get() function in the Memcached plugin is missing its fourth parameter.

#24 @johnbillion
4 years ago

In 35146:

Skip the pluggable function signature tests when an external object cache is in use.

See #31491

#25 @johnbillion
4 years ago

In 35147:

Reinstate wp_cache_get() into the pluggable function tests. The signature tests are now skipped if an external object cache is in use.

See #31491

@ocean90
4 years ago

#26 @ocean90
4 years ago

In 35148:

Improve [35146] to only skip pluggable function signature tests for wp-includes/cache.php when an external object cache is in use.

See #31491, #33867.

#28 @ocean90
4 years ago

In 35209:

Tests: Don't preserve the global state for Ajax tests when using an external object cache.

Most of the Ajax tests are running in a separate PHP process, and thus PHPUnit attempts to preserve the global state from the parent process by serializing all globals. But this doesn't work for external object caches so we have to disable this "feature".

See #31491.

#29 @ocean90
4 years ago

$ phpunit -c tests/phpunit/multisite.xml

// …

2171) Tests_XMLRPC_wp_uploadFile::test_valid_attachment
Undefined property: stdClass::$domain

/srv/www/wp-develop/svn/tests/phpunit/includes/factory.php:190
/srv/www/wp-develop/svn/tests/phpunit/includes/factory.php:59
/srv/www/wp-develop/svn/tests/phpunit/includes/testcase.php:85
/srv/www/wp-develop/svn/tests/phpunit/includes/testcase-xmlrpc.php:10
/usr/local/src/composer/vendor/phpunit/phpunit/phpunit:56

FAILURES!
Tests: 4875, Assertions: 7913, Errors: 2171, Skipped: 15.

#30 @ocean90
4 years ago

In 35211:

Tests: Remove the @runTestsInSeparateProcesses annotation for Ajax tests.

They were added 3 years ago in [846/tests] because tests weren't excluded from the normal runs.

Reverts [35209] because it doesn't work with Xdebug.
Tests_Ajax_Response::test_response_charset_in_header is the only test which needs to run in a separate process, see [975/tests].

See #31491.

#31 @ocean90
4 years ago

In 35212:

Multisite: Remove the strictness for $using_paths in WP_Network::get_by_path().

The network lookup was broken when using an external object cache because $using_paths isn't always a boolean. Added in [34099].

See #31985, #31491.

#32 @ocean90
4 years ago

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

In 35213:

Travis: Extend our test suite with a memcached instance.

To test persistent object caching Travis CI runs now a build on PHP 5.6 with memached enabled. The build uses the drop-in from https://github.com/tollmanz/wordpress-pecl-memcached-object-cache which is based on the PECL memcached extension.

Props pento, ocean90.
Fixes #31491.

#33 @tollmanz
4 years ago

@ocean90 I was just made aware that this happened! Awesome stuff.

I would recommend using this as the link for the object-cache.php file: https://raw.githubusercontent.com/tollmanz/wordpress-pecl-memcached-object-cache/584392b56dc4adbe52bd2c7b86f875e23a3e5f75/object-cache.php. When I merge develop back to master, the location of the object-cache.php file will change. The URL I provided will be more reliable longer term.

#34 @ocean90
4 years ago

In 35218:

Travis CI: Use a pinned version of the object cache drop-in.

Props tollmanz.
See #31491.

Note: See TracTickets for help on using tickets.