Skip to content
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

FAILING test in dev env for streaming groups and messages at the same time #491

Closed
wants to merge 2 commits into from

Conversation

nmalzieu
Copy link
Contributor

While working on group invite links, I noticed that when joining a group, sometimes Converse didn't detect the new group that had just been joined and did not redirect to it
By commenting parts of the code it seems to be linked to streaming groups & messages at the same time
I actually can make the attached test fail in dev env but not in local: if I streamAllMessages, I can't stream groups anymore!

Capture d’écran 2024-09-12 à 15 59 53

@nmalzieu nmalzieu requested a review from a team as a code owner September 12, 2024 14:00
@nmalzieu nmalzieu changed the title Add a test in dev env for streaming groups and messages at the same time FAILING test in dev env for streaming groups and messages at the same time Sep 12, 2024
@@ -1247,6 +1247,35 @@ test('can stream all groups and conversations', async () => {
return true
})

test('can stream groups and messages', async () => {
for (const env of ['local', 'dev'] as const) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would be the only test in that test suite that runs not only on local env. Is that ok?

// bo creates a group with alix so a stream callback is fired
// eslint-disable-next-line @typescript-eslint/no-unused-vars
await boClient.conversations.newGroup([alixClient.address])
await delayToPropogate()
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested passing in a longer value to delayToPropogate here (default is 100 ms). Passing in 2000 ms, test passed 5/5 times for me on ios and android. There could just be network delays initiating the stream.

To better deal with this flakiness, I think we need to update these tests to use Promise/Resolve instead of relying on delays, see this draft PR: https://github.com/xmtp/xmtp-react-native/pull/284/files

(I think at the time that PR was made, there was other issues that were breaking streams, but now that streams are more stable now might be a good time to revisit that improvement for tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah interesting, so I might have written a wrong test for a real problem I have in the app (that has nothing to do with 100ms I was waiting like 10 secs). I'll keep investigating then.

@nplasterer
Copy link
Contributor

I think this is fixed now.

@nplasterer nplasterer closed this Oct 3, 2024
@nplasterer nplasterer deleted the noe/stream-groups-and-messages-test branch October 3, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants