[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