Unit Tests should run in Debug mode

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

In 25355:

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

See #25282.

[wonderboymusic]

In 25356:

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

See #25282.

[nacin]

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.

[markoheijnen]

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.

[bpetty]

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

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

In 25366:

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

See #25282.

[wonderboymusic]

In 25367:

Suppress deprecated notices for wp_shrink_dimensions().

See #25282.

[wonderboymusic]

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.

[wonderboymusic]

In 25373:

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

See #25282.

[SergeyBiryukov]

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.

[wonderboymusic]

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.

[wonderboymusic]

In 25375:

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

See #25282.

[wonderboymusic]

In 25376:

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

See #25282.

[wonderboymusic]

In 25377:

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

See #25282.

[wonderboymusic]

In 25378:

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

See #25282.

[wonderboymusic]

In 25379:

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

See #25282.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

In 25384:

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

See #25282.

[wonderboymusic]

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.

[wonderboymusic]

In 25387:

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

See #25282.

[wonderboymusic]

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.

[wonderboymusic]

In 25389:

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

See #25282.

[wonderboymusic]

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.

[wonderboymusic]

In 25391:

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

See #25282.

[wonderboymusic]

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.

[wonderboymusic]

In 25393:

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

See #25282.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[nacin]

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.

[bpetty]

That sounds like a great idea to me.

[wonderboymusic]

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.

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

[nacin]

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?

[wonderboymusic]

I don't even have that directory in my checkout

[SergeyBiryukov]

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

[nacin]

In 25407:

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

[nacin]

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.

[nacin]

In 25409:

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

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[nacin]

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.

[nacin]

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.

[nacin]

In 25440:

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

[jdgrimes]

Related: #23931

[jdgrimes]

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.

[ocean90]

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.

[nacin]

In 25571:

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

[wonderboymusic]

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

[wonderboymusic]

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.

[markoheijnen]

This is seriously a no go. See #21663.

[wonderboymusic]

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

[markoheijnen]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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

[nacin]

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.

[wonderboymusic]

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

[nacin]

In 25785:

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

see #24813, #25282.

[markoheijnen]

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

[nacin]

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

[wonderboymusic]

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.

[wonderboymusic]

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.

[bpetty]

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.

[nacin]

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

[wonderboymusic]

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.

[nacin]

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