Skip to content

Tests: Gracefully skip Tests_Readme when external version APIs time out or are unavailable - #14683

Closed
devmuhib009 wants to merge 3 commits into
WordPress:trunkfrom
devmuhib009:tests/63914-readme-timeout-handling
Closed

devmuhib009 wants to merge 3 commits into
WordPress:trunkfrom
devmuhib009:tests/63914-readme-timeout-handling

Conversation

@devmuhib009

Copy link
Copy Markdown
Member

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

Description

In Tests_Readme, get_response_body() contacts external APIs (php.net, endoflife.date, and mariadb.org) to verify the minimum recommended versions in readme.html.

Previously, if a request failed due to a network timeout or connection failure, assertNotWPError() caused an immediate hard test failure. Furthermore, only HTTP 503 was handled for skipping; other server errors or rate limiting (e.g. 429, 500, 502, 504) resulted in hard test failures.

This PR updates Tests_Readme::get_response_body() to:

  1. Check is_wp_error( $response ) and call markTestSkipped() with the error message when connection or timeout issues occur.
  2. Gracefully skip on HTTP 429, 500, 502, 503, or 504 server responses.

This prevents intermittent external network outages and rate limits from causing CI test failures on trunk.

…ut or are unavailable.

Updates get_response_body() in Tests_Readme to skip the test when a network timeout or connection error occurs (is_wp_error()) or when external servers return HTTP 429, 500, 502, 503, or 504 errors, preventing intermittent CI test failures caused by external service availability.

Props devmuhib009.
See #63914.
@github-actions

github-actions Bot commented Oct 22, 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.

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

Props devmuhib, huzaifaalmesbah, lancewillett.

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


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

@huzaifaalmesbah huzaifaalmesbah 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.

Thanks for working on this, @devmuhib009! Skipping on 429 and 5xx responses should cut a lot of flaky readme failures. LGTM.

AI Review: Opus 5.5

@lancewillett lancewillett 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.

One blocker: the blanket WP_Error skip masks errors beyond external availability failures. Details inline.

AI review · gpt-6

$host = isset( $parsed_url['host'] ) ? $parsed_url['host'] : $url;
$response = $this->wp_remote_get( $url );

if ( is_wp_error( $response ) ) {

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.

The explicit 429/500/502/503/504 skip list looks reasonable, but please retain failures for invalid URLs, locally blocked requests, and unknown errors instead of skipping every WP_Error. The inherited HTTP helper already handles recognized timeouts. A focused probe confirms that http_request_failed with “A valid URL was not provided.” and http_request_not_executed now both skip. Those can indicate broken test setup or a local HTTP regression rather than an unavailable upstream.

Please narrow the additional error skips to identified availability failures and check that invalid/local errors still fail. Checking only http_request_failed is not sufficient: invalid URLs use that code too.

AI review · gpt-6

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.

🟢 Approval recommended

The focused changes implement the requested outage handling while preserving successful-response validation, with no blocking issues identified.

No open issues 🎉

What changed in this PR

This PR makes Tests_Readme resilient to external version API outages, preventing transient network failures and rate limits from failing CI.

Changes:

  • Skip tests when requests return a WP_Error, including the error message in the skip reason.
  • Skip HTTP 429, 500, 502, 503, and 504 responses while preserving failures for other unexpected responses.
  • Reuse a host label with a URL fallback in diagnostic messages.
File Description
tests/​phpunit/​tests/​readme.php Adds graceful skips and diagnostic messages for external API failures.

Review effort: Balanced

@lancewillett

Copy link
Copy Markdown
Member

@devmuhib009 Could you please pick this one back up when you have time?

…s_Readme

Retain failures for invalid URLs, locally blocked requests, and unknown errors via assertNotWPError(). Only skip on identified external network availability failures or 429/5xx status codes, with dedicated unit tests covering both skipped and failing outcomes.

See #63914.

@lancewillett lancewillett 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.

The narrowed availability classification addresses the previous concern. One issue remains in the regression checks: SkippedTestError extends AssertionFailedError in PHPUnit 9.6, so the three tests expecting AssertionFailedError also pass if the helper skips. A standalone check confirmed that false pass.

Please explicitly reject SkippedTestError before accepting the expected assertion failure in the invalid-URL, blocked-request and 404 cases. Optional: avoid the HTTP wrapper’s retry sleeps for these fixed error fixtures; they add 45 seconds without exercising a changing response.

AI review · gpt-6

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.

🟡 Changes recommended

PHPUnit 5.7 exception mismatches break tests and bypass skip-message checks, while some network failures remain unrecognized and offline mocks incur substantial retry delays.

3 open issues

Review effort: Balanced

Comment thread tests/phpunit/tests/readme.php Resolved
Comment thread tests/phpunit/tests/readme.php Resolved
Comment thread tests/phpunit/tests/readme.php Resolved
@lancewillett

Copy link
Copy Markdown
Member

One additional compatibility point from Copilot: the supported PHPUnit 5.7 path uses legacy assertion exceptions; the namespaced AssertionFailedError is not an alias there. Please preserve the supported-version behavior while explicitly rejecting skips in cases that expect a failure. No broader network-message matrix is needed.

AI review · gpt-6

…d use compatible assertions in Tests_Readme. See #63914. Props devmuhib009.

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.

🟡 Changes recommended

The skip logic misses several cURL availability failures, so they can still cause hard CI failures without regression coverage.

1 open issue
3 resolved since last review

Review effort: Balanced

$error_message = $response->get_error_message();
$is_availability_failure = false;

if ( 'http_request_failed' === $response->get_error_code() ) {

@lancewillett lancewillett 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.

The response checks now distinguish expected skips from assertion failures and retain failures for invalid requests and unexpected responses. All 12 deterministic response checks pass in both single-site and multisite (27 assertions each). Live external API checks and PHPUnit 5.7 were not run locally. Current-head Copilot feedback was assessed; broader availability classification can be considered separately.

CI is green at this head. No remaining blocking finding.

AI review · gpt-6

wporg-sync pushed a commit that referenced this pull request Nov 1, 2026
Skip recognized availability failures in the readme version checks while retaining failures for invalid requests and unexpected responses. Add deterministic response checks that distinguish skipped tests from assertion failures.

Developed in: #14683

Props devmuhib, huzaifaalmesbah.
See #63914.


git-svn-id: https://develop.svn.wordpress.org/trunk@65176 602fd350-edb4-49c9-b593-d223f7449a82
@lancewillett

Copy link
Copy Markdown
Member

wporg-sync pushed a commit to WordPress/WordPress that referenced this pull request Nov 1, 2026
Skip recognized availability failures in the readme version checks while retaining failures for invalid requests and unexpected responses. Add deterministic response checks that distinguish skipped tests from assertion failures.

Developed in: WordPress/wordpress-develop#14683

Props devmuhib, huzaifaalmesbah.
See #63914.

Built from https://develop.svn.wordpress.org/trunk@65176


git-svn-id: http://core.svn.wordpress.org/trunk@64325 1a063a9b-81f0-0310-95a4-ce76da25c4cd
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.

4 participants