Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#33968 closed defect (bug) (fixed)

Improve performance of Unit Tests for 4.4

Reported by: jorbin's profile jorbin Owned by: jorbin's profile jorbin
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: Cc:

Description

This is a general catchall ticket for unit test performance improvements in 4.4. There are many things we can do to help the unit tests perform better and also have less of an impact on the wordpress.org infrastructure.

Attachments (5)

33968.patch (1.2 KB) - added by SergeyBiryukov 9 years ago.
33968.diff (1.9 KB) - added by boonebgorges 9 years ago.
awesome.diff (1.1 KB) - added by wonderboymusic 9 years ago.
fix-blown-up-factory.diff (1.6 KB) - added by nerrad 9 years ago.
preserve back compat of $factory property for plugins extending WP_UnitTestCase
awesome-2.diff (740.9 KB) - added by nerrad 9 years ago.
expands on awesome.diff and implements factory() to grab the WP_UnitTestFactory instance for all wp tests. (Search and Replace self::$factory with self::factory()

Download all attachments as: .zip

Change History (89)

#1 @jorbin
9 years ago

One of the first changes we should make is to stop hitting the svn repos in tests. Three tests in the http base test set (and potentially others) test by pinging urls that are in the svn repo. We should move those files to somewhere else to not hammer the svn repo.

#2 @jorbin
9 years ago

  • Owner set to jorbin
  • Status changed from new to assigned

#3 @jorbin
9 years ago

@samuelsidler is going to work on finding a new place for those files.

#4 follow-up: @jorbin
9 years ago

The three files right now are:

./http/base.php:  
183:		$url = 'http://unit-tests.svn.wordpress.org/trunk/data/images/2004-07-22-DSC_0007.jpg'; // we'll test against a file in the unit test data 
  
./http/base.php:  
204:		$url = 'http://unit-tests.svn.wordpress.org/trunk/data/images/2004-07-22-DSC_0007.jpg'; // we'll test against a file in the unit test data 
  
./http/base.php:  
226:		$url = 'http://develop.svn.wordpress.org/trunk/tests/phpunit/data/images/2004-07-22-DSC_0007.jpg'; 

#5 in reply to: ↑ 4 @SergeyBiryukov
9 years ago

Replying to jorbin:

The three files right now are:

This appears to be a single random image. Could we just switch it to another random image from the CDN, e.g. https://s.w.org/screenshots/3.9/dashboard.png?

#6 @jorbin
9 years ago

In 34568:

Stop hitting SVN for http tests

The automated tests can fail due to svn. Change the tests to use a WordPress CDN image.

see #33968

#7 @jorbin
9 years ago

In 34600:

Use an http URL rather then an https URL for tests

Travis doesn't properly support SSL on older versions of PHP. Since we don't test for SSL support in all of the tests, this can cause these tests to fail. This changes a URL introduced in [34568]

See #33968

#8 @jorbin
9 years ago

In 34656:

Exclude external HTTP tests from multisite run

In [30298] the unit tests default confirguration was modified to exclude external-http tests. This change was never migrated to the multisite XML configuration. The external HTTP code doesn't follow different logic in multisite, so the logic to exclude the tests then ( The external-http tests are very slow, and Wp_Http functionality is fairly isolated ) holds true here as well.

See #33968

#9 @jorbin
9 years ago

Two tests that have intermittent failures and could use a little TLC are: Tests_Media::test_add_image_size and Tests_Image_Intermediate_Size::test_get_intermediate_sizes_by_array_zero_width

See https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/82641433

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


9 years ago

#11 @jorbin
9 years ago

In 34694:

Cleanup image size in unit tests

This helps prevent test leakage that can cause Tests_Media::test_add_image_size to break.

See #33968

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


9 years ago

#13 follow-up: @jorbin
9 years ago

Intermittant failure: Tests_Ajax_DeleteComment::test_ajax_comment_trash_actions_as_administrator

See https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/82849174

#14 @DrewAPicture
9 years ago

Intermittent failure on 5.5.9: Tests_Theme::test_get_themes_contents. Haven't seen this in Travis, but I see it almost every time I run the suite on local.

#15 @DrewAPicture
9 years ago

In 34802:

Tests: Introduce WP_UnitTestCase::reset_permalinks(), an attempt to DRY up logic for resetting and restoring default permalinks on setUp() and tearDown().

See #33968.

#16 @DrewAPicture
9 years ago

In 34803:

Tests: Move the global permalinks reset routine to only fire when running core tests.

Should fix intermittent mysqli response errors in the AJAX tests.

See #33968.

#17 @DrewAPicture
9 years ago

In 34807:

Tests: Last try: It's redundant to reset permalinks on tearDown() if we're already doing it on every setUp().

Removes the restoration logic, which leveraged a static property initialized with get_option().

See #33968.

#18 @DrewAPicture
9 years ago

In 34810:

Tests: Permalink Structures Phase II: DRY up logic for setting permalink structures in test methods.

Renames reset_permalinks() to set_permalink_structure() (mimicking $wp_rewrite->set_permalink_structure()) and allows it to accept an optional permalink structure. In this way, we can double dip using it to both set and reset the permalink structure from anywhere.

Removes alot of duplicated code from tests.

See #33968.

#19 @DrewAPicture
9 years ago

Following [34802], [34803], [34807], and [34810], setting permalink structures in tests is now a lot simpler.

For generic cases, all you need to do is set it and forget it, e.g:

$this->set_permalink_structure( ‘/%postname%/‘ );

Doesn’t cover anything other than structures though. If you’re doing stuff with new rules, etc. continue to use the $wp_rewrite global as necessary. If you’re registering taxonomies or post types, you’re still going to want to run flush_rewrite_rules() afterward, regardless of whether you set the structure earlier and the rules were flushed then.

Side note: I avoided changing the canonical tests because I found them to be fragile. Maybe followup on that later.

Last edited 9 years ago by DrewAPicture (previous) (diff)

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


9 years ago

#21 @netweb
9 years ago

The PHPUnit ajax and external-http group of tests are not run as part of the PHP 5.2 Travis CI tests:

i.e. Both the grunt sub tasks of part of grunt phpunit task:

  • phpunit -c phpunit.xml.dist --group ajax
  • phpunit -c phpunit.xml.dist --group external-http

Example via Travis CI job ~2 months ago: https://travis-ci.org/aaronjorbin/develop.wordpress/builds/72667225

Running "phpunit:ajax" (phpunit) task
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 3.6.12 by Sebastian Bergmann.
Configuration read from /home/travis/build/aaronjorbin/develop.wordpress/phpunit.xml.dist
Time: 1 second, Memory: 116.00Mb
OK (0 tests, 0 assertions)

And

Running "phpunit:external-http" (phpunit) task
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
PHPUnit 3.6.12 by Sebastian Bergmann.
Configuration read from /home/travis/build/aaronjorbin/develop.wordpress/phpunit.xml.dist
Time: 2 seconds, Memory: 116.00Mb
OK (0 tests, 0 assertions)

Where both should respectively be:

Running "phpunit:ajax" (phpunit) task
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 4.5.0 by Sebastian Bergmann and contributors.
Configuration read from /home/travis/build/aaronjorbin/develop.wordpress/phpunit.xml.dist
................................................................. 65 / 70 ( 92%)
.....
Time: 1.9 minutes, Memory: 108.50Mb
OK (70 tests, 271 assertions)

And

Running "phpunit:external-http" (phpunit) task
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
PHPUnit 4.5.0 by Sebastian Bergmann and contributors.
Configuration read from /home/travis/build/aaronjorbin/develop.wordpress/phpunit.xml.dist
..................................................
Time: 25.5 seconds, Memory: 102.50Mb
OK (50 tests, 112 assertions)

Sadly PHPUnit error codes do not detect this: Done, without errors.

I've tried to find unearth some older build/job URL's but due to the way Travis CI UI works this is particularly difficult, I'd hoped to get a build/job before r30298 but haven't been able to, the closest I've got is #951 from ~10 months ago where the ajax and external-http are not run.

#22 @netweb
9 years ago

Intermittant failure: Tests_HTTP_curl::test_request_limited_size
See https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/83814943

#23 @jorbin
9 years ago

In 34848:

HTTP timeouts should cause some tests to be skipped, not failed

A number of the HTTP external tests can inconstantly fail. As the HTTP API is one that doesn't change often, this failure creates noise. With the goal of increasing the signal from the unit tests, these tests are now skipped if they timeout. A notice is added when running the external http tests so that the developer knows what skipped tests may mean here.

See #33968

#24 @jorbin
9 years ago

In 34849:

Fix whitespace issues introduced in [34848]

Bad Jorbin.

See #33968

#25 @DrewAPicture
9 years ago

In 34870:

Tests: Update Tests_Rewrite_AddRewriteRule->setUp() to use the new set_permalink_structure() helper.

See #33968.

#26 @SergeyBiryukov
9 years ago

In 34872:

Fix typo in [34848].

See #33968.

#27 @jorbin
9 years ago

In 34874:

Adjust detection of stream timeouts in maybe skip tests for https tests

This is a follow up to [34848].

See #33968.

#28 @jorbin
9 years ago

In 34879:

Add message with status info to temperamental assertion.

The ajax delete comment tests are intermittently failing. Many of the assertions make it hard to tell why they are failing. This adds a message to one of those assertions that contains some info on what is being asserted with the goal that it helps developers understand why the failure is failing.

See #33968

#29 @DrewAPicture
9 years ago

In 34983:

Tests: Add basic DocBlocks for four helper methods in general/template.php used to assist testing the Site Icon feature.

All four helpers were introduced in the feature merge for 4.3.

See #33968.

#30 in reply to: ↑ 13 @ocean90
9 years ago

Replying to jorbin:

Intermittant failure: Tests_Ajax_DeleteComment::test_ajax_comment_trash_actions_as_administrator

See https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/82849174

Also Tests_Ajax_DeleteComment::test_ajax_trash_comment_no_id

See https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/84939133

#31 @boonebgorges
9 years ago

In 35136:

In WP_UnitTestCase, only flush rewrite rules when they're set.

See [34810]. See #33968.

#32 @boonebgorges
9 years ago

In 35137:

Create fewer fixtures in some XML-RPC tests.

See #30017, #33968.

#33 @SergeyBiryukov
9 years ago

Some performance breakdown on individual tests: https://gist.github.com/ocean90/9ef0f64955ef9df2304f.

phpunit --exclude-group xmlrpc doesn't work as expected, it causes a bunch of errors and fails with a "Database is dead" message, see #34185.

However, excluding them in the config file, if we want to do that, seems to work.

33968.patch excludes XML-RPC tests from normal runs and moves them to a separate group.

Version 0, edited 9 years ago by SergeyBiryukov (next)

#34 @boonebgorges
9 years ago

The XML-RPC tests don't run all that slowly for me - certainly not on the same level as external-http and ajax. IMO, we should be wary of skipping any group of tests on normal runs, and only do it when the performance benefits are very large.

#35 @helen
9 years ago

I accept that there is almost definitely something going wrong with my setup which I cannot identify, but here are my SpeedTrap results for your tears: https://gist.github.com/helenhousandi/b6bae6c1a89b53842da8

I'm going to exclude the xmlrpc group from my own runs, as I would like to actually be able to run unit tests before commit without losing my sanity.

#36 @boonebgorges
9 years ago

In 35154:

Create fewer fixtures in XML-RPC getComments tests.

See #30017, #33968.

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


9 years ago

#38 @boonebgorges
9 years ago

In 35162:

Create fewer fixtures in some tests.

See #30017, #33968.

#39 @boonebgorges
9 years ago

In 35163:

Share fixtures in Tests_Get_Archives.

See #30017, #33968.

#40 @boonebgorges
9 years ago

In 35164:

Share fixtures in Tests_Admin_includesListTable tests.

See #30017, #33968.

#41 @boonebgorges
9 years ago

In 35165:

Fix incorrect variable names from [35164].

Cool story - the tests appeared to pass with the typos.

See #30017, #33968.

#42 @wonderboymusic
9 years ago

In 35171:

Unit Tests: wrestle performance out of Tests_Auth by cloning the same user for a majority of the tests.

See #30017, #33968.

#43 @wonderboymusic
9 years ago

In 35172:

Unit Tests: move ->test_readme() out of Tests_Basic and into Tests_External_HTTP_Basic in tests/external-http/.

I intend to move other wp_remote_get() tests into similar classes.

See #30017, #33968.

#44 @wonderboymusic
9 years ago

In 35173:

Unit Tests: reuse fixtures in Tests_Comment.

See #30017, #33968.

#45 @wonderboymusic
9 years ago

In 35174:

Unit Tests: call commit_transaction() in Tests_Auth set up.

See #30017, #33968.

#46 @wonderboymusic
9 years ago

In 35175:

Unit Tests: Tests_Auth needs a tearDownAfterClass impl to avoid spillage.

See #30017, #33968.

#47 @wonderboymusic
9 years ago

In 35176:

Unit Tests: Tests_Comment needs a tearDownAfterClass impl to avoid spillage.

See #30017, #33968.

#48 @wonderboymusic
9 years ago

In 35177:

Unit Tests: Tests_Canonical doesn't need to call wp_set_current_user() or implement tearDown because its grandparent calls wp_set_current_user( 0 ) in tearDown().

See #30017, #33968.

#49 @wonderboymusic
9 years ago

In 35178:

Unit Tests: move some oEmbed tests that can trigger HTTP calls to Tests_External_HTTP_OEmbed.

See #30017, #33968.

#50 @wonderboymusic
9 years ago

In 35179:

Unit Tests: make a fixture in Tests_Media to represent the large image, instead of creating it 10 times.

See #30017, #33968.

#51 @wonderboymusic
9 years ago

In 35181:

Unit Tests: in Tests_Media::test_wp_get_attachment_image_srcset_array_no_width(), just toggle metadata, instead of creating a new attachment. Shaves 75ms off the test.

See #30017, #33968.

#52 @wonderboymusic
9 years ago

In 35183:

Unit Tests: in Tests_Post, create fixtures for users.

See #30017, #33968.

#53 @wonderboymusic
9 years ago

In 35185:

Unit Tests: in Tests_Term, create fixtures for posts.

See #30017, #33968.

#54 follow-up: @wonderboymusic
9 years ago

In 35186:

Unit Tests: implement setUpBeforeClass() and tearDownAfterClass() on WP_UnitTestCase. Use late static binding (plus a gross fallback for PHP 5.2) to check if wpSetUpBeforeClass() or wpTearDownAfterClass() exist on the called class, and then call it and pass a static WP_UnitTest_Factory instance via Dependency Injection, if it exists.

This makes it way easier to add fixtures, and tear them down, without needing to instantiate WP_UnitTest_Factory in every class - removes the need to call commit_transaction() in each individual class.

See #30017, #33968.

#55 @wonderboymusic
9 years ago

In 35188:

Unit Tests: create more fixtures for Tests_User. When using a factory to create ad hoc users, use the inherited static prop $static_factory instead of the instance prop, $factory. If 2 factories are used out of sync, the generator sequences diverge and dupes can be created, causing an untold number of unforeseen errors. Yay.

See #30017, #33968.

#56 @wonderboymusic
9 years ago

In 35191:

Unit Tests: after [35186], "upgrade" the Canonical fixtures.

See #30017, #33968.

#57 @wonderboymusic
9 years ago

In 35192:

Unit Tests: in Tests_Comment_Query::test_get_comments_for_post(), create fewer comments (5, instead of 10).

See #30017, #33968.

#58 @wonderboymusic
9 years ago

In 35193:

Unit Tests: upgrade the fixtures in Tests_Post_Revisions.

See #30017, #33968.

#59 @wonderboymusic
9 years ago

In 35194:

Unit Tests: upgrade the fixtures in Tests_Post_Thumbnail_Template.

See #30017, #33968.

#60 @wonderboymusic
9 years ago

In 35196:

Unit Tests: create fewer terms in Tests_Term_getTerms::test_get_terms_parent_zero()

See #30017, #33968.

#61 @wonderboymusic
9 years ago

In 35197:

Unit Tests: add/upgrade the fixtures in Tests_User_Query.

See #30017, #33968.

#62 in reply to: ↑ 54 @jdgrimes
9 years ago

Replying to wonderboymusic:

In 35186:

Unit Tests: implement setUpBeforeClass() and tearDownAfterClass() on WP_UnitTestCase. Use late static binding (plus a gross fallback for PHP 5.2) to check if wpSetUpBeforeClass() or wpTearDownAfterClass() exist on the called class, and then call it and pass a static WP_UnitTest_Factory instance via Dependency Injection, if it exists.

This makes it way easier to add fixtures, and tear them down, without needing to instantiate WP_UnitTest_Factory in every class - removes the need to call commit_transaction() in each individual class.

See #30017, #33968.

Is there any reason why we can't just create one single instance of the factory and reuse it across all of the tests?

// in bootstrap.php 
include 'wp-unit-test-case.php';

WP_UnitTestCase::$static_factory = new WP_UnitTest_Factory();
// in WP_UnitTestCase::setUp();

$this->factory = self::$static_factory;

And there would no longer be any need to create the factory in setUpBeforeClass().

@boonebgorges
9 years ago

#63 @boonebgorges
9 years ago

I profiled the xmlrpc tests, and one bottleneck is the authentication process. get_user_by( 'login' ) takes a stupid amount of time. 33968.diff mocks the authentication process. I'm not sure if this is safe to do across all xmlrpc tests - probably not for the ones that are meant to test invalid u/p - but it does seem to reduce phpunit --group xmlrpc by about 15-20%.

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


9 years ago

#65 @boonebgorges
9 years ago

In 35206:

Fix incorrect variable name introduced in [35197].

See #30017, #33968.

#66 @wonderboymusic
9 years ago

In 35214:

Unit Tests: add SpeedTrapListener to phpunit/includes and add the config node to phpunit.xml.dist.

See #30017, #33968.

#67 @wonderboymusic
9 years ago

In 35224:

Unit Tests: since [32953], we can just use self::delete_user() instead of using if logic for Multisite.

See #30017, #33968.

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


9 years ago

#69 @wonderboymusic
9 years ago

In 35226:

Unit Tests: PHP 5.2, I Hate You and You Are Bringing Me Down.

#YOLOFriday

See #30017, #33968.

@nerrad
9 years ago

preserve back compat of $factory property for plugins extending WP_UnitTestCase

#70 @nerrad
9 years ago

So as brought up in slack, switching $factory to a static property blows up plugins extending that WP_UnitTestCase and the WP factory. The patch I added in fix-blown-up-factory.diff will fix it for plugins and I think preserve the intent with the changes, but of course means that all the changes made in WordPress testcases using the static property will need updated to use $_factory instead. I didn't have time to go through and do this for this patch but just threw this up for now in case it helps.

#71 @nerrad
9 years ago

duh missed awesome.diff. I gave it a go and although it fixes for plugins using WP_UnitTestCase (with a tweak) WP unit tests will need updated because __get() only works in object context. I don't think you're going to be able to avoid changing the $factory property back to a non static property and using something else like $_factory to hold the static instance.

@nerrad
9 years ago

expands on awesome.diff and implements factory() to grab the WP_UnitTestFactory instance for all wp tests. (Search and Replace self::$factory with self::factory()

#72 @nerrad
9 years ago

So basically I think you could just call self::factory() for all WP tests, so @wonderboymusic I made the tweak to your original diff and went through and did the search/replace for ya. This should work nicely to make sure plugins declaring using a $factory property still work.

#73 @wonderboymusic
9 years ago

In 35242:

Unit Tests: after [35225], make factory a method/getter on WP_UnitTestCase and add magic methods for BC for every plugin that is extending WP_UnitTestCase and accessing the $factory instance prop.

Props nerrad, wonderboymusic.
See #30017, #33968.

#74 @wonderboymusic
9 years ago

In 35243:

Unit Tests: after [35242], declare some missing instance props on individual test classes.

See #30017, #33968.

#75 @wonderboymusic
9 years ago

In 35244:

Unit Tests: WP_UnitTest_Generator_Sequence needs a static incrementer - otherwise, it assumes every test class is a reset, which it no longer is (it is now static).

While we're at it, remove unnecessary tearDown() code.

See #30017, #33968.

#76 @wonderboymusic
9 years ago

In 35245:

Unit Tests: better fixtures for Tests_Admin_Includes_Post.

See #30017, #33968.

#77 @wonderboymusic
9 years ago

In 35246:

Unit Tests: better fixtures for Tests_AdminBar. Don't force-delete some posts: some filter callbacks are no-ops on Multisite if the post is vanquished.

See #30017, #33968.

#78 @wonderboymusic
9 years ago

In 35247:

Unit Tests: better fixtures for Tests_User.

See #30017, #33968.

#79 @wonderboymusic
9 years ago

In 35248:

Unit Tests: better fixtures for Tests_User_WpSetCurrentUser.

See #30017, #33968.

#80 @wonderboymusic
9 years ago

In 35249:

Unit Tests: better fixtures for Tests_Meta_Slashes and Tests_WP_Customize_Section.

See #30017, #33968.

#81 @DrewAPicture
9 years ago

In 35270:

Tests: Add some more test coverage for get_term_field().

See #33968.

#82 @wonderboymusic
9 years ago

In 35311:

AJAX UNIT TESTS: Have you ever wondered why these take 600 forevers to run? They all eventually call do_action( 'admin_init' ), which has _maybe_update_core, _maybe_update_plugins, and _maybe_update_themes hooked to it. REMOVE THEM, and AJAX unit tests run like the wind. Tests_Ajax_Response is still slow.

See #30017, #33968.

#83 @wonderboymusic
9 years ago

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

we will be improving speed forever and always - #30017 already marked "ongoing"

#84 @DrewAPicture
7 years ago

In 42645:

Tests: Fix two typos introduced for a get_term_field() test in [35270].

See #33968.

Note: See TracTickets for help on using tickets.