Call wpTearDownAfterClass() before deleting all data, instead of after

[SergeyBiryukov]

Background: #38264, #53011, #53746

As of [35186] and [51568], there are two sets of methods used for setup/teardown in the test suite before and after a test class is run:

• set_up_before_class() / tear_down_after_class()
• wpSetUpBeforeClass() / wpTearDownAfterClass()

The main difference is that wpSetUpBeforeClass() receives the $factory argument for ease of use, and both wpSetUpBeforeClass() and wpTearDownAfterClass() don't need to call self::commit_transaction().

Many tests use the wpTearDownAfterClass() method to clean up posts, users, roles, etc. created via wpSetUpBeforeClass(). However, looking at how the method is called​, this cleanup happens after all data is already deleted from the database. So it looks like most of the time the cleanup just silently fails, while the intended effect is still achieved, as the data is already gone and there is nothing more to delete.

This can cause some confusion when refactoring tests. Looking at the code in media tests, added in [41693] / #38264:

public static function wpTearDownAfterClass() {
	$GLOBALS['_wp_additional_image_sizes'] = self::$_sizes;
}

public static function tear_down_after_class() {
	wp_delete_attachment( self::$large_id, true );
	parent::tear_down_after_class();
}

At a glance, it seems like wp_delete_attachment() call can be moved to the first method, and the second one can be removed as redundant:

public static function wpTearDownAfterClass() {
	wp_delete_attachment( self::$large_id, true );

	$GLOBALS['_wp_additional_image_sizes'] = self::$_sizes;
}

However, at present, that would not work as expected: by the time wp_delete_attachment() runs, the attachment ID is no longer in the database, so it returns early, leaving some files in the uploads directory. This was previously tangentially noted in comment:6:ticket:38264.

By calling wpTearDownAfterClass() in WP_UnitTestCase_Base::tear_down_after_class() before deleting all data, instead of after, we can make sure both of these methods have access to the same data and can perform cleanup as necessary.

As a potential further enhancement, we could standardize on wpSetUpBeforeClass() / wpTearDownAfterClass() across the test suite for consistency, as these are already used in the majority of tests.

https://core.trac.wordpress.org/ticket/55918
https://core.trac.wordpress.org/ticket/55652

[SergeyBiryukov]

The PR also moves the calls to parent methods in WP_UnitTestCase_Base:

• parent::set_up_before_class() to be the first thing called in ::set_up_before_class()
• parent::tear_down_after_class() to be the last thing called in ::tear_down_after_class()

This does not have any effect in practice, but brings consistency with how these methods are called in the test suite.

[SergeyBiryukov]

It also looks like most of wpTearDownAfterClass() methods in tests are not strictly necessary and can be removed, as the data is deleted anyway via _delete_all_data() in WP_UnitTestCase_Base::tear_down_after_class(). That can be further explored in #53011 or #53746.

[ironprogrammer]

Due to ​lingering questions related to the continued use of wpSetUpBeforeClass() / wpTearDownAfterClass(), versus ​recent Core Handbook guidance to favor the newer set_up_before_class() / tear_down_after_class() methods, I propose punting this for further exploration and consensus.

For more background on the shift to use the snake_case methods, see ​Changes to the WordPress Core PHP Test Suite (27 September 2021).

And for completeness, the Core Handbook also mentions the older methods in the ​Slow Fixtures section. Additional discussion should pave a way forward to reconcile these differences.

[SergeyBiryukov]

Replying to ironprogrammer:

Due to ​lingering questions related to the continued use of wpSetUpBeforeClass() / wpTearDownAfterClass(), versus ​recent Core Handbook guidance to favor the newer set_up_before_class() / tear_down_after_class() methods, I propose punting this for further exploration and consensus.

Hmm, there might be some confusion here. As previously noted in ​this comment, that handbook paragraph specifically applies to the native PHPUnit methods that need to be replaced with WordPress' snake_case counterparts, as the tests would not pass across all supported PHP and PHPUnit versions otherwise:

• setUpBeforeClass() → set_up_before_class()
• tearDownAfterClass() → tear_down_after_class()

It does not apply to WordPress' own methods (note the wp prefix):

• wpSetUpBeforeClass()
• wpTearDownAfterClass()

These are fine to use and are used in many other tests, much more often than the previous methods. That is the reason I'd like to standardize on these across the test suite for consistency, since they are already used in the majority of tests.

Happy to do it the other way around and standardize on set_up_before_class() / tear_down_after_class() if that's preferred. Either way, the bug should be fixed for wpTearDownAfterClass() and tear_down_after_class() to have access to the same data and to become interchangeable. As mentioned in the ticket description, the standardization can be a potential future enhancement and does not have to be in the scope of this ticket.

[ironprogrammer]

Replying to SergeyBiryukov:

Happy to do it the other way around and standardize on set_up_before_class() / tear_down_after_class() if that's preferred. Either way, the bug should be fixed for wpTearDownAfterClass() and tear_down_after_class() to have access to the same data and to become interchangeable. As mentioned in the ticket description, the standardization can be a potential future enhancement and does not have to be in the scope of this ticket.

🙇🏼‍♂️ Please accept my apologies, Sergey, as my comments pertained to the standardization bits of this ticket. I didn't intend to squelch the fixes that have been proposed 😔 I agree that the silent failure described should be addressed for now.

[SergeyBiryukov]

In 54366:

Build/Test Tools: Call wpTearDownAfterClass() before deleting all data, instead of after.

As of [35186] and [51568], there are two sets of methods used for setup/teardown in the test suite before and after a test class is run:

• set_up_before_class() / tear_down_after_class()
• wpSetUpBeforeClass() / wpTearDownAfterClass(). (Note the wp prefix, these are WordPress' own methods and are not the same as the native PHPUnit setUpBeforeClass() / tearDownAfterClass() methods.)

The main difference is that wpSetUpBeforeClass() receives the $factory argument for ease of use, and both wpSetUpBeforeClass() and wpTearDownAfterClass() don't need to call self::commit_transaction().

Many tests use the wpTearDownAfterClass() method to clean up posts, users, roles, etc. created via wpSetUpBeforeClass(). However, due to how the method was previously called​, this cleanup happened after all data is already deleted from the database.

This could cause some confusion when refactoring tests. For example:

public static function wpTearDownAfterClass() {
	$GLOBALS['_wp_additional_image_sizes'] = self::$_sizes;
}

public static function tear_down_after_class() {
	wp_delete_attachment( self::$large_id, true );
	parent::tear_down_after_class();
}

At a glance, it seems like these two methods can be combined:

public static function wpTearDownAfterClass() {
	wp_delete_attachment( self::$large_id, true );

	$GLOBALS['_wp_additional_image_sizes'] = self::$_sizes;
}

However, that would not work as expected: by the time wp_delete_attachment() runs, the attachment ID is no longer in the database, so it returns early, leaving some files in the uploads directory.

By calling wpTearDownAfterClass() in WP_UnitTestCase_Base::tear_down_after_class() before deleting all data, instead of after, we ensure that both of these methods have access to the same data and can be used interchangeably to perform cleanup as necessary.

Additionally, this commit moves the calls to parent methods in WP_UnitTestCase_Base:

• parent::set_up_before_class() to be the first thing called in ::set_up_before_class()
• parent::tear_down_after_class() to be the last thing called in ::tear_down_after_class()

This does not have any effect in practice, but brings consistency with how these methods are called in the test suite.

Follow-up to [35186], [35225], [35242], [38398], [39626], [49001], [51568].

Props ironprogrammer, SergeyBiryukov.
Fixes #55918. See #55652.

Merged in r54366.

[SergeyBiryukov]

Replying to SergeyBiryukov:

Happy to do it the other way around and standardize on set_up_before_class() / tear_down_after_class() if that's preferred. [...] As mentioned in the ticket description, the standardization can be a potential future enhancement and does not have to be in the scope of this ticket.

Follow-up: #56740