#25282 closed defect (bug) (fixed)
Unit Tests should run in Debug mode
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 3.7 | Priority: | normal |
Severity: | normal | Version: | 3.7 |
Component: | Unit Tests | Keywords: | |
Focuses: | Cc: |
Description
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)
Change History (86)
#6
@
11 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.
#7
@
11 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.
#8
@
11 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.
#17
@
11 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.
#40
@
11 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.
#42
@
11 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.
#57
@
11 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 matchp
in the request, the cap check might pass while the queries for comments will blow up. I addedunset( $GLOBALS['post_id'] )
toTests_Ajax_GetComments::setUp()
.- If the global
$post_id
is empty, but$_REQUEST['p']
is not,$post_id
is now set toabsint( $_REQUEST['p'] )
and sanity-checked inwp_ajax_get_comments()
. map_meta_cap()
always assumes thatget_comment()
succeeds when checking for theedit_comment
cap. It doesn't. I added sanity checks in a few places where it will break early ifget_post()
orget_comment()
are empty.wp_update_comment()
always assumesget_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.
#58
follow-up:
↓ 62
@
11 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.
#61
@
11 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.
#66
@
11 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.
#69
@
11 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.
#72
@
11 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
#73
@
11 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.
#74
@
11 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
#76
@
11 years 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
#77
@
11 years 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.
#80
@
11 years 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.
#81
@
11 years 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.
#82
@
11 years 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.
#84
@
11 years ago
- Resolution set to fixed
- Status changed from reopened to closed
We can fix this later; closing this ticket as fixed.
In 25352: