Tests: Improve unit test coverage for WP_Sitemaps_Renderer. - #13537
huzaifaalmesbah wants to merge 2 commits into
Conversation
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
lancewillett
left a comment
There was a problem hiding this comment.
Requesting one change before landing: preserve PHPUnit’s error handling in the rendering tests. The other three inline comments are optional improvements.
Local validation passed on PHP 8.3 / MySQL 9.7:
- Renderer class: 22 tests / 46 assertions in single-site and multisite.
- Full sitemaps group: 161 tests / 238 assertions in both modes.
- Two additional random-order class runs and all 11 new methods run individually passed.
- PHPCS passed.
The separate-process alternative was also validated locally. No production or test changes from the review probes remain.
Adversarial review · gpt-6
| if ( str_contains( $errstr, 'Cannot modify header information' ) ) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Requested change: please preserve PHPUnit’s error handling in these four rendering tests.
Returning false here invokes PHP’s built-in handler, not PHPUnit’s previous handler. An unrelated warning injected into render_index() failed as an output mismatch instead of being reported as a PHPUnit error.
Please remove these handler/try/finally blocks and use @runInSeparateProcess with @preserveGlobalState disabled on the retained rendering tests. The same pattern exists in Tests_Media_wpCrossOriginIsolation for calls that send headers.
I tried that replacement: all four rendering tests passed in both single-site and multisite, and the injected warning was correctly reported as a PHPUnit error.
Adversarial review · gpt-6
| ->getMock(); | ||
|
|
||
| $renderer->method( 'get_sitemap_index_xml' ) | ||
| ->willReturn( '' ); |
There was a problem hiding this comment.
Optional simplification, also applicable to test_render_sitemap_empty(): consider dropping these two mocked empty-output cases.
The real getters produce a root XML document even with no entries; their documented failure result is false. Returning '' here creates an artificial case with little additional protection. Removing the non-empty output guard also leaves these tests green because echoing '' emits nothing. That is equivalent behavior, not evidence of a missed functional regression.
The two real empty-input getter tests above are useful and should stay. If serialization failure is the intended scenario here, use false and describe that intent instead.
Adversarial review · gpt-6
| $actual = $renderer->get_sitemap_xml( $url_list ); | ||
|
|
||
| $this->assertStringContainsString( '<loc>http://' . WP_TESTS_DOMAIN . '/?foo=1&bar=2</loc>', $actual ); | ||
| $this->assertStringContainsString( '<lastmod>2020-01-01 & "quotes"</lastmod>', $actual ); |
There was a problem hiding this comment.
Optional improvement for both escaping tests: use the existing XML helper to verify the document is well formed, then assert the decoded element values.
These string fragments also constrain serialization choices. For example, literal quotes and " produce the same element text; > and > are equivalent in the priority fixture. The current assertions reject those valid alternatives.
Parsed-value assertions would retain protection against lost, malformed, or double-escaped content while allowing equivalent XML serialization. The special-character inputs are useful and can stay.
Adversarial review · gpt-6
| $sitemap_renderer = new WP_Sitemaps_Renderer(); | ||
| $stylesheet_url = $sitemap_renderer->get_sitemap_stylesheet_url(); | ||
|
|
||
| $this->assertSame( $custom_url, $stylesheet_url ); |
There was a problem hiding this comment.
Optional simplification for both stylesheet filter tests: remove the direct getter call and assertion, and keep the generated stylesheet processing-instruction assertion below.
That output check already proves the custom filter result reaches the renderer. It preserves the useful integration coverage while removing a redundant second call to the getter.
Adversarial review · gpt-6
This PR adds comprehensive unit test coverage for
WP_Sitemaps_Renderer(src/wp-includes/sitemaps/class-wp-sitemaps-renderer.php) intests/phpunit/tests/sitemaps/wpSitemapsRenderer.php.Prior to this PR, several methods, supported specification tags, and edge cases in
WP_Sitemaps_Rendererlacked dedicated unit tests.Trac ticket: https://core.trac.wordpress.org/ticket/65819
Changes & New Tests Added
WP_Sitemaps_Renderer::render_index():test_render_index(): Verifies that index XML output is rendered and echoed properly.test_render_index_empty(): Verifies that nothing is echoed when index XML is empty.WP_Sitemaps_Renderer::render_sitemap():test_render_sitemap(): Verifies that sitemap XML output is rendered and echoed properly.test_render_sitemap_empty(): Verifies that nothing is echoed when sitemap XML is empty.WP_Sitemaps_Renderer::get_sitemap_xml():test_get_sitemap_xml_with_all_supported_fields(): Tests all supported sitemap tags (loc,lastmod,changefreq, andpriority) to ensure full coverage of the tags block.test_get_sitemap_xml_empty(): Verifies empty URL array returns valid<urlset .../>XML without child<url>entries.test_get_sitemap_xml_escaping(): Tests proper XML entity escaping for special characters (&,<,>,") across supported fields.WP_Sitemaps_Renderer::get_sitemap_index_xml():test_get_sitemap_index_xml_empty(): Verifies empty entries array returns valid<sitemapindex .../>XML without child<sitemap>entries.test_get_sitemap_index_xml_escaping(): Tests XML entity escaping for index entries.Stylesheet Filter Tests:
test_get_sitemap_stylesheet_url_filter(): Tests filteringwp_sitemaps_stylesheet_urlto a custom URL and confirms the<?xml-stylesheet ?>processing instruction reflects the filtered URL.test_get_sitemap_index_stylesheet_url_filter(): Tests filteringwp_sitemaps_stylesheet_index_urlto a custom URL and confirms the<?xml-stylesheet ?>processing instruction reflects the filtered URL.Testing Instructions
Run the targeted test suite:
Run all sitemaps tests to ensure no regressions:
Use of AI Tools
AI assistance: Yes
Model(s): Gemini 3.8 Flash High
Used for: Identifying uncovered branches in
class-wp-sitemaps-renderer.php(specificallyrender_index(),render_sitemap(), empty dataset fallbacks, and supported XML tag mappings), drafting unit test assertions with proper error handling for CLI HTTP header constraints, and preparing this PR description. All generated test cases, assertions, PHPUnit execution (22 tests, 46 assertions), and WordPress Coding Standards compliance were thoroughly reviewed, executed, and validated by me in a local Docker environment.