[wp-trac] [WordPress Trac] #55918: Call wpTearDownAfterClass() before deleting all data, instead of after

[WordPress Trac]

#55918: Call wpTearDownAfterClass() before deleting all data, instead of after
--------------------------------------+---------------------
 Reporter:  SergeyBiryukov            |       Owner:  (none)
     Type:  defect (bug)              |      Status:  new
 Priority:  normal                    |   Milestone:  6.1
Component:  Build/Test Tools          |     Version:
 Severity:  normal                    |  Resolution:
 Keywords:  has-patch has-unit-tests  |     Focuses:
--------------------------------------+---------------------
Description changed by SergeyBiryukov:

Old description:

> 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 [source:tags/6.0/tests/phpunit/includes/abstract-
> testcase.php?marks=88-95#L82 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 `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 peform 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.

New description:

 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
 [source:tags/6.0/tests/phpunit/includes/abstract-
 testcase.php?marks=88-95#L82 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 peform 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.

--

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/55918#comment:4>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform