expectedDeprecated() does unexpected things when there's an unexpected deprecated notice

[jdgrimes]

While testing some deprecated code yesterday I found some bugs with the @expectedDeprecated features of WP_UnitTestCase.

• If there are multiple unexpected deprecated warnings in a test, only the first is reported. Because you can only fail() once, you know. Also, if there is both a deprecated and a doing-it-wrong error, only the deprecated error will be reported. This is annoying, because when you add the @expectedDeprecated annotation you expect it to be fixed, and its not. There's another warning you weren't told about before.
• When there is an unexpected deprecated/doing-it-wrong error, the will tests fail() at the start tearDown(), meaning that the cleanup is incomplete. This can cause lots of weirdness in later tests that are perfectly fine otherwise. All of that goes away once the appropriate @expectedDeprecated annotation is added, but it can really befuddle you.

Patch forthcoming.

Introduced in [25408].

[jdgrimes]

Demonstrates the issues bing reported

[jdgrimes]

31362.diff​ fixes both of the reported issues. I've made it so expectedDeprecated() accumulates the unexpected notices, and then calls fail() with all of them at the end. I've also moved the call to it from tearDown() to assertPostConditions(), an apparently little-known (I've never heard of it before, and found it by looking at the source) method provided by PHPUnit which is the correct one to use to execute any assertions that should run after every test in a testcase.

unexpected-deprecated-demo.diff​ is path to the tests/phpunit/tests/includes/helpers.php testcase, meant only to demonstrate the issues. Once applied, if you run phpunit tests/phpunit/tests/includes/helpers.php, you'll get this:

$ phpunit tests/phpunit/tests/includes/helpers.php
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.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 3.7.28 by Sebastian Bergmann.

Configuration read from /Users/johngrimes/svn/wordpress/trunk/phpunit.xml.dist

........................FFF

Time: 16.08 seconds, Memory: 26.25Mb

There were 3 failures:

1) Tests_TestHelpers::test_is_true_on_occasion
Unexpected deprecated notice for Tests_TestHelpers::mock_deprecated

/Users/johngrimes/svn/wordpress/trunk/tests/phpunit/includes/testcase.php:238
/Users/johngrimes/svn/wordpress/trunk/tests/phpunit/includes/testcase.php:64

2) Tests_TestHelpers::test_is_false_by_default
Failed asserting that true is false.

/Users/johngrimes/svn/wordpress/trunk/tests/phpunit/tests/includes/helpers.php:195

3) Tests_TestHelpers::test_both_reported
Unexpected deprecated notice for Tests_TestHelpers::mock_deprecated

/Users/johngrimes/svn/wordpress/trunk/tests/phpunit/includes/testcase.php:238
/Users/johngrimes/svn/wordpress/trunk/tests/phpunit/includes/testcase.php:64

FAILURES!
Tests: 27, Assertions: 24, Failures: 3.

The first and third failures are intended, but that third failing test should also be reporting a doing-it-wrong notice. The second failure demonstrates that tearDown() has been interrupted and hasn't restored the hooks.

With 31362.diff​ applied, you'll get this:

$ phpunit tests/phpunit/tests/includes/helpers.php
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.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 3.7.28 by Sebastian Bergmann.

Configuration read from /Users/johngrimes/svn/wordpress/trunk/phpunit.xml.dist

........................F.F

Time: 16.5 seconds, Memory: 26.25Mb

There were 2 failures:

1) Tests_TestHelpers::test_is_true_on_occasion
Unexpected deprecated notice for Tests_TestHelpers::mock_deprecated

/Users/johngrimes/svn/wordpress/trunk/tests/phpunit/includes/testcase.php:253
/Users/johngrimes/svn/wordpress/trunk/tests/phpunit/includes/testcase.php:59

2) Tests_TestHelpers::test_both_reported
Unexpected deprecated notice for Tests_TestHelpers::mock_deprecated
Unexpected incorrect usage notice for Tests_TestHelpers::mock_incorrect_usage

/Users/johngrimes/svn/wordpress/trunk/tests/phpunit/includes/testcase.php:253
/Users/johngrimes/svn/wordpress/trunk/tests/phpunit/includes/testcase.php:59

FAILURES!
Tests: 27, Assertions: 24, Failures: 2.

Again, the first one is intended to fail. The second one succeeds, showing that tearDown() is no longer interrupted. Number three fails (as expected) and reports *both* unexpected notices.

[boonebgorges]

Awesome - thanks for the patch.

[boonebgorges]

In 31469:

Improved handling of expectedDeprecated and expectedIncorrectUsage annotations in unit tests.

• Do the expectedDeprecated() check in assertPostConditions() instead of tearDown(). Previously, failing inside of tearDown() was causing the rest of the teardown process to be aborted, resulting in inter-test leakage.
• Collect all expectedDeprecated and expectedIncorrectUsage annotations in an entire method and display them all when failing, instead of showing only the first one.

Props jdgrimes.
Fixes #31362.