Conversation
also removed createContact msg req logic
| await clickOnElement({ | ||
| window, | ||
| strategy: 'data-testid', | ||
| selector: 'session-confirm-ok-button', | ||
| }); |
There was a problem hiding this comment.
This can be simplified to use clickOn instead:
await clickOn(window, Global.confirmButton);
There was a problem hiding this comment.
replaced a bunch of the existing ones too
| export async function waitForElement({ | ||
| selector, | ||
| strategy, | ||
| window, | ||
| maxWaitMs, | ||
| shouldLog, | ||
| text, | ||
| }: { | ||
| window: Page; | ||
| strategy: Strategy; | ||
| selector: string; | ||
| maxWaitMs?: number; | ||
| text?: string; | ||
| shouldLog: boolean; | ||
| }) { |
There was a problem hiding this comment.
This pattern discourages the use of the centralized locator classes that have been introduced in an earlier PR. I would prefer something like this:
export async function waitFor(
window: Page,
locator: StrategyExtractionObj,
options?: { maxWait?: number; text?: string; shouldLog?: boolean },
)
A legacy wrapper (similar to clickOnElement -> clickOn) might help in keeping this minimal effort.
There was a problem hiding this comment.
In a perfect world we'd then have a ClickOptions and a WaitOptions type (replacing the current ElementOptions) but that might be going too far for this PR.
There was a problem hiding this comment.
I have implemented the first comment as part of this PR
| export async function waitForMatchingText( | ||
| window: Page, | ||
| text: string, | ||
| maxWait?: number, | ||
| maxWait: number, | ||
| ) { | ||
| const builtSelector = `css=:has-text("${text}")`; | ||
| const maxTimeout = maxWait ?? 55000; |
There was a problem hiding this comment.
I'd rather pick a sensible default (e.g. 10-15s) and let the caller decide if they want to deviate from that
There was a problem hiding this comment.
I think the caller should pick the sensible default, as this is the one that has the context. Hence why I think this should always be provided.
Some of those calls are looking for a context menu item to show after a right click. And they were waiting for 55s until they eventually failed, when 50ms would have been enough to know for sure.
| const banMeUnbanLaterMsg = `Ban me but unban me later! - ${Date.now()}`; | ||
| const bannedCheckMsg = `I'm banned :( - ${Date.now()}`; | ||
| const msg3 = `Freedom! - ${Date.now()}`; |
There was a problem hiding this comment.
Odd choice to rename 2 out of 3 messages only.
There was a problem hiding this comment.
renamed to freedomNowMsg
| // Now, try to remove a new message as Alice (admin) sent by Bob | ||
| console.log( | ||
| `Now, try to remove a new message as ${alice.userName} (admin) sent by ${bob.userName}`, | ||
| ); |
There was a problem hiding this comment.
Without knowing the deletion code I can't say if this is the same code path for desktop; but in my mind, a member being able to global delete, and an admin being able to global delete, are two different things.
If bob's deletion fails the test never gets to checking if an admin can delete. Therefore these should be independent tests.
There was a problem hiding this comment.
I have tweaked this, let me know what you think
| test_group_Alice_2W_Bob_1W_Charlie_1W( | ||
| 'Delete message locally in group', |
There was a problem hiding this comment.
It doesn't look like we're checking anything related to an admin's linked devices so this can probably be simplified to test_group_Alice_1W_Bob_1W_Charlie_1W
| await deleteMessageFor( | ||
| charlieWindow1, | ||
| deletedMessageFromBob2, | ||
| 'device_only', |
There was a problem hiding this comment.
These are independent events. If Bob's local deletion of his own message fails we get no test result on Charlie deleting someone else's message locally. These should be separate tests.
There was a problem hiding this comment.
The comments in this file expand on this but the 100% test coverage would be as follows. Maybe a job for a follow-up PR.
- Admin deletes outgoing (local and global)
- Admin deletes incoming (local and global)
- Member deletes outgoing (local and global)
- Member deletes incoming (local only)
| // Now that the user is unban, check that we can ban and delete all. | ||
| // Note: a single test is doing all of those steps because having two of them on the same seed turns out to make both unreliable |
There was a problem hiding this comment.
I understand the limitation but this now depends on every step working along the path.
Would it be too silly to assign a second admin seed for this test?
2 participants
No description provided.