Skip to content

Build/Test Tools: Throw exceptions from unit test factories instead of returning WP_Error - #13612

Open
faisalahammad wants to merge 2 commits into
WordPress:trunkfrom
faisalahammad:ticket/66111-factory-throws-exceptions
Open

faisalahammad wants to merge 2 commits into
WordPress:trunkfrom
faisalahammad:ticket/66111-factory-throws-exceptions

Conversation

@faisalahammad

@faisalahammad faisalahammad commented Sep 18, 2026

Copy link
Copy Markdown

https://core.trac.wordpress.org/ticket/66111

Trac ticket: 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 single failure present on a clean checkout is unrelated to this change (a missing script module build artifact) and reproduces on trunk without this patch. Trunk reports 1 failure, 86 warnings and 0 errors; this branch reports the same.

One test needed a companion update for the new contract. Tests_Post_Revisions::test_wp_save_post_revision_error created a post through the factory with an invalid ID and relied on the factory handing back a WP_Error. Now that the factory throws, that setup never reached its assertion. The test now passes the invalid ID to _wp_put_post_revision() directly, which is what the test is actually about, so the behavior under test is unchanged.

Props westonruter.

Use of AI Tools

AI assistance: Yes
Tool(s): ZCode
Model(s): Claude
Used for: Drafting the test-only follow-up changes requested in review (a typed helper signature, a comment update, and an added assertion) and running the test and lint verification. The implementation was reviewed and edited by me.

@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props faisalahammad, westonruter.

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.
@faisalahammad
faisalahammad force-pushed the ticket/66111-factory-throws-exceptions branch from 3b6fe78 to ab76c76 Compare September 18, 2026 21:23

@westonruter westonruter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good. Really cleans up a lot of unhelpful assertions.

I left a few minor comments.

Comment thread tests/phpunit/tests/term/wpInsertTerm.php Outdated
Comment thread tests/phpunit/tests/term/wpInsertTerm.php
Comment thread tests/phpunit/includes/factory/class-wp-unittest-factory-for-thing.php Outdated

@westonruter westonruter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Adds native parameter and return types to get_object_id(), rewords a
comment in the term insertion tests, and asserts the last term name now
that create_and_get() is used. Updates the revisions test to call
_wp_put_post_revision() directly, as the factory throws an exception
for an invalid post ID instead of returning a WP_Error.

See #66111.
@faisalahammad

Copy link
Copy Markdown
Author

All three review points are fixed and pushed in c0c486f:

  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants