Unit test factory should throw exceptions instead of returning WP_Error instances

[westonruter]

Common unit code includes statements like:

<?php
wp_set_current_user( self::factory()->user->create( array( 'role' => 'administrator' ) ) );

And:

<?php
$post_id = self::factory()->post->create( $data );
// ...
do_something_with_post( get_post( $post_id ) );

When analyzing the PHPUnit test code with PHPStan, this often results in argument.type errors like:

Parameter #1 $id of function wp_set_current_user expects int|null, int|WP_Error given.

and

Parameter #1 $post of function get_post expects int|numeric-string|WP_Post|null, int|WP_Error given

The workaround for these PHPStan errors is to do:

<?php
$user_id = self::factory()->user->create( array( 'role' => 'administrator' ) );
$this->assertIsInt( $user_id );
wp_set_current_user( $user_id );

And:

<?php
$post_id = self::factory()->post->create( $data );
$this->assertIsInt( $post_id );
// ...
do_something_with_post( get_post( $post_id ) );

Aside: Actually the create_and_get() factory method should actually be used in these cases, but in their cases the problem would be the same and you'd have to do:

<?php
$this->assertInstanceOf( WP_User::class, $user );
$this->assertInstanceOf( WP_Post::class, $post );

It seems exceptional to have to add these assertions. It would seem to be much more helpful to have the factory methods throw exceptions in the unlikely case that the object fails to be created. This would allow them

So the scope of this would be:

1. Throw exception whenever a WP_Error would otherwise be returned in the factory method.
2. Update the docs to indicate that WP_Error is never returned.
3. Remove extraneous assertions which have been added to the tests (like $this->assertIsInt( $post_id ) and $this->assertInstanceOf( WP_Post::class, $post )) which were added for the sake of static analysis.

Bonus: Whenever a set up method assigns a factory-created object to a member variable, we can add a native property type hint to the variable instead of using @var, unless the @var is able to express additional type information which isn't supported in PHP 7.4.

- /**
-  * @var int
-  */
- private $post_id;
+ private int $post_id;

Assigning a WP_Error to such a property with a native type hint would cause a fatal error in PHP, but since an exception would already have been thrown if a WP_Error were returned by a factory method, then the end result is the same.

By throwing an exception at the point of the factory being invoked, errors will be caught much earlier without crypting assertion failures which could happen when attempting to work with a WP_Error when an int or a some other object instance was expected.

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:

• Throw a WP_UnitTest_Factory_Exception whenever a factory method would otherwise return a WP_Error.
• Throw on falsy results (0 or false) as well, since a factory that returns no ID has failed the same way.
• Update documentation to drop WP_Error from the documented return types.
• Remove assertions that only existed to satisfy static analysis, since there is no longer an error to assert on.

Out of scope:

• Native property type hints on the factory classes.
• The low-level create_object(), update_object(), and create_upload_object() methods, which keep their int|WP_Error return values. These are extension points used by plugin and theme test suites, so changing their contract would be a backward compatibility break. Errors from them still surface through create().

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:

npm run env:start
npm run test:php -- --filter TestFactoryFor
npm run test:php -- --filter 'wpInsertPost|wpInsertTerm|wpInsertUser'

Then run the full suite to confirm nothing depends on the old return
value:

npm run test:php

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.

All three review points are fixed and pushed in c0c486fdcc:

• get_object_id() now has the string and int types.
• The comment in wpInsertTerm.php matches your wording.
• $term20 uses create_and_get() and the test asserts the name is A--.

I also fixed one test this change broke: Tests_Post_Revisions::test_wp_save_post_revision_error called the post factory with ID => PHP_INT_MAX and relied on the old WP_Error return. It now calls _wp_put_post_revision() directly, which is the function the test covers.

@westonruter this is ready for another look. I could not request review with the API (no push access), so I am asking here.

@westonruter All 11 review points are fixed and pushed in 37e85e47e9. Details are in the summary review above and each thread is replied to and resolved.

Short version:

• assert_valid_object_id() replaces get_object_id(), returns void, has @phpstan-assert int $object_id, and the docblock no longer claims to return an ID.
• test_insert_invalid_post_date now reaches the invalid-date path and asserts invalid_date in all six cases.
• test_wp_insert_term_duplicate_name asserts the bozo1 slug and closes the A family with a unique-slug count.
• missing_parent and invalid_post are pinned, the @method padding is fixed.

TestFactoryFor 9/9, wpInsertPost|wpInsertTerm|wpInsertUser 98/98, Tests_Term|Tests_Post_Revisions 680/680, full suite same single unrelated failure as trunk. phpcs and PHPStan clean.

This is ready for another look. I cannot request review with the API (no push access on this repo), so I am asking here.

@westonruter All 11 review points are fixed and pushed in 37e85e47e9. Details are in the summary review above and each thread is replied to and resolved.

Short version:

• assert_valid_object_id() replaces get_object_id(), returns void, has @phpstan-assert int $object_id, and the docblock no longer claims to return an ID.
• test_insert_invalid_post_date now reaches the invalid-date path and asserts invalid_date in all six cases.
• test_wp_insert_term_duplicate_name asserts the bozo1 slug and closes the A family with a unique-slug count.
• missing_parent and invalid_post are pinned, the @method padding is fixed.

TestFactoryFor 9/9, wpInsertPost|wpInsertTerm|wpInsertUser 98/98, Tests_Term|Tests_Post_Revisions 680/680, full suite same single unrelated failure as trunk. phpcs and PHPStan clean.

This is ready for another look. I cannot request review with the API (no push access on this repo), so I am asking here.

@westonruter I addressed the review feedback in commit 189bd547e0, pushed the branch, and resolved all 9 open review threads. Please review again when you have time.

@faisalahammad Please enable the PR option for maintainers to be able to make edits. I have some additional commits to push up.

In the meantime, I've opened a PR from my fork which you can merge into your branch: ​https://github.com/faisalahammad/wordpress-develop/pull/3

@westonruter I've enabled it. Could you please check to confirm whether it's working for you? Thank you for your help.

@faisalahammad Could you please merge ​https://github.com/faisalahammad/wordpress-develop/pull/3 first?

Your follow-up commits from faisalahammad/wordpress-develop#3 are merged into the branch, and the PR now points at 28267a3d6d. It went in as a fast-forward, so your 11 commits are intact and that PR closed as merged on its own.

The description has been updated to match the new scope: retrieval (get_object_by_id) throwing, create_object() and update_object() throwing, and the three bug fixes are now listed as in scope rather than out of scope. create_upload_object() is still kept as-is as the extension point.

Verification on this branch:

• TestFactoryFor: 31 tests, 62 assertions, pass.
• Affected group (wpInsertPost|wpInsertTerm|wpInsertUser|Tests_Term|Tests_Post_Revisions): 741 tests, 2011 assertions, pass.
• Full suite: 26304 tests, 4560180 assertions, 2 failures, 86 warnings, 0 errors. Both failures reproduce on clean trunk: the missing script module build artifact, and Tests_Admin_wpPostsListTable::test_search_hierarchical_pages_first_page (confirmed failing on trunk with the same message).
• PHPCS clean on the changed files. PHPStan on the factory files reports only the two pre-existing WP_TESTS_DOMAIN / WP_TESTS_EMAIL constant diagnostics in the network factory, nothing introduced.

@westonruter this is ready for another look. I could not request review with the API (no push access on this repo), so I am asking here.

The eight Copilot review comments are fixed and resolved in 842a1d53d4. I could not trigger the Copilot re-review because the GitHub request returned a fetch failure. Please trigger a re-review when convenient.

@westonruter The last open comment was about the test double in tests/phpunit/tests/includes/factory.php. Both docblocks now document the positive-int return and the WP_UnitTest_Factory_Exception. No code changed. Pushed in 7a845f0140. All review threads are resolved now. Please take another look when you have time.

[westonruter]

In 63924:

Build/Test Tools: Assert insertion failures without the factories.

Several tests triggered a failure by creating a fixture through a unit test factory and then asserting that the factory returned a WP_Error. These now call the function under test directly, whether wp_insert_post(), wp_insert_term(), wp_insert_user() or _wp_put_post_revision(), and pin the expected error code, so each test fails on the error it is actually about rather than on any error at all.

The invalid date cases in Tests_Post_wpInsertPost also gain a post title. With no title, content or excerpt, wp_insert_post() returned early with empty_content before it ever resolved the date, so those cases were passing on the wrong error. The duplicate term name test now also asserts the unique slugs its successful insertions produce.

Across the rest of the suite, assertions that only guarded against a factory returning a WP_Error are removed, such as assertNotWPError() and assertIsInt() checks on fixtures that are created successfully.

This prepares for a follow-up commit which makes the unit test factories throw an exception instead of returning a WP_Error. That leaves nothing for those guards to catch, and the tests rewritten here would otherwise not survive it.

Developed as subset of ​https://github.com/WordPress/wordpress-develop/pull/13612.
Follow-up to r29196, r34646, r45858, r50012, r51124.

Props faisalahammad, westonruter.
See #66111.

[westonruter]

In 63925:

Build/Test Tools: Make unit test factories throw on failure.

Unit test factory methods returned a WP_Error when the underlying create, update or retrieval failed. Most callers expect an object ID or an object, so the error went unchecked and the failure resurfaced later as an unrelated assertion, hiding its cause. The factories now throw a new WP_UnitTest_Factory_Exception instead, which stops the test at the point where its fixture could not be built.

The create(), create_and_get() and create_many() methods throw, as do the core create_object(), update_object() and get_object_by_id() implementations, which now document the type they return. Factories defined outside core that still return a WP_Error, null or false from those methods keep working, since create() and create_and_get() turn such a result into an exception as well. The create_upload_object() method is unchanged and still returns a WP_Error on failure.

Asserting these results surfaced several fixtures that had been silently wrong. The term factory's update_object() read an undefined taxonomy, so a term fixture with an after-create callback failed to update. The comment factory treated zero affected rows from wp_update_comment() as a failure. The term factory's create_and_get() looked the term up in the wrong taxonomy when one was given in the generation definitions. Updating a site or a network through the factory was never implemented and silently did nothing, and now throws.

The narrowed return types are documented rather than declared natively, so that a subclass which overrides get_object_by_id() without a return type still loads. The factory's own tests cover each of these behaviors, including a stub factory written against the contract before this change.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/13612.
Follow-up to r40968, r55019, r63924.

Props faisalahammad, westonruter.
Fixes #66111.