Skip to content

Fixed flaky bookmark and call-to-action tests#26834

Merged
EvanHahn merged 4 commits intomainfrom
cmraible/fix-bookmark-test
Mar 17, 2026
Merged

Fixed flaky bookmark and call-to-action tests#26834
EvanHahn merged 4 commits intomainfrom
cmraible/fix-bookmark-test

Conversation

@cmraible
Copy link
Copy Markdown
Collaborator

@cmraible cmraible commented Mar 16, 2026

refs https://github.com/TryGhost/Ghost/actions/runs/23157241597/job/67275778166

This is a test only change; no user-facing changes.

Summary

  • Extracted an openSlashMenu(page, command) helper that types the slash command with a delay and waits for the menu to render before continuing
  • Applied the helper to all 5 slash menu selection tests (bookmark, call-to-action, product, GIF)
  • Fixed the GIF-not-configured test which previously passed vacuously — it now waits for the slash menu to appear before asserting GIF is absent

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 16, 2026

Walkthrough

Adds an internal helper openSlashMenu(page, command) in an acceptance test to type /<command> with a 50ms delay and wait up to 5s for [data-kg-slash-menu] to appear. Replaces multiple direct page.keyboard.type(...) sequences with calls to the helper, updates subsequent Enter key presses where applicable, and modifies GIF-related assertions to check the slash-menu contents after invoking the helper. No changes to exported APIs or public interfaces.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing flaky tests by extracting and applying a helper function for slash command invocation across multiple tests.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed The pull request description accurately describes the changeset: extracting a helper function, applying it to slash menu tests, and fixing the GIF test synchronization.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cmraible/fix-bookmark-test
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cmraible cmraible marked this pull request as ready for review March 16, 2026 21:31
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts (1)

420-423: Consider adding a wait for consistency with other GIF test.

This test checks that the GIF option is not visible, but it doesn't first wait for the slash menu to render. If the menu hasn't appeared yet, the assertion could pass vacuously. The other GIF test (line 464-466) does wait for the menu before checking.

For robustness, consider adding a wait or a brief delay here as well:

💡 Suggested improvement
 await editor.type('/gif');
 
 const slashMenu = page.locator('[data-kg-slash-menu]');
+// Wait briefly to ensure menu has had time to render (if it will)
+await page.waitForTimeout(100);
 await expect(slashMenu.getByText('GIF', {exact: true})).not.toBeVisible();

Alternatively, you could wait for some menu content to appear before asserting the GIF option is absent. This is a minor observation outside the PR's immediate scope.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts`
around lines 420 - 423, The assertion that the GIF option is not visible runs
before ensuring the slash menu has rendered; update the test around
editor.type('/gif') to first wait for the slash menu to appear (e.g., await
slashMenu.waitFor or waitForSelector on '[data-kg-slash-menu]' or wait for some
menu item text) and then assert that slashMenu.getByText('GIF', {exact: true})
is not visible, using the existing slashMenu locator to target the menu.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts`:
- Around line 420-423: The assertion that the GIF option is not visible runs
before ensuring the slash menu has rendered; update the test around
editor.type('/gif') to first wait for the slash menu to appear (e.g., await
slashMenu.waitFor or waitForSelector on '[data-kg-slash-menu]' or wait for some
menu item text) and then assert that slashMenu.getByText('GIF', {exact: true})
is not visible, using the existing slashMenu locator to target the menu.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 454f1fa2-5c0e-4380-8a8e-1095c4969961

📥 Commits

Reviewing files that changed from the base of the PR and between b2823f2 and b7879f0.

📒 Files selected for processing (1)
  • apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts

Consolidates the delay + wait pattern into a reusable helper across
all 6 slash menu test sites. Also fixes the GIF-not-configured test
which previously passed vacuously by not waiting for the menu to render.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts (1)

428-430: Make the GIF-not-configured assertion non-vacuous and reuse the helper.

At Line 430, not.toBeVisible() can still pass when the menu content is missing unexpectedly. Add one positive menu-item assertion first, and reuse openSlashMenu for consistency.

♻️ Proposed refinement
-            await page.keyboard.type('/', {delay: 50});
-            await expect(page.locator('[data-kg-slash-menu]')).toBeVisible({timeout: 5000});
-            await expect(page.locator('[data-kg-slash-menu]').getByText('GIF', {exact: true})).not.toBeVisible();
+            await openSlashMenu(page, '');
+            await expect(page.locator('[data-kg-slash-menu]').getByText('Bookmark', {exact: true})).toBeVisible();
+            await expect(page.locator('[data-kg-slash-menu]').getByText('GIF', {exact: true})).not.toBeVisible();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts`
around lines 428 - 430, The current negative assertion for the GIF menu item is
vacuous; replace the manual slash typing with the shared helper openSlashMenu()
to open the slash menu, then first assert a known positive menu item is visible
(e.g., expect(page.locator('[data-kg-slash-menu]').getByText('Image', {exact:
true})).toBeVisible()) to ensure the menu rendered, and only then assert the GIF
entry is not visible (expect(...getByText('GIF', {exact:
true})).not.toBeVisible()); use the openSlashMenu helper and the
'[data-kg-slash-menu]' locator to locate the menu and items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts`:
- Around line 428-430: The current negative assertion for the GIF menu item is
vacuous; replace the manual slash typing with the shared helper openSlashMenu()
to open the slash menu, then first assert a known positive menu item is visible
(e.g., expect(page.locator('[data-kg-slash-menu]').getByText('Image', {exact:
true})).toBeVisible()) to ensure the menu rendered, and only then assert the GIF
entry is not visible (expect(...getByText('GIF', {exact:
true})).not.toBeVisible()); use the openSlashMenu helper and the
'[data-kg-slash-menu]' locator to locate the menu and items.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9264d26d-0ccc-467a-81e3-7bd72edafa03

📥 Commits

Reviewing files that changed from the base of the PR and between b7879f0 and d921587.

📒 Files selected for processing (1)
  • apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts

Ensures the slash menu items have populated before asserting
GIF is not present, preventing a vacuous negative assertion.
@cmraible cmraible added the ok to merge for me You can merge this on my behalf if you want. label Mar 16, 2026
@cmraible cmraible requested a review from EvanHahn March 16, 2026 23:09
@EvanHahn EvanHahn merged commit 0d3df45 into main Mar 17, 2026
31 checks passed
@EvanHahn EvanHahn deleted the cmraible/fix-bookmark-test branch March 17, 2026 12:54
franky19 pushed a commit to franky19/Ghost that referenced this pull request Apr 18, 2026
refs https://github.com/TryGhost/Ghost/actions/runs/23157241597/job/67275778166

This is a test only change; no user-facing changes.

## Summary

- Extracted an `openSlashMenu(page, command)` helper that types the
slash command with a delay and waits for the menu to render before
continuing
- Applied the helper to all 5 slash menu selection tests (bookmark,
call-to-action, product, GIF)
- Fixed the GIF-not-configured test which previously passed vacuously —
it now waits for the slash menu to appear before asserting GIF is absent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok to merge for me You can merge this on my behalf if you want.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants