Fixed flakey test for unread notifications

[sagzy]

closes https://linear.app/ghost/issue/PROD-2663

• The notification count test scenario was flaky when running the entire cucumber test suite, returning a count of 6 instead of 4, sometimes
• We’re not testing the exact count of unread notifications anymore, but rather whether new notifications are marked as unread / visiting the notifications marks them as read - which is closer to the main use case for the notification badge

[coderabbitai]

Walkthrough

Renamed two notification scenarios in features/notifications.feature to check for presence/absence of unread notifications and added visiting the notifications page to mark them read. In features/step_definitions/notifications_steps.js: removed the explicit unread-count assertion, imported waitForZeroUnreadNotifications, replaced the reset action with "When we visit the notifications page", and added steps "Then we have unread notifications" and "Then all notifications are marked as read". In features/support/notifications.js: removed the count parameter from waitForUnreadNotifications (now treats any positive count as unread), added waitForZeroUnreadNotifications, adjusted retry/delay recursion and error messages, and exported the new function.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Title Check	✅ Passed	The title succinctly captures the main change of fixing a flaky test related to unread notifications and clearly reflects the nature of the pull request.
Description Check	✅ Passed	The description closes the linked issue, describes the flaky behavior in the notification count test, and explains how the test scenarios were updated to focus on marking notifications as unread or read, which directly aligns with the modifications in the feature files and support code.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch flakey-test/notifications-count

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

features/notifications.feature (1)

13-13: Prefer an explicit Given for the precondition; single step-def verified

Change the precondition to Given to make intent explicit:
features/notifications.feature:13

-    And we reset unread notifications count
+    Given we reset unread notifications count

Verification: the step is implemented once as When('we reset unread notifications count', ...) in features/step_definitions/notifications_steps.js (≈line 79) — no duplicate/ambiguous step definitions found.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5da34b7 and a25fb8a.

📒 Files selected for processing (1)

• features/notifications.feature (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

features/**/*.feature

📄 CodeRabbit inference engine (AGENTS.md)

e2e tests should reside in the features directory

Files:

• features/notifications.feature

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: build / Build Docker Images (linux/amd64, amd64)

🔇 Additional comments (1)

features/notifications.feature (1)

10-10: LGTM: Scenario rename and consolidation look good

Merging the reset flow into this scenario reduces redundancy and clarifies intent.

[mike182uk]

I don't think we should talk about "pages" here - I know this aligns better with the action the user is taking, but its not a pattern we follow in any other scenarios and is potentially confusing / misleading. What about just:

Retrieving notifications marks them all as read

[mike182uk]

I'm not following how this is any different to waitForUnreadNotifications(0)?

[mike182uk]

🤔 My concern is here we are just masking a problem? It is valid that we do expect a specific amount of notifications, and if there are more or less, than something is not correct somewhere

[sagzy]

Closing, as the solution doesn't feel 100% right