Opened 3 years ago
Last modified 2 years ago
#54029 new task (blessed)
Rename WordPress native expectDeprecated() and @ExpectedDeprecated to avoid confusion with PHPUnit native expectDeprecation and for future-proofing
Reported by: | hellofromTonya | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | |
Focuses: | Cc: |
Description
WordPress' test suite has a native expectation and annotation for expecting a deprecation notice: expectDeprecated()
and @ExpectedDeprecated
.
PHPUnit 8 introduced a new set of deprecation expectations: expectDeprecation()
, expectDeprecationMessage()
, and expectDeprecationMessageMatches()
.
Notice naming conventions between WordPress and PHPUnit. They are very similar.
WordPress | PHPUnit |
---|---|
expectDeprecated() | expectDeprecation()
|
@ExpectedDeprecated | - |
What is the problem?
The names are too similar. As such, confusion over which to use can happen. There's a potential of using the wrong one.
Future-proof against future PHPUnit changes:
Luckily PHPUnit used "deprecation" instead of "deprecated". However, there's the potential of future changes which may conflict with the native WP ones.
Proposal
Rename the expectation to include WP
and deprecate the currently ones.
Why deprecate? To avoid a backwards-compatibility break for any extenders using WordPress' native one.
- Confusion and potential to use the wrong one: the names are too similar.
- Future-proof:
Change History (6)
#3
@
3 years ago
I've just had a closer look at these functions - thanks @SergeyBiryukov for the list! - and I think we should probably consider doing a more thorough re-organization.
What do these functions do ?
First, let's look at the list of functions:
setExpectedDeprecated()
- This is the actual function which needs to be called within test methods to set an expectation for a deprecation notice.setExpectedIncorrectUsage()
- This is the actual function which needs to be called within test methods to set an expectation for a "doing it wrong" notice.expectDeprecated()
- This is an internal test framework function to set up the handling of the notifications thrown by the WP native_deprecated...()
functions and should never be called from within a test.expectedDeprecated()
- This is an internal test framework function to actually throw the errors when the expectations are not met.deprecated_function_run()
- This is an internal test framework (callback) function to register that a deprecation notice was caught.doing_it_wrong_run()
- This is an internal test framework (callback) function to register that a "doing it wrong" notice was caught.- This one doesn't actually belong in this list as it emulates the removed PHPUnit native method and has already been deprecated.setExpectedException()
Okay, so now let's dig a little deeper.
Are these functions still needed ?
Yes and no.
The WP native _deprecated_..()
functions all use the PHP E_USER_DEPRECATED
error level since [46625]. Also see #36561.
The WP native _doing_it_wrong()
function uses E_USER_NOTICE
.
So, realistically, these WP native setExpectedDeprecated()
and setExpectedIncorrectUsage()
methods and the @expectedDeprecated
and @expectedIncorrectUsage
annotations probably haven't really been needed for a while now and calls to these methods and use of these annotations should have been replaced with calls to the PHPUnit native expectDeprecation()
and expectNotice()
methods respectively.
However, there is an important difference between the PHPUnit native methods and the WP native methods:
The PHPUnit native methods can only set ONE expectation per test, while the WP native methods can set multiple.
The question then becomes: is this "multiple expectations" functionality actually being used ? and should it be ?
Multiple uses of the functions/annotations in tests
A quick search of the test code base tells me:
That there is one (1) test method which contains multiple calls to setExpectedDeprecated()
.
Tests_WP_Customize_Widgets::test_deprecated_methods()
. This test should be (and can easily be) refactored to a test with a data provider as the test is now testing four different things in one test method, which is just plain bad test design.
There are three (3) test methods which contain a call to setExpectedIncorrectUsage)
as well as a call to one of the PHPUnit native expect...()
methods:
Tests_Dependencies_Scripts::test_wp_localize_script_data_formats()
WP_Test_REST_Schema_Sanitization::test_format_validation_is_applied_if_missing_type()
WP_Test_REST_Schema_Validation::test_format_validation_is_applied_if_missing_type()
There are four (4) test methods which contain multiple calls to setExpectedIncorrectUsage)
WP_Test_REST_Schema_Sanitization::test_multi_type_with_no_known_types()
WP_Test_REST_Schema_Sanitization::test_multi_type_with_some_unknown_types()
WP_Test_REST_Schema_Validation::test_multi_type_with_no_known_types()
WP_Test_REST_Schema_Validation::test_multi_type_with_some_unknown_types()
The are seven (7) test methods which have multiple @expectedDeprecated
annotations - all related to the same methods get_theme
and get_themes
:
Tests_Theme::test_get_themes_default()
Tests_Theme::test_get_theme()
Tests_Theme::test_switch_theme()
Tests_Admin_IncludesTheme::test_page_templates()
Tests_Theme_ThemeDir::test_theme_default()
Tests_Theme_ThemeDir::test_theme_sandbox()
Tests_Theme_ThemeDir::test_broken_themes()
The are no test methods which have multiple @expectedIncorrectUsage
annotations.
A quick glance at the above listed methods shows that these are probably, more than anything, a case of bad test design and that the tests should be refactored, but this does have to be looked into in more depth for each individual case.
Problem outline
Looking at the basic principle more closely, we actually have five "problems" which would be great to solve in one go:
- The
setExpectedDeprecated()
and thesetExpectedIncorrectUsage()
method names "mirror" the PHPUnit nativesetExpectedException()
method name, but that method has been removed from PHPUnit in favour of theexpect...()
methods, making the WP nativeset...()
method names inconsistent and unexpected. - PHPUnit has removed support for using the
@expect...
annotations completely. Again, this makes supporting the annotations in the WP framework inconsistent and unexpected. - The fact that the WP native
setExpectedDeprecated()
and thesetExpectedIncorrectUsage()
methods, as well as the annotations, support multiple expectations for each test, again, is inconsistent with the PHPUnit native methods and therefore unexpected. - The
expectDeprecated()
andexpectedDeprecated()
methods are WP test framework internal methods, but 1) the names match the PHPUnit native method names far too closely, while the methods do something completely different and 2) these methods arepublic
, while they should in all reality beprivate
. The methods beingpublic
increases the risk of incorrect usage of these methods. - The
expectDeprecated()
andexpectedDeprecated()
methods use PHPUnit internal functionality which already broke during the current upgrade (and was fixed), and may well break again in the future as the PHPUnit native functionality used is explicitly not covered by the PHPUnit backward compatibility promise, so can even break on a minor or patch version of PHPUnit.
The deprecated_function_run()
and doing_it_wrong_run()
methods are not necessarily problematic. The names are not that well chosen, nor descriptive of what the methods do, but won't cause confusion.
Next steps
I'd like to suggest the following for next steps:
- In-depth investigate the above listed "problem" tests.
- If these can be refactored to still test what they were testing, but without multiple calls to the
setExpected...()
methods/multipleexpected...
annotations, they should be.
If all these cases can be fixed, I'd suggest we deprecate the whole WP native handling of the _deprecated...()
and _doing_it_wrong()
errors in favour of using the PHPUnit native expectDeprecation()
and expectNotice()
methods.
If not all these cases can be fixed, I'd suggest we:
- Deprecate the use of the annotations in favour of the function calls, in line with PHPUnit.
- Rename the
setExpectedDeprecated()
and thesetExpectedIncorrectUsage()
methods to better, more consistent names (names to be determined), deprecate the old names and throw a notice if they are still used. - Rename the
expectDeprecated()
andexpectedDeprecated()
methods, make the new methodsprivate
and deprecated the old methods, while also making themprotected
(this won't break anything as all test classes extend theWP_UnitTestCase_Base
anyway)
Opinions ?
/cc @johnbillion
Refs:
Thanks for the ticket!
Just noting that there are a few related methods in
WP_UnitTestCase_Base
that we might want to consider when making any changes here for consistent naming:expectDeprecated()
expectedDeprecated()
setExpectedDeprecated()
setExpectedIncorrectUsage()
setExpectedException()
deprecated_function_run()
doing_it_wrong_run()
On a slightly related note, I think
assertDiscardWhitespace()
should be renamed toassertEqualsDiscardWhitespace()
, for consistency withassertEqualsIgnoreEOL()
, but that's something for another ticket.