Repository navigation
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. |
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @3kori for the PR!
Add ticket annotation in new tests.
|
Tests with multiple assertions are missing the message parameter. |
|
Updated with suggested changes |
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks for adding coverage for wp_admin_bar_shortlink_menu()! I ran the tests locally (single site, multisite, and the full admin-bar group in both default and reverse order) and they all pass. A few suggestions to make them stronger:
1. Make the empty-shortlink setup explicit
// Establish a non-singular query context.
$short = wp_get_shortlink( 0, 'query' );This line doesn't establish any context; the test passes because the base test case resets $wp_query between tests. It would be clearer to set the state explicitly:
$this->go_to( home_url( '/' ) );
$this->assertFalse( is_singular(), 'Precondition: the query should not be singular.' );Alternatively, add_filter( 'pre_get_shortlink', '__return_empty_string' ) exercises the empty( $short ) branch directly.
2. Avoid circular assertions in the "node exists" test
wp_admin_bar_shortlink_menu() calls wp_get_shortlink( 0, 'query' ) itself, so comparing $node->href against that same call doesn't verify much, and assertNotSame( '', $short ) accepts any non-empty value. Asserting against the known expected value is stronger:
public function test_wp_admin_bar_shortlink_menu_adds_node_when_shortlink_exists() {
$post_id = self::factory()->post->create();
$expected = home_url( '?p=' . $post_id );
$this->go_to( get_permalink( $post_id ) );
$admin_bar = new WP_Admin_Bar();
wp_admin_bar_shortlink_menu( $admin_bar );
$node = $admin_bar->get_node( 'get-shortlink' );
$this->assertNotNull( $node, 'The Shortlink node should be added when a shortlink exists.' );
$this->assertSame( $expected, $node->href, 'The node href should be the post shortlink.' );
$this->assertSame( 'Shortlink', $node->title, "The node title should be 'Shortlink'." );
$this->assertStringContainsString(
'value="' . esc_attr( $expected ) . '"',
$node->meta['html'],
'The node HTML should contain the shortlink as the input value.'
);
}3. Assertion message mismatch
The last assertStringContainsString() checks the <input> value attribute, but the message says "...should use the generated shortlink as its href."
Nitpicks
*@ticket 66223is missing a space:* @ticket 66223.- Double space in
'should have \'Shortlink\' as title.'; double quotes would also avoid the escaping. - Two assertion messages are missing a trailing period.
- Core docblocks typically use "Tests that ..." as the summary, e.g. "Tests that the Shortlink node is not added when the shortlink is empty."
- "admin bar" rather than "admin-bar" in messages.
Optional additions
- A test that uses
pre_get_shortlinkto return a value containing"and&and asserts it is escaped in the inputvalue(e.g."b"&c). That covers theesc_attr()call, which is the main logic beyond the empty check. - Asserting the input's
aria-label="Shortlink".
I verified the suggested versions above pass locally as well.
|
Thank you for the review. Updated with suggested changes and add the special characters tests. |
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @3kori for the update!
Left some more feedbacks.
|
Thank you again @mukeshpanchal27. Applied all the changes suggested. |
Tests when shortlink when shortlink doesn't exists and when it does.
Trac ticket: #66223
Use of AI Tools
AI assistance: Yes
Tool(s): ChatGPT
Used for: Initial code skeleton and test suggestions; final implementation and tests were reviewed and edited by me.
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.