Build/Test Tools: Throw exceptions from unit test factories instead of returning WP_Error - #13612
faisalahammad wants to merge 1 commit into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Factory methods used to return a WP_Error object when the underlying create or update call failed. Most callers expect an object ID or an object, so the error was not checked and the failure resurfaced later as an unrelated assertion failure, making the real cause hard to find. This introduces WP_UnitTest_Factory_Exception and throws it from create(), create_and_get(), and generate_args() whenever a WP_Error or a falsy result would have been returned. The low-level create_object(), update_object(), and create_upload_object() methods keep their int|WP_Error return values, as they are extension points relied upon by plugin and theme test suites. A get_object_id() helper centralizes the conversion so subclasses do not need changes. The get_object_by_id() call in create_and_get() now throws instead of returning an error, and the term factory override is updated to match. Documentation is updated to drop WP_Error from the return types, and assertions that existed only to satisfy static analysis are removed from test cases, since the factories no longer return an error to assert on. Props westonruter. See #66111.
3b6fe78 to
ab76c76
Compare
westonruter
left a comment
There was a problem hiding this comment.
This is looking really good. Really cleans up a lot of unhelpful assertions.
I left a few minor comments.
| */ | ||
| public function test_wp_insert_term_duplicate_name() { | ||
| // The factory throws an exception when a term cannot be created, so failures are | ||
| // asserted with wp_insert_term() directly. |
There was a problem hiding this comment.
| // asserted with wp_insert_term() directly. | |
| // asserted with wp_insert_term() directly for subsequent term insertions. |
| $term19 = wp_insert_term( 'A-', 'post_tag' ); | ||
| $this->assertWPError( $term19 ); | ||
| $term20 = self::factory()->tag->create( array( 'name' => 'A--' ) ); | ||
| $this->assertNotWPError( $term20 ); |
There was a problem hiding this comment.
There is now not any assertion after the previous term is created. It should probably assert something like instead of create() use create_and_get() and assert that the name is equal to A--. Otherwise, the test success is measured by the absence of an error which isn't ideal.
| * @return int The object ID. | ||
| * @throws WP_UnitTest_Factory_Exception When the value is a WP_Error object or falsy. | ||
| */ | ||
| protected function get_object_id( $object_id, $message ) { |
There was a problem hiding this comment.
| protected function get_object_id( $object_id, $message ) { | |
| protected function get_object_id( $object_id, string $message ): int { |
westonruter
left a comment
There was a problem hiding this comment.
There is also an error in some PHP versions:
1) Tests_Post_Revisions::test_wp_save_post_revision_error
WP_UnitTest_Factory_Exception: Unable to create the object: Invalid post ID.
/var/www/tests/phpunit/includes/factory/class-wp-unittest-factory-for-thing.php:241
/var/www/tests/phpunit/includes/factory/class-wp-unittest-factory-for-thing.php:72
/var/www/tests/phpunit/includes/factory/class-wp-unittest-factory-for-thing.php:99
/var/www/tests/phpunit/tests/post/revisions.php:647
/var/www/vendor/bin/phpunit:122
https://core.trac.wordpress.org/ticket/66111
What and why
Unit test factory methods currently return a WP_Error object when the underlying create or update call fails. Most callers expect an object ID or an object, so the error goes unchecked and the failure reappears later as an unrelated assertion, which hides the real cause. A factory that cannot build its fixture should stop the test at that point, not hand back an error the caller has to remember to check.
Scope
In scope:
Out of scope:
Implementation notes
A new WP_UnitTest_Factory_Exception class extends Exception, matching the existing WPDieException pattern. A protected get_object_id() helper on WP_UnitTest_Factory_For_Thing centralizes the conversion, so create(), create_and_get(), and generate_args() throw without any subclass needing to change its own logic. The term factory override of create_and_get() is updated to match, and the @method IDE annotations in the subclasses are adjusted for readability.
Testing
Run the factory and affected test groups:
Then run the full suite to confirm nothing depends on the old return value:
Expected: all tests pass. The two failures present on a clean checkout are unrelated to this change (a missing script module build artifact and a PHPUnit 10 deprecation expectation), and they reproduce on trunk without this patch.
Props westonruter.