Make WordPress Core

Opened 2 years ago

Closed 21 months ago

#25282 closed defect (bug) (fixed)

Unit Tests should run in Debug mode

Reported by: wonderboymusic Owned by:
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.7
Component: Unit Tests Keywords:
Focuses: Cc:


We get reports for notices / warnings sporadically and arbitrarily. Almost every time, they reveal coding errors, inconsistencies, or missed variable checks. Currently, if you add these lines to wp-tests-config.php:

error_reporting( -1 );
define( 'WP_DEBUG', true );

A LOT of tests fail and a lot of errors/warnings are exposed. Let's clean this up.

Attachments (1)

25282.diff (6.5 KB) - added by wonderboymusic 2 years ago.

Download all attachments as: .zip

Change History (85)

comment:1 @wonderboymusic2 years ago

In 25352:

Add set_current_screen( 'front' ) to query/conditionals tests setUp routine so that cruff from previous tests doesn't cause every conditional test to fail in debug mode.

See #25282.

comment:2 @wonderboymusic2 years ago

In 25353:

Use get_users() instead of the deprecated get_users_of_blog() in Tests_User_Capabilities::setUp() to avoid a tornado of warnings in debug mode.

See #25282.

comment:3 @wonderboymusic2 years ago

In 25354:

Add action/filter to Tests_Theme::setUp() to suppress errors while running back-compat tests on get_theme(), get_themes(), get_theme_data(), get_current_theme().

See #25282.

comment:4 @wonderboymusic2 years ago

In 25355:

Update the unit test methods in Tests_Admin_includesTheme. Use wp_get_theme() instead of get_theme().

See #25282.

comment:5 @wonderboymusic2 years ago

In 25356:

Avoid the following notice: Use of undefined constant term_id - assumed 'term_id', while running in debug mode.

See #25282.

comment:6 @nacin2 years ago

Many tests running get_themes() or get_theme() are deliberately doing so, such as [25355]. Rather than move this to use wp_get_theme(), I'd like to copy-paste the test so we have both old and new.

comment:7 @markoheijnen2 years ago

Deprecation really should be dealt differently. https://unit-tests.trac.wordpress.org/attachment/ticket/142/142.diff is having a WP_Deprecated_UnitTestCase class.

Maybe we should have an PHPDoc argument showing the function tests deprecated code.

comment:8 @bpetty2 years ago

Yeah, the tests are frequently the only place that ensures we're still maintaining back compat on deprecated functionality. We need those tests there.

comment:9 @wonderboymusic2 years ago

In 25362:

Restore the test code for get_theme(), add the notice suppression filters, and fix the type in the remove_filter() call.

See #25282.

comment:10 @wonderboymusic2 years ago

In 25363:

Set $_SERVER['REQUEST_METHOD'] = null in Tests_Auth::setUp() to suppress undefined index notices when wp_validate_auth_cookie() is called.

See #25282.

comment:11 @wonderboymusic2 years ago

In 25364:

Add a $post_id fixture to Tests_Comment_Query. Comments created without passing comment_post_ID produce an undefined variable notice in wp_insert_comment().

See #25282.

comment:12 @wonderboymusic2 years ago

In 25365:

Fix undefined index notices in comment/slashes test cases. wp_new_comment() and edit_comment() expect more variables than were being passed.

See #25282.

comment:13 @wonderboymusic2 years ago

In 25366:

get_home_url() assumes $GLOBALS['pagenow'] is set. It isn't in Tests_URL... and now it is.

See #25282.

comment:14 @wonderboymusic2 years ago

In 25367:

Suppress deprecated notices for wp_shrink_dimensions().

See #25282.

comment:15 @wonderboymusic2 years ago

In 25368:

Pass $args as an array, rather than a splat, to wp_clear_scheduled_hook(). There are comments in the code describing the deprecated argument behavior.

See #25282.

comment:16 @wonderboymusic2 years ago

In 25373:

Fix the PHP 5.4 Only variables should be passed by reference notices in tests/action.php.

See #25282.

comment:17 @SergeyBiryukov2 years ago

The change to a2-small-100x75.jpg in [25373] appears to be unrelated, but I had that file listed as changed on my install too, so something is probably going on with it.

comment:18 @wonderboymusic2 years ago

In 25374:

Passing non-existent object properties to WP_UnitTestCase::assertNull() produces notices, opt instead for WP_UnitTestCase::assertTrue( empty( $obj->prop ) ) in tests/db.php.

See #25282.

comment:19 @wonderboymusic2 years ago

In 25375:

Suppress the doing_it_wrong notice from firing in tests/phpunit/tests/dependencies/jquery.php` when running in debug mode.

See #25282.

comment:20 @wonderboymusic2 years ago

In 25376:

Avoid an undefined index error in tests/phpunit/tests/dependencies/styles.php

See #25282.

comment:21 @wonderboymusic2 years ago

In 25377:

Fix the PHP 5.4 Only variables should be passed by reference notices in tests/filters.php.

See #25282.

comment:22 @wonderboymusic2 years ago

In 25378:

Suppress the deprecated function notice in tests/formatting/CleanPre.php

See #25282.

comment:23 @wonderboymusic2 years ago

In 25379:

Pass a variable that actually exists to seems_utf8() in tests/formatting/SeemsUtf8.php

See #25282.

comment:24 @wonderboymusic2 years ago

In 25380:

  • Avoid notice by making WP_Image_Editor_Mock::test() compatible with WP_Image_Editor::test().
  • Suppress deprecated function notice for wp_load_image().
  • Add assertion for wp_get_image_editor().

See #25282.

comment:25 @wonderboymusic2 years ago

In 25381:

Avoid a notice and clean up setting/unsetting of globals by moving them into setUp() and tearDown() methods in Tests_Mail.

See #25282.

comment:26 @wonderboymusic2 years ago

In 25382:

  • Fix the horrendous whitespace in tests/media.php
  • Suppress the deprecated function notice for wp_convert_bytes_to_hr()
  • Add assertions for size_format()

See #25282.

comment:27 @wonderboymusic2 years ago

In 25383:

Add a post_id fixture in test/meta/slashes.php so Undefined variable: comment_post_ID notices don't fire when creating comments.

See #25282.

comment:28 @wonderboymusic2 years ago

In 25384:

Use reset() to access the first element of an array, don't assume there is an item at index 0.

See #25282.

comment:29 @wonderboymusic2 years ago

In 25386:

Add sanity checks to get_*_template() functions to ensure that the return value of get_queried_object() is compatible with the assumed context.

Fixes #25291.
See #25282.

comment:30 @wonderboymusic2 years ago

In 25387:

Suppress the _doing_it_wrong notices when calling add_theme_support( 'html5' ) in tests/theme/support.php

See #25282.

comment:31 @wonderboymusic2 years ago

In 25388:

  • Suppress deprecated function notices in tests/theme/themeDir.php
  • Set $theme['Template Files'] and $theme['Stylesheet Files'] to a variable before calling array methods upon them - avoids Indirect modification of overloaded element has no effect notice
  • Clean up setUp/tearDown in tests/theme.php

See #25282.

comment:32 @wonderboymusic2 years ago

In 25389:

Don't assume $GLOBALS['post'] is set in tests/url.php.

See #25282.

comment:33 @wonderboymusic2 years ago

In 25390:

  • Avoid Only variables should be passed by reference notice by passing a var to array_pop() in tests/user.php
  • Suppress a notice by silencing the passing of a non-existent object prop

See #25282.

comment:34 @wonderboymusic2 years ago

In 25391:

  • Suppress deprecated function notice for set_current_user()
  • Add assertions for wp_set_current_user()

See #25282.

comment:35 @wonderboymusic2 years ago

In 25392:

  • Add isset() checks all over WP_User_Query::prepare_query() and WP_User_Query::query(). When a WP_User_Query instance is constructed without passing args, no query vars are filled in, thus $qv doesn't contain most of the expected indices.
  • Suppress an undefined index notice in tests/user/query.php

Fixes #25292.
See #25282.

comment:36 @wonderboymusic2 years ago

In 25393:

Avoid undefined variable notice - wp_insert_user() expects user_pass to be passed to it.

See #25282.

comment:37 @wonderboymusic2 years ago

In 25394:

  • sort() returns a boolean, not a sorted set. Move the calls out of the assertions and fix the test methods.
  • Fix instances where Only variables should be passed by reference was being triggered by assigning array_keys() return value to a var

See #25282.

comment:38 @wonderboymusic2 years ago

In 25395:

Suppress Non-static method PO::*() should not be called statically, assuming $this from incompatible context by using the @ silencer in tests/pomo/po.php

See #25282.

comment:39 @wonderboymusic2 years ago

In 25397:

  • Fill in undefined var in Tests_Option_BlogOption
  • Add defined() check for BLOGSUPLOADDIR
  • Suppress deprecated function notices for is_blog_user() and get_dashboard_blog()
  • Check existence of $user in wpmu_log_new_registrations() before arbitrarily making a database query

Fixes all notices in multisite unit tests.

See #25282.

comment:40 @wonderboymusic2 years ago

Tests can now run in debug mode (multisite too) and produce no errors/warnings. 3 tests are still failing, but that's been the case for awhile.

AJAX tests are exploding. Will pick this up again tomorrow.

comment:41 @wonderboymusic2 years ago

In 25402:

There was way too much duplicated code in my notice cleanup, it built up over time, and there's definitely a need to standardize.

  • Remove duplicated code for deprecated function notice suppression
  • Add support in WP_UnitTestCase setUp/tearDown methods for $deprecated_functions fixture if the extending class has added it
  • Add a $deprecated_functions fixture to each extending class that needs it

To use this fixture, add something to your Test Case class like so:
protected $deprecated_functions = array( 'get_theme', 'get_themes', 'get_theme_data', 'get_current_theme' );

See #25282.

comment:42 @nacin2 years ago

I like [25402]. To take it further, what do we think about something like an annotation?

 * @expectedDeprecated get_themes
 * @expectedDeprecated get_theme

It could be applied to a single test, or to an entire class. But, it would be enforced as an assertion. If a test marked with this annotation stopped triggering a deprecated error for get_themes(), the test would fail.

We could implement this the same way PHPUnit implements @expectedException.

Any uncaught/unexpected deprecated notice should then cause the test to fail, regardless of WP_DEBUG.

Let's not rush to a follow-up commit on this; take our time to figure out if it's what we want to do.

comment:43 @bpetty2 years ago

  • Cc bpetty added

That sounds like a great idea to me.

comment:44 @jdgrimes2 years ago

  • Cc jdg@… added

comment:45 @wonderboymusic2 years ago

In 25404:

Fix the failing Tests_Link::test_wp_get_shortlink() assertion:

  • wp_get_shortlink() was firing a notice when reading $post->ID while $post was null in some cases
  • Before the assertions that assume $GLOBALS['post'] is not set, call unset( $GLOBALS['post'] ); - there was global spillage from other tests

See #25282.

comment:46 follow-up: @wonderboymusic2 years ago

In 25405:

Fix the failing l10n assertions:

  • Check if the file exists before running true assertions
  • If the file doesn't exist, run false assertions

See #25282.

comment:47 in reply to: ↑ 46 @nacin2 years ago

Replying to wonderboymusic:

In 25405:

Fix the failing l10n assertions:

  • Check if the file exists before running true assertions
  • If the file doesn't exist, run false assertions

See #25282.

When does this file not exist?

comment:48 @wonderboymusic2 years ago

I don't even have that directory in my checkout

comment:49 @SergeyBiryukov2 years ago

wpcom-themes appears to be removed in [25004].

comment:50 @nacin2 years ago

In 25407:

Revert [25405] and use a file that does exist. see #25282.

comment:51 @nacin2 years ago

In 25408:

Test framework: Introduce the annotation @expectedDeprecated, modeled after PHPUnit's @expectedException.

It works for both functions and arguments (using the value of the first argument passed to _deprecated_function() or _deprecated_argument(), which is typically the function name). It asserts both ways:

  • If specified, those deprecated notices must be caught, or the test fails.
  • If not specified, any other deprecated notices cause the test to fail.

Works regardless of WP_DEBUG.
see #25282.

comment:52 @nacin2 years ago

In 25409:

Use @expectedDeprecated. see #25282, [25408].

comment:53 @wonderboymusic2 years ago

In 25430:

  • Avoid notices in tests/ajax/Autosave by bailing early when get_post() returns nothing.
  • Check for the existence of $_POST['catslist'] before using it in wp_ajax_autosave().

See #25282.

comment:54 @wonderboymusic2 years ago

In 25431:

Fix an error in tests/ajax/Compression by removing the unnecessary call to ob_end_clean() immediately after ob_get_clean() in _gzdecode, which has aleady deleted the buffer.

See #25282.

comment:55 @wonderboymusic2 years ago

In 25432:

Remove the unnecessary call to ob_end_clean() directly after ob_get_clean() in WP_Ajax_UnitTestCase::dieHandler(). This fixes a large number of AJAX errors in debug mode.

See #25282.

comment:56 @wonderboymusic2 years ago

In 25433:

Fix some undefined index notices related to Comment unit tests:

  • There are several places where a $_POST index was unchecked before setting a variable
  • In wp_notify_postauthor(), $comment was being returned null, but its properties were being accessed.
  • In check_ajax_referer(), 3 different values can be checked for nonce on $_REQUEST, but only 1 had an isset()

See #25282.

comment:57 @nacin2 years ago

wonderboymusic in [25438]:

Fix several esoteric errors related to AJAX unit tests for comments:

  • wp_ajax_get_comments() relies on the $post_id global - even though $_POST['p'] is passed to every action in the test methods. If $post_id is still lingering in between tests and doesn't match p in the request, the cap check might pass while the queries for comments will blow up. I added unset( $GLOBALS['post_id'] ) to Tests_Ajax_GetComments::setUp().
  • If the global $post_id is empty, but $_REQUEST['p'] is not, $post_id is now set to absint( $_REQUEST['p'] ) and sanity-checked in wp_ajax_get_comments().
  • map_meta_cap() always assumes that get_comment() succeeds when checking for the edit_comment cap. It doesn't. I added sanity checks in a few places where it will break early if get_post() or get_comment() are empty.
  • wp_update_comment() always assumes get_comment() succeeds. It doesn't. I added a check for empty.

All AJAX unit tests run and pass in debug mode. All general unit tests pass against these changes.

Fixes #25282.

comment:58 follow-up: @nacin2 years ago

Deliberately not closing this. [25438/trunk/src/wp-includes/capabilities.php] should be reverted. See #13905.

I am somewhat concerned about some of the changesets in this ticket — any changes to /src need to be carefully reviewed. Sometimes papering over a notice can cause actual API changes. For example [25121] introduced a security issue fixed in [25137]. [25433] doesn't appear to have such an issue but we ideally should avoid modifying $_POST directly where possible.

comment:59 @nacin2 years ago

In 25440:

Reinstate an assertion commented out in [25409]. This test is skipped due to @ticket anyway. see #25282.

comment:61 @jdgrimes2 years ago

I think we should consider changing wp-tests-config-sample.php so WP_DEBUG is on by default, now that this is cleaned up. That's best practice for development environments, as we all know.

comment:62 in reply to: ↑ 58 @ocean902 years ago

Replying to nacin:

I am somewhat concerned about some of the changesets in this ticket — any changes to /src need to be carefully reviewed. [...] [25433] doesn't appear to have such an issue but we ideally should avoid modifying $_POST directly where possible.

See #25369.

comment:63 @nacin2 years ago

In 25571:

Update wp-tests-config-sample.php to run with WP_DEBUG by default. see #25282.

comment:64 @kovshenin2 years ago

  • Cc kovshenin added

comment:65 @wonderboymusic2 years ago

Unit tests explode in PHP 5.5 due to the mysql_connect() deprecation notice.

@wonderboymusic2 years ago

comment:66 @wonderboymusic2 years ago

25282.diff implements MySQLi procedurally and makes the Unit Tests that were producing ungodly numbers of database errors pass. I invite everyone with PHP 5.5 to add error_reporting(-1) to their config and see the difference.

This is going to be really annoying for anyone on 5.5 to deal with, but I anticipate hearing the words "no" and "punt." HyperDB should be updated as well.

comment:67 @markoheijnen2 years ago

This is seriously a no go. See #21663.

comment:68 @wonderboymusic2 years ago

well aware of the ticket - doesn't have many champions

comment:69 @markoheijnen2 years ago

Not sure about it. The plugin is pretty much stable and tries to care about back compat but still needs a few tries. Also that doesn't mean this patch a winner.

comment:70 @wonderboymusic2 years ago

In 25660:

Add a default value to WP_UnitTest_Factory_For_Comment::default_generation_definitions['comment_content'] to avoid a tornado of database errors in PHP 5.5/MySQL 5.6, even when WP_DEBUG is turned off.

See #25282.

comment:71 @wonderboymusic2 years ago

In 25661:

post_content does not have a default value in the $wpdb->posts table. Add a default value of empty string to wp_insert_attachment() to avoid a tornado of database errors in PHP 5.5/MySQL 5.6, even when debug mode is turned off.

See #25282.

comment:72 @wonderboymusic2 years ago

to clarify, 25282.diff​ was just a POC, and is universally hated - the Marko PDO thing will get a better look once we branch 3.7

comment:73 @nacin2 years ago

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

Unit tests now run in debug mode, and that's now the default. Closing this ticket as fixed.

[25660] and [25661] were ultimately not necessary as this was to support MySQL strict mode, which we don't otherwise support. Don't get me wrong, they were still a good idea to do, but we don't need to go on a strict mode rampage beyond that, unless we actually decide to approach that systematically.

comment:74 @wonderboymusic2 years ago

Ironically, there's only one other error to fix for strict mode, and it's when a a negative number is passed as the value for post_parent - but no biggie

comment:75 @nacin23 months ago

In 25785:

Test runner: Add @expectedIncorrectUsage to trap _doing_it_wrong() calls.

see #24813, #25282.

comment:76 @markoheijnen23 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Can we revert chaneset [25395]? It's lame to suppress those valid notices. I rather would update that code from http://glotpress.trac.wordpress.org/browser/trunk/pomo/po.php

Last edited 23 months ago by markoheijnen (previous) (diff)

comment:77 @nacin23 months ago

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

marko: Sure, but let's do that in a new ticket, because it will also require us to change the pomo library.

comment:78 @wonderboymusic22 months ago

In 26004:

Set the default parent id to 0, instead of -1, in Tests_Post_Attachments::_make_attachment(). Prevents Out of range value for column 'post_parent' database error without papering over it in core.

See #25282.

comment:79 @wonderboymusic22 months ago

In 26187:

Remove the Featured Content term filters when running Unit Tests. Set the return value of wp_get_object_terms() to a var before passing to array_shift() in test_get_object_terms_types(), which expects a var to be passed by reference.

See #25282.

comment:80 @bpetty22 months ago

That's a *really* specific job for includes/utils.php with _clean_term_filters() there. There's a whole bunch of tests that are leaving behind or removing hooks that they are supposed to reset at the end of their tests like this, and we really shouldn't be just adding a ton of really obscure and specific functions in a generic framework-wide include like this.

Ideally, the test(s) responsible for leaving the hooks behind should be removing them on teardown cleanup, or if this was just some default hooks you needed removed specifically for these tests, then just remove them in setUp(), and add them back in tearDown() (or in the specific test itself). It should be clear about what those tests are doing within just that file though.

P.S. This ticket is closed, and I'm not even sure what that last commit has to do with this ticket at all anymore. Really need to start new tickets for these now. Is this 5.5 debug errors now? These run fine with debug in 5.4 and below.

comment:81 @nacin22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Also not a fan of [26187]. The unit tests should use a clean slate and that includes not depending on the theme.

comment:82 @wonderboymusic22 months ago

The unit tests were failing hard, and no one was stepping in to fix them - replace that function with whatever tickles your fancy, but the theme was being oblivious to the environment as a whole, and no one offered an alternate patch, which I would happily use in place of mine.

comment:83 @SergeyBiryukov21 months ago

  • Version changed from trunk to 3.7

comment:84 @nacin21 months ago

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

We can fix this later; closing this ticket as fixed.

Note: See TracTickets for help on using tickets.