Flaky test: test_get_theme_featured_list_api fails with external HTTP unavailability

[rakeshfalke]

The test file: tests/phpunit/tests/admin/includesTheme.php::test_get_theme_featured_list_api
Error message about WordPress.org API unavailability
That it's marked with @group external-http
Reference job ID: 70948707430
Your suggested fix (mock the API or skip gracefully)

…ilable

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

[SergeyBiryukov]

Hi there, thanks for the ticket!

Just noting that this previously came up in comment:18:ticket:54420.

The test was added in [39906] / #28121 to make sure that the list of theme features pulled from the WordPress.org API returns the expected data structure. It seems that mocking this response might defeat the purpose of the test, as it was specifically written to protect from unexpected changes in the WordPress.org API response.

[audrasjb]

Removing trunk version as this is not going to be shipped with WP 7.0 but in the next releases.

Hi, @Sukhendu2002, is this PR ready for review?

[wildworks]

This ticket has seen little movement and doesn't appear to be ready for PR review, so we will punt it to 7.2.

Thanks for working on this, @Sukhendu2002.

After review, I believe the diagnosis is slightly off in a way that changes the fix.

The assertion never fails. When themes_api() returns a WP_Error, get_theme_feature_list() returns the hardcoded fallback list (src/wp-admin/includes/theme.php:373-375), and that fallback satisfies assertNonEmptyMultidimensionalArray(). The red is entirely the wp_trigger_error() call at theme.php:564, which PHPUnit converts into an error because phpunit.xml.dist sets convertWarningsToExceptions="true".

That matters because of what comes next in themes_api(): line 564 fires the warning, then line 574 retries over plain HTTP. Under PHPUnit the warning throws first, so that retry is currently dead code. Your suppression filter is what finally lets it run — and the patch then skips anyway, keyed on whether the warning fired rather than on whether data came back.

I checked by running both versions against forced HTTP conditions:

Scenario	trunk	this PR
All outbound HTTP fails	error	skipped ✅
HTTPS fails, plain-HTTP retry returns valid data	error	skipped ⚠️
API reachable, returns a garbage body	passes	passes

Row 2 is the concern: the API is reachable, the fallback request succeeds, and the test skips regardless. Since external-http is excluded from the default suite (phpunit.xml.dist:31) and runs only in its dedicated CI job, that quietly removes the coverage that job exists to provide.

Two suggestions:

1. Base the skip on the returned value, not on a hook firing. Calling themes_api( 'feature_list', array() ) and checking is_wp_error() skips on exactly the right condition — and it also covers row 3, since a body that will not decode returns a WP_Error too.

2. Trunk already has a one-line form of this suppression, in tests/phpunit/tests/icons/wpIconsRegistry.php and tests/phpunit/tests/html-api/wpHtmlProcessor-serialize.php:

add_filter( 'wp_trigger_error_trigger_error', '__return_false' );
try {
        $feature_list = themes_api( 'feature_list', array() );
} finally {
        remove_filter( 'wp_trigger_error_trigger_error', '__return_false' );
}

if ( is_wp_error( $feature_list ) ) {
        $this->markTestSkipped(
                'WordPress.org Themes API is unavailable: ' . $feature_list->get_error_message()
        );
}

$this->assertNonEmptyMultidimensionalArray( get_theme_feature_list( true ) );

That drops wp_trigger_error_always_run and both closures. _backup_hooks() / _restore_hooks() in tests/phpunit/includes/abstract-testcase.php already clear leaked hooks at teardown, so the manual removals are a safety net rather than a requirement.

Worth noting for the Trac thread: @SergeyBiryukov's point in comment:2 was about mocking the API. Neither approach mocks it, so that objection should not block this.

Minor: please add @ticket 65064 alongside the existing @ticket 28121 — this file already stacks tickets on test_page_templates. Both hooks are @since 7.0.0, so this is trunk-only; worth saying so in the description.

---
_{AI use: <code>claude-opus-5</code>}

One more item of feedback, on wording rather than code: could you retitle this away from "flaky"?

See ​https://github.com/lancewillett/ai-plugins/tree/main/plugins/flake-to-fact

"Flaky" describes how a failure looks, not what it is — and here we know the mechanism precisely. The WordPress.org Themes API was unreachable over HTTPS, themes_api() emitted an E_USER_WARNING, and PHPUnit converted that into a test error. Nothing in the test itself is nondeterministic. Naming the cause helps the next person who hits this find the ticket.

The current title also says "skip instead of fail", but it is not a failure; the assertion passes on the hardcoded fallback. It is an error from the converted warning.

You can adjust the PR and Trac ticket titles to something like:

Build/Test Tools: Skip test_get_theme_featured_list_api when the .org Themes API is unreachable.

A description that names the converted warning as the cause makes everything more clear.