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

Add tests for EDUs during fast joins #465

Merged
merged 21 commits into from
Oct 26, 2022
Merged

Add tests for EDUs during fast joins #465

merged 21 commits into from
Oct 26, 2022

Conversation

MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Sep 16, 2022

The typing test will fail until matrix-org/synapse#13830 is merged.

Presence test is not testing the sync because it seems to be lost sometimes, I haven't investigated yet.
At least it doesn't fail and block the transaction like with typing.

Signed-off-by: Mathieu Velten <mathieuv@matrix.org>
@MatMaul MatMaul requested review from a team as code owners September 16, 2022 14:41
@MatMaul MatMaul changed the title Add tests for typing/presence/to_device EDUs during fast joins Add tests for typing/presence/to_device/receipt EDUs during fast joins Sep 19, 2022
@MatMaul MatMaul changed the title Add tests for typing/presence/to_device/receipt EDUs during fast joins Add tests for EDUs during fast joins Sep 19, 2022
DMRobertson
DMRobertson previously approved these changes Sep 20, 2022
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Seems reasonable. I've left a few thoughts but nothing that I think you necessarily need to action.

tests/federation_room_join_partial_state_test.go Outdated Show resolved Hide resolved
tests/federation_room_join_partial_state_test.go Outdated Show resolved Hide resolved
@MatMaul MatMaul self-assigned this Sep 22, 2022
@DMRobertson DMRobertson self-assigned this Oct 10, 2022
@DMRobertson
Copy link
Contributor

I think some of these failures are expected per Sean's comment here.

For each failing test, I need to

  • to confirm the failures are expected
  • skipping it
  • raise a Synapse TODO bug

@DMRobertson
Copy link
Contributor

DMRobertson commented Oct 21, 2022

In the light of Sean's comment #465 (comment) I'd like to write a test which checks you can still see incoming messages during resync after the server receives a presence EDU. But reviews welcome nonetheless.

Edit: oh, that was bodged into an existing test in 7331ebc. Let's get this reviewed then.

@DMRobertson DMRobertson requested a review from a team October 21, 2022 20:53
@DMRobertson DMRobertson dismissed their stale review October 24, 2022 11:54

subsequent changes need eyes

tests/federation_room_join_partial_state_test.go Outdated Show resolved Hide resolved
tests/federation_room_join_partial_state_test.go Outdated Show resolved Hide resolved
tests/federation_room_join_partial_state_test.go Outdated Show resolved Hide resolved
})

// we should be able to receive device list update EDU over federation during the resync
t.Run("CanReceiveDeviceListUpdateDuringPartialStateJoin", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit dubious about this test TBH, I don't think it tests anything particularly useful as is and may just cause confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully follow this either. Shouldn't the device list update be send to Alice, even if she and Derek aren't in an encrypted room?

Copy link
Contributor

Choose a reason for hiding this comment

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

As in: Alice should get told via /sync that Derek's device list has changed.

Copy link
Member

Choose a reason for hiding this comment

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

In Synapse currently, yes, though its a bit of a grey area and we've certainly talked about only sending device list updates for E2EE enabled devices and/or users who share rooms.

OTOH, I think all the current integration tests assume this, so I don't see a problem assuming it here

@DMRobertson DMRobertson merged commit 6a3248f into main Oct 26, 2022
@DMRobertson DMRobertson deleted the mv/fast-join-edus branch October 26, 2022 09:24
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.

4 participants