Skip to content

Build/Test Tools: Use strict assertions in REST schema sanitization tests#12613

Open
thisismyurl wants to merge 1 commit into
WordPress:trunkfrom
thisismyurl:fix/64895-strict-assertions-rest-schema-sanitization
Open

Build/Test Tools: Use strict assertions in REST schema sanitization tests#12613
thisismyurl wants to merge 1 commit into
WordPress:trunkfrom
thisismyurl:fix/64895-strict-assertions-rest-schema-sanitization

Conversation

@thisismyurl

Copy link
Copy Markdown

Replaces the remaining assertEquals() calls in the REST schema sanitization tests with assertSame().

These assertions are a good fit for strict comparison, because the type of the returned value is the thing under test. rest_sanitize_value_from_schema() promises to coerce a value to the type declared in the schema, and most of these cases pass a deliberately mistyped input to prove it does:

$this->assertSame( array( 1.0 ), rest_sanitize_value_from_schema( array( '1' ), $schema ) );

With a loose assertion that test still passes if the sanitizer hands back the uncoerced array( '1' ), so it cannot fail for the one reason it exists.

Worth flagging for review, since it is the easy thing to get wrong here: every schema in these particular tests declares 'type' => 'number', which returns (float), so the expected values are floats rather than integers. That matches the convention already used by test_type_number() at the top of this file, which asserts assertSame( 1.0, ... ).

I checked each expected literal against its own schema rather than swapping the method names mechanically, which also means three values are deliberately left alone:

  • 'd' => '1' in the nested-properties test stays a string, because d has no matching entry in properties, no patternProperties, and additionalProperties is unset, so it is never coerced.
  • The top-level 'b' => 1 in that same test stays an integer for the same reason.
  • 'b' => 1 in test_type_object() likewise stays an integer.

The strict assertions now document that behaviour instead of glossing over it.

Only assertion methods and expected literals changed. The file already used assertSame() for 65 of its assertions, so this makes it consistent throughout.

One disclosure on verification: this box has no Docker/wp-env, so I could not run the full PHPUnit suite locally. I verified each conversion by reading the schema declared in each test and matching the expected literal to the type rest_sanitize_value_from_schema() returns for it, and php -l is clean. I would appreciate a CI run confirming it.

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

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.8
Used for: Helping locate candidate files and cross-check each expected literal against the schema type. I reviewed the conversions and take responsibility for the final code.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

…ests

Replace the remaining assertEquals() calls with assertSame() in the REST
schema sanitization tests.

These assertions exist to verify that rest_sanitize_value_from_schema()
coerces a value to the type declared in the schema, so the type of the
returned value is the whole point of the test. A loose assertion passes
even when the sanitizer returns the uncoerced value, which hides exactly
the class of bug these tests are meant to catch.

The file already used assertSame() for the majority of its assertions,
so this makes it consistent throughout.

Props thisismyurl.
See #64895.
Copilot AI review requested due to automatic review settings July 20, 2026 12:46
@github-actions

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

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request tightens the REST schema sanitization test suite by replacing remaining loose PHPUnit assertions with strict assertions, ensuring the tests actually verify that rest_sanitize_value_from_schema() returns values coerced to the schema-declared types (not just loosely equal values).

Changes:

  • Replaced remaining assertEquals() calls with assertSame() in REST schema sanitization tests.
  • Updated expected literals from integers to floats where the schema declares 'type' => 'number' (since sanitization returns (float)).
  • Kept specific values intentionally non-coerced (e.g., properties not declared in properties and with additionalProperties unset), making that behavior explicit under strict comparison.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

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