-
-
Notifications
You must be signed in to change notification settings - Fork 21
Fixed flakey test for unread notifications #1325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,22 +7,18 @@ Feature: Notifications | |
| When an authenticated request is made to "/.ghost/activitypub/v1/notifications?limit=200" | ||
| Then the request is rejected with a 400 | ||
|
|
||
| Scenario: Requests for unread notifications count | ||
| Scenario: New notifications are marked as unread | ||
| Given we are following "Alice" | ||
| And we are not following "Bob" | ||
| When we get a like notification from "Alice" | ||
| And we get a like notification from "Bob" | ||
| And we get a reply notification from "Alice" | ||
| And we get a reply notification from "Bob" | ||
| Then the unread notifications count is 4 | ||
| Then we have unread notifications | ||
|
|
||
| Scenario: Reset unread notifications count | ||
| Scenario: Visiting the notifications page marks notifications as read | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
| Given we are following "Alice" | ||
| And we are not following "Bob" | ||
| And we get a like notification from "Alice" | ||
| And we get a like notification from "Bob" | ||
| And we get a reply notification from "Alice" | ||
| And we get a reply notification from "Bob" | ||
| And the unread notifications count is 4 | ||
| When we reset unread notifications count | ||
| Then the unread notifications count is 0 | ||
| And we have unread notifications | ||
| When we visit the notifications page | ||
| Then all notifications are marked as read | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,7 +54,6 @@ export async function waitForItemInNotifications( | |
| } | ||
|
|
||
| export async function waitForUnreadNotifications( | ||
| unreadNotificationCount, | ||
| options = { | ||
| retryCount: 0, | ||
| delay: 0, | ||
|
|
@@ -68,23 +67,59 @@ export async function waitForUnreadNotifications( | |
|
|
||
| const responseJson = await response.clone().json(); | ||
|
|
||
| const found = responseJson.count === unreadNotificationCount; | ||
| const unreadNotifications = responseJson.count > 0; | ||
|
|
||
| if (found) { | ||
| return found; | ||
| if (unreadNotifications) { | ||
| return true; | ||
| } | ||
|
|
||
| if (options.retryCount === MAX_RETRIES) { | ||
| throw new Error( | ||
| `Max retries reached (${MAX_RETRIES}) when waiting for unread notifications. No unread notifications found.`, | ||
| ); | ||
| } | ||
|
|
||
| if (options.delay > 0) { | ||
| await new Promise((resolve) => setTimeout(resolve, options.delay)); | ||
| } | ||
|
|
||
| return await waitForUnreadNotifications({ | ||
| retryCount: options.retryCount + 1, | ||
| delay: options.delay + 500, | ||
| }); | ||
| } | ||
|
|
||
| export async function waitForZeroUnreadNotifications( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not following how this is any different to |
||
| options = { | ||
| retryCount: 0, | ||
| delay: 0, | ||
| }, | ||
| ) { | ||
| const MAX_RETRIES = 5; | ||
|
|
||
| const response = await fetchActivityPub( | ||
| 'https://self.test/.ghost/activitypub/v1/notifications/unread/count', | ||
| ); | ||
|
|
||
| const responseJson = await response.clone().json(); | ||
|
|
||
| const zeroUnreadNotifications = responseJson.count === 0; | ||
|
|
||
| if (zeroUnreadNotifications) { | ||
| return true; | ||
| } | ||
|
|
||
| if (options.retryCount === MAX_RETRIES) { | ||
| throw new Error( | ||
| `Max retries reached (${MAX_RETRIES}) when waiting for notifications count ${unreadNotificationCount}. Notification count found ${responseJson.count}`, | ||
| `Max retries reached (${MAX_RETRIES}) when waiting for zero unread notifications. Unread notifications found: ${responseJson.count}.`, | ||
| ); | ||
| } | ||
|
|
||
| if (options.delay > 0) { | ||
| await new Promise((resolve) => setTimeout(resolve, options.delay)); | ||
| } | ||
|
|
||
| return await waitForUnreadNotifications(unreadNotificationCount, { | ||
| return await waitForZeroUnreadNotifications({ | ||
| retryCount: options.retryCount + 1, | ||
| delay: options.delay + 500, | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 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