Make WordPress Core

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's profile 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.

  1. Confusion and potential to use the wrong one: the names are too similar.
  2. Future-proof:

https://youtu.be/SAtiKaUwLU4?t=12019

Change History (6)

#1 @hellofromTonya
3 years ago

  • Component changed from General to Build/Test Tools

#2 follow-up: @SergeyBiryukov
3 years ago

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 to assertEqualsDiscardWhitespace(), for consistency with assertEqualsIgnoreEOL(), but that's something for another ticket.

#3 @jrf
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.
  • 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 far 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:

Last edited 3 years ago by jrf (previous) (diff)

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#5 @SergeyBiryukov
3 years ago

In 51872:

Tests: Update the Services_JSON test for PHPUnit 9.5.10/8.5.21+.

Since PHPUnit 9.5.10 and 8.5.21, PHP deprecations are no longer converted to exceptions by default (convertDeprecationsToExceptions="true" can be configured to enable this).

Reference: Do not convert PHP deprecations to exceptions by default; PHPUnit 9.5.10 changelog.

With this change, the test for the Services_JSON compat class started failing:

There was 1 failure:

1) Tests_Compat_jsonEncodeDecode::test_json_encode_decode
Failed asserting that exception of type "PHPUnit\Framework\Error\Deprecated" is thrown.

This converts the native PHPUnit ::expectDeprecation() method call in the test to a set of individual WP-specific ::setExpectedDeprecated() method calls in order to not depend on PHPUnit behavior that is no longer the default.

Additionally, this commit includes support for catching deprecation notices from _deprecated_file() function calls to the WP_UnitTestCase_Base::expectDeprecated() method.

Follow-up to [46205], [46625], [48996], [51563], [51852], [51871].

Props jrf, netweb, SergeyBiryukov.
See #54183, #54029, #53363.

#6 in reply to: ↑ 2 @SergeyBiryukov
2 years ago

Replying to SergeyBiryukov:

On a slightly related note, I think assertDiscardWhitespace() should be renamed to assertEqualsDiscardWhitespace(), for consistency with assertEqualsIgnoreEOL(), but that's something for another ticket.

Follow-up: #56021

Note: See TracTickets for help on using tickets.