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

Don't rely on device list being updated on invite/join for x-signing federation tests #1119

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

neilalexander
Copy link
Contributor

@neilalexander neilalexander commented Aug 13, 2021

The tests were using sync_until_user_in_device_list before, so even if a device list update is generated for the key/signature change, it will still fail if device list updates aren't generated for the invite/join, which seems like a very unfortunate side effect.

There are already separate tests for device list updates being generated on invites/joins, and that's not really what these specific tests should be trying to prove.

@neilalexander neilalexander requested a review from richvdh August 13, 2021 15:08
@richvdh
Copy link
Member

richvdh commented Aug 13, 2021

For links: some of these lines were added in #749.

@richvdh
Copy link
Member

richvdh commented Aug 13, 2021

The problem here is that it's presumably legitimate for a server to send the join event in one /sync and the device update in the next (or vice-versa)

If your only reason for wanting to change these things is "there are already separate tests for device list updates", I'd probably rather leave it as-is rather than risk making the tests flaky again.

@neilalexander
Copy link
Contributor Author

Yeah — it was largely because I still wanted to test the x-signing behaviour but I don't think Dendrite always generates device change notifications when users are invited/join yet, but if it introduces a race then we may have to rethink.

@richvdh
Copy link
Member

richvdh commented Aug 13, 2021

I'm not absolutely convinced that your change will introduce a race - note that #749 added a sync_until_user_in_device_list where there was nothing at all before - having a sync_until_join_event (isn't there a utility method we can use for that?) may well serve the purpose adequately, so if this is a blocker to you we can always try merging it and seeing what happens. But if it's just a matter of holding off until Dendrite implements a bit more stuff, I'd wait for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants