[wp-trac] [WordPress Trac] #54029: Rename WordPress native expectDeprecated() and @ExpectedDeprecated to avoid confusion with PHPUnit native expectDeprecation and for future-proofing
WordPress Trac
noreply at wordpress.org
Thu Sep 2 15:44:52 UTC 2021
#54029: Rename WordPress native expectDeprecated() and @ExpectedDeprecated to avoid
confusion with PHPUnit native expectDeprecation and for future-proofing
------------------------------+------------------------------
Reporter: hellofromTonya | Owner: (none)
Type: task (blessed) | Status: new
Priority: normal | Milestone: Awaiting Review
Component: Build/Test Tools | Version:
Severity: normal | Resolution:
Keywords: | Focuses:
------------------------------+------------------------------
Comment (by jrf):
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.
* ~~`setExpectedException()`~~ - This one doesn't actually belong in this
list as it emulates the removed PHPUnit native method and has already been
deprecated.
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:
1. The `setExpectedDeprecated()` and the `setExpectedIncorrectUsage()`
method names "mirror" the PHPUnit native `setExpectedException()` method
name, but that method has been removed from PHPUnit in favour of the
`expect...()` methods, making the WP native `set...()` method names
inconsistent and unexpected.
2. PHPUnit has removed support for using the `@expect...` annotations
completely. Again, this makes supporting the annotations in the WP
framework inconsistent and unexpected.
3. The fact that the WP native `setExpectedDeprecated()` and the
`setExpectedIncorrectUsage()` methods, as well as the annotations, support
multiple expectations for each test, again, is inconsistent with the
PHPUnit native methods and therefore unexpected.
4. The `expectDeprecated()` and `expectedDeprecated()` methods are WP test
framework internal methods, but 1) the names match the PHPUnit native
method names for too closely, while the methods do something completely
different and 2) these methods are `public`, while they should in all
reality be `private`. The methods being `public` increases the risk of
incorrect usage of these methods.
5. The `expectDeprecated()` and `expectedDeprecated()` 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:
1. In-depth investigate the above listed "problem" tests.
2. If these can be refactored to still test what they were testing, but
without multiple calls to the `setExpected...()` methods/multiple
`expected...` 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:
1. Deprecate the use of the annotations in favour of the function calls,
in line with PHPUnit.
2. Rename the `setExpectedDeprecated()` and the
`setExpectedIncorrectUsage()` methods to better, more consistent names
(names to be determined), deprecate the old names and throw a notice if
they are still used.
3. Rename the `expectDeprecated()` and `expectedDeprecated()` methods,
make the new methods `private` and deprecated the old methods, while also
making them `protected` (this won't break anything as all test classes
extend the `WP_UnitTestCase_Base` anyway)
Opinions ?
/cc @johnbillion
Refs:
*
[https://developer.wordpress.org/reference/functions/_deprecated_argument/
_deprecated_argument()]
*
[https://developer.wordpress.org/reference/functions/_deprecated_constructor/
_deprecated_constructor()]
* [https://developer.wordpress.org/reference/functions/_deprecated_file/
_deprecated_file()]
*
[https://developer.wordpress.org/reference/functions/_deprecated_function/
_deprecated_function()]
* [https://developer.wordpress.org/reference/functions/_deprecated_hook/
_deprecated_hook()]
* [https://developer.wordpress.org/reference/functions/_doing_it_wrong/
_doing_it_wrong()]
--
Ticket URL: <https://core.trac.wordpress.org/ticket/54029#comment:3>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac
mailing list