Update PR 1751 + add tests, improve coverage.

[costdev]

This PR expands on PR 1754 to:

• bring PR 1754 in line with modern Core/Test standards including:
  • improved test method descriptions.
  • clearer test method names.
  • data_ prefixes for data providers per Trac 53119.
  • $message parameters per Core Handbook: Writing PHPUnit Tests - Using Assertions.
  • line length limits for contributor a11y consideration.
  • filter callbacks changed to closures + static where appropriate per Core Handbook: Writing PHPUnit Tests - One-off Functions for Hooks.
• remove an unnecessary constant added to tests/phpunit/includes/bootstrap.php in PR 1754.
• remove tests related to the Rollback feature, which is not yet committed.
  • These have also been updated with the above and retained in a separate branch for a later time.
• add more tests/datasets.
• remove a refactor that should be separated out into a different ticket/PR.

Line coverage is now 48.21% with trunk.

I believe this is as far as I can go for now with unit tests due to missing strings, a missing guard, and the use of general functions and the need to mock WPDB to ensure the tests are pure unit tests. Otherwise, integration tests can be added to cover the areas not covered by this PR.

Trac ticket: https://core.trac.wordpress.org/ticket/54245

[jrfnl]

Reviewed at the request of @costdev - note: this is only a partial review as I ran out of time, so I've reviewed only the first 662 lines of the test file.

[jrfnl]

Be aware that withConsecutive() was deprecated in PHPUnit 9 and is being removed in PHPUnit 10.
Refs:

• Improvements on withConsecutive with return sebastianbergmann/phpunit#4255 (comment)
• Remove withConsecutive() sebastianbergmann/phpunit#4564

[costdev]

Thanks for that info! I'll have to see what's a suitable replacement for this then.

[costdev]

Update: I'm yet to find a suitable replacement for this. From what I can see in the references above, an alternative hasn't been implemented, but I may be missing something. Not sure of the path to move this part forward.

[peterwilsoncc]

@jrfnl As the WP test suite doesn't yet support PHPUnit 10 (via it's use of Yoast/PHPUnit-Polyfills 1.1), do you think it's worth putting this is as is to get some tests in for the class?

It doesn't look like there's a replacement for withConsecutive yet but I see you did some work on PHPCSStandards/PHPCSUtils to manage that so let me know if you have other suggestions.

[jrfnl]

@peterwilsoncc For now, I'd just use the withConsecutive().

If/when we update the test suite for PHPUnit 10.x with Polyfills 2.x, we can implement a solution for all the places where withConsecutive() is being used in one go. That should also make sure that we don't get fragmented solutions all over the place with people already trying to avoid withConsecutive(), but there not being a replacement.

As a side-note: the reason for not updating the WP Core test suite for PHPUnit 10.x so far, is largely related to the changes in PHPUnit 10.x which make it neigh impossible to reliable check for PHP deprecations/notices/warnings in PHPUnit 10.x. That, combined with PHPUnit 9.x still running fine on PHP 8.3, means we don't have a reason at this time to update the test suite.

The PHP deprecations/notices/warnings change in PHPUnit 10.x means we won't be able to fail the test suite on those anymore (= problematic for WP) or if we turn the option on to fail the test suite, the tests suite would also fail on PHPUnit native warnings, which is something we really don't want.
On top of that we actually have tests setting expectations for PHP deprecations/notices/warning and those will also become problematic.

All in all, enough reason not to update (yet) if we don't have to.

[peterwilsoncc]

@peterwilsoncc For now, I'd just use the withConsecutive().

If/when we update the test suite for PHPUnit 10.x with Polyfills 2.x, we can implement a solution for all the places where withConsecutive() is being used in one go. That should also make sure that we don't get fragmented solutions all over the place with people already trying to avoid withConsecutive(), but there not being a replacement.

Thanks @jrfnl, I think I'll merge in trunk and commit what's in the PR to trunk and the 6.4 branch provided everything still passes, that will avoid what we have slipping another release.

[peterwilsoncc]

Tests added in r56992 / b5392cc and subsequently backported to the 6.4 branch.

The src changes and the tests relating to those fixes were not included to allow for the backport, these will need to be done as a follow up during the 6.5 release cycle. As WordPress 6.4 is in the release candidate stage it's too late to include the bug fix.