Add navigation smoke tests #2054
Conversation
|
Claude finished @gcsecsey's task —— View job Code Review: Add navigation smoke testsI've completed a comprehensive review of the new E2E test file. Overall, this is a solid addition to the test suite with good coverage of basic navigation flows. Below is my detailed feedback. SummaryStrengths:
Issues Found:
Detailed Findings1. Cross-Platform Compatibility ✅The test file is cross-platform compatible:
Minor suggestions: Use 2. Code Quality IssuesBug: Loop iteration mismatch (line 15)The comment says "up to 3 times" but the loop only runs twice. See inline comment for fix. Fragile selectors (lines 122-127)Using Silent error swallowing (line 25-29)The empty catch block makes debugging harder. Consider at least logging the error or adding a comment explaining why it's expected. 3. Security ✅No security concerns:
4. Performance ⚡
5. Testing QualityTest Coverage: Good smoke test coverage:
Potential improvements:
Test Quality:
6. DocumentationMissing:
Docs/ai-instructions.md: No updates needed - this is a straightforward E2E test addition that doesn't change architecture or introduce new patterns. Test Patterns Consistency ✅Compared to
RecommendationsMust Fix (blocks merge):
Should Fix (before merge): Nice to Have (can be follow-up): Pre-merge Checklist ItemsBased on the PR template:
Great work on adding these smoke tests! The core logic is solid and follows good patterns. Please address the bug on line 15 and consider the selector improvements to reduce test flakiness. Let me know if you have questions about any of the feedback. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| */ | ||
| async function closeWelcomeGuide( page: Page ) { | ||
| // Try to close the modal up to 3 times | ||
| for ( let i = 0; i < 2; i++ ) { |
There was a problem hiding this comment.
Bug: Loop counter mismatch
The comment on line 10 states "Attempts to close up to 3 times" but the loop condition is i < 2, which only allows 2 iterations (i=0 and i=1).
| for ( let i = 0; i < 2; i++ ) { | |
| for ( let i = 0; i < 3; i++ ) { |
Either update the loop to i < 3 or update the comment to say "up to 2 times".
| } catch ( e ) { | ||
| // Modal not found or already closed, exit the loop | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Code Quality: Empty catch block loses error context
The catch block silently swallows the error without logging. While this may be intentional for the modal not appearing, it makes debugging harder if there's an actual error.
Consider logging at debug level:
} catch ( e ) {
// Modal not found or already closed, exit the loop
// Note: This is expected when the modal doesn't appear
break;
}Or if you have a logger, use it for visibility during test debugging.
| test( 'creates a post', async ( { page } ) => { | ||
| // Navigate to new post page | ||
| const newPostUrl = `${ wpAdminUrl }/post-new.php`; | ||
| await page.goto( getUrlWithAutoLogin( newPostUrl ) ); | ||
|
|
||
| const editorFrame = page.frameLocator( 'iframe[name="editor-canvas"]' ); | ||
|
|
||
| // Close welcome guide if it appears (always on main page, not in iframe) | ||
| await closeWelcomeGuide( page ); | ||
|
|
||
| // Wait for title to be available in iframe | ||
| const titleSelector = 'h1.editor-post-title'; | ||
| await editorFrame.locator( titleSelector ).waitFor( { timeout: 30_000 } ); | ||
| await editorFrame.locator( titleSelector ).fill( 'E2E Test Post' ); | ||
|
|
||
| // Click into the content area and type | ||
| await editorFrame.locator( titleSelector ).press( 'Enter' ); | ||
| const contentBlock = editorFrame.locator( 'p[role="document"]' ).first(); | ||
| await contentBlock.click(); | ||
| await contentBlock.fill( 'This is a test post created by automated E2E tests.' ); | ||
|
|
||
| // Publish the post (publish buttons are on main page) | ||
| const publishButton = page.locator( 'button.editor-post-publish-button__button' ).first(); | ||
| await publishButton.waitFor( { state: 'visible', timeout: 10_000 } ); | ||
| await publishButton.click(); | ||
|
|
||
| // Wait for and click the confirm publish button in the panel | ||
| const confirmPublishButton = page.locator( 'button.editor-post-publish-button__button' ).last(); | ||
| await confirmPublishButton.waitFor( { state: 'visible', timeout: 10_000 } ); | ||
| await confirmPublishButton.click(); | ||
|
|
||
| // Wait for success message | ||
| await expect( page.locator( '.components-snackbar' ) ).toBeVisible( { timeout: 10_000 } ); | ||
|
|
||
| // Verify post was created by visiting posts list | ||
| await page.goto( getUrlWithAutoLogin( `${ wpAdminUrl }/edit.php` ) ); | ||
| await expect( page.locator( 'a.row-title:has-text("E2E Test Post")' ) ).toBeVisible(); | ||
| } ); |
There was a problem hiding this comment.
Test Coverage: Missing cleanup
This test creates a post but doesn't clean it up afterward. Consider one of these approaches:
- Add cleanup: Delete the post after verification
- Use a unique identifier: Add timestamp/UUID to post title to avoid conflicts in repeated runs
- Document the behavior: Add a comment explaining that test posts accumulate
Since this is an E2E test and the entire site is ephemeral (created per test session), this may be acceptable, but it's worth making the decision explicit.
|
|
||
| test( 'creates a post', async ( { page } ) => { | ||
| // Navigate to new post page | ||
| const newPostUrl = `${ wpAdminUrl }/post-new.php`; |
There was a problem hiding this comment.
Cross-Platform: Path concatenation
While string concatenation works here for URLs, consider using new URL() for consistency and to avoid potential issues:
const newPostUrl = new URL( '/wp-admin/post-new.php', wpAdminUrl ).toString();This ensures proper URL handling even if wpAdminUrl has trailing slashes or other edge cases.
|
|
||
| // Wait for title to be available in iframe | ||
| const titleSelector = 'h1.editor-post-title'; | ||
| await editorFrame.locator( titleSelector ).waitFor( { timeout: 30_000 } ); |
There was a problem hiding this comment.
Performance: Long timeout
The 30-second timeout seems excessive for an iframe element to load. Consider:
- Reducing to 15-20 seconds if this typically loads faster
- Adding a comment explaining why 30s is needed (e.g., slow CI environments)
// Wait for title to be available in iframe (long timeout for slower CI environments)
await editorFrame.locator( titleSelector ).waitFor( { timeout: 30_000 } );
| const publishButton = page.locator( 'button.editor-post-publish-button__button' ).first(); | ||
| await publishButton.waitFor( { state: 'visible', timeout: 10_000 } ); | ||
| await publishButton.click(); |
There was a problem hiding this comment.
Flaky Test Risk: Selector fragility
Using .first() on a selector that matches multiple buttons is fragile. If WordPress changes the order or adds more buttons, this could break or click the wrong button.
Consider more specific selectors:
const publishButton = page.locator( '.editor-header__settings button.editor-post-publish-button__button' );Or use a more semantic selector like getByRole('button', { name: /publish/i }) if the button text is stable.
| await publishButton.click(); | ||
|
|
||
| // Wait for and click the confirm publish button in the panel | ||
| const confirmPublishButton = page.locator( 'button.editor-post-publish-button__button' ).last(); |
There was a problem hiding this comment.
Flaky Test Risk: Using .last() is fragile
Similar to the previous comment, using .last() is brittle. The "confirm publish" button should have a more specific selector or be located within the publish panel:
const confirmPublishButton = page.locator( '.editor-post-publish-panel button.editor-post-publish-button__button' );
| await expect( page.locator( '.components-snackbar' ) ).toBeVisible( { timeout: 10_000 } ); | ||
|
|
||
| // Verify post was created by visiting posts list | ||
| await page.goto( getUrlWithAutoLogin( `${ wpAdminUrl }/edit.php` ) ); |
There was a problem hiding this comment.
Cross-Platform: URL construction
Same as the earlier comment - consider using new URL():
await page.goto( getUrlWithAutoLogin( new URL( '/wp-admin/edit.php', wpAdminUrl ).toString() ) );
📊 Performance Test ResultsComparing d723766 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
Related issues
Proposed Changes
Testing Instructions
Pre-merge Checklist