Skip to content

Deflake faster joins device list tests by waiting for leave event#627

Merged
squahtx merged 4 commits intomainfrom
squah/faster_room_joins_fix_test_flakes_2
Mar 16, 2023
Merged

Deflake faster joins device list tests by waiting for leave event#627
squahtx merged 4 commits intomainfrom
squah/faster_room_joins_fix_test_flakes_2

Conversation

@squahtx
Copy link
Contributor

@squahtx squahtx commented Mar 14, 2023 •
edited
Loading

Many of the faster joins test flakes are due to the homeserver under
test failing to contact Complement homeservers after they have been
torn down. When this happens, subsequent tests can fail if they use a
Complement homeserver that happens to have the same hostname:port as one
which the homeserver under test has previously marked as offline.

Wait for the homeserver under test to finish broadcasting its leave at
the end of the device list tests.

Signed-off-by: Sean Quah seanq@matrix.org

Builds on top of #626.
Reviewable commit by commit.

NB: you can tease out the flakes by forcing Complement to reuse ports whenever possible:

diff --git a/internal/federation/server.go b/internal/federation/server.go
index ed7974d..4c4ccbe 100644
--- a/internal/federation/server.go
+++ b/internal/federation/server.go
@@ -455,7 +455,10 @@ func (s *Server) Listen() (cancel func()) {
        var wg sync.WaitGroup
        wg.Add(1)

-       ln, err := net.Listen("tcp", ":0") //nolint
+       ln, err := net.Listen("tcp", ":63333") //nolint
+       if err != nil {
+               ln, err = net.Listen("tcp", ":63334") //nolint
+       }
        if err != nil {
                s.t.Fatalf("ListenFederationServer: net.Listen failed: %s", err)
        }

@squahtx squahtx requested review from a team and kegsay as code owners March 14, 2023 17:20
@squahtx
Copy link
Contributor Author

squahtx commented Mar 14, 2023

CI flaked due to the issue fixed by #628.

Sean Quah added 4 commits March 15, 2023 10:44
Add MustJoinRoom option to allow Complement to do a partial state join
4d31726 
Some of the faster joins tests have two Complement homeservers in the
same room. We need both Complement homeservers to believe they are in
the room, so that we can wait to observe a leave for the test user as
part of test cleanup. The easiest way to do this is to have the second
Complement homeserver perform a real join.

Signed-off-by: Sean Quah <seanq@matrix.org>
Allow Complement homeservers to contact each other
5b64af3 
This is useful for setting up tests with two Complement homeservers in
the same room by having one of them join off the other.

Signed-off-by: Sean Quah <seanq@matrix.org>
Have the second Complement homeserver join rooms properly
e7f7e71 
...so that we can wait for the homeserver under test to send us leave
events during test cleanup.

Signed-off-by: Sean Quah <seanq@matrix.org>
Leave rooms in test cleanup: device list tests
926a40f 
This depends on the second Complement homeserver believing it is joined
to the test room.

Signed-off-by: Sean Quah <seanq@matrix.org>
@squahtx squahtx force-pushed the squah/faster_room_joins_fix_test_flakes_2 branch from 632ac7b to 926a40f Compare March 15, 2023 10:44
DMRobertson
DMRobertson approved these changes Mar 16, 2023
View reviewed changes
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.

Looks like this was painful to diagnose; thank you for persisting.

internal/federation/server.go
// MustJoinRoom will make the server send a make_join and a send_join to join a room
// It returns the resultant room.
func (s *Server) MustJoinRoom(t *testing.T, deployment *docker.Deployment, remoteServer gomatrixserverlib.ServerName, roomID string, userID string) *ServerRoom {
func (s *Server) MustJoinRoom(t *testing.T, deployment *docker.Deployment, remoteServer gomatrixserverlib.ServerName, roomID string, userID string, partialState ...bool) *ServerRoom {
Copy link
Contributor

@DMRobertson DMRobertson Mar 16, 2023

Choose a reason for hiding this comment

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

I guess this is go's idiom for an optional argument? Feels a bit odd to allow an arbitrary length array/slice, but I guess it's easier than updating every call site?

Copy link
Contributor Author

@squahtx squahtx Mar 16, 2023

Choose a reason for hiding this comment

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

Not sure actually. Functional varargs (https://stackoverflow.com/a/26326418) is the pattern MustDoFunc uses. Here I didn't bother with them since there's only one optional argument.

This was linked to issues Mar 16, 2023
Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_no_longer_reach_departed_servers_after_partial_state_join_completes is flakey matrix-org/synapse#13977
Closed
Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_newly_joined_servers_in_partial_state_rooms is flakey matrix-org/synapse#14010
Closed
Complement TestPartialStateJoin/Device_list_tracking/Device_list_no_longer_tracked_when_new_member_leaves_partial_state_room is flakey matrix-org/synapse#14306
Closed
Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_incorrectly_kicked_servers_once_partial_state_join_completes_even_though_remote_server_left_room is flakey matrix-org/synapse#14432
Closed
Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_all_servers_in_partial_state_rooms is flakey matrix-org/synapse#14475
Closed
Complement TestPartialStateJoin/Device_list_tracking/Device_list_tracking_for_pre-existing_members_in_partial_state_room is flakey matrix-org/synapse#14506
Closed
Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_incorrectly_absent_servers_once_partial_state_join_completes_even_though_remote_server_left_room is flakey? matrix-org/synapse#14853
Closed
Complement flake TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_incorrectly_kicked_servers_once_partial_state_join_complete matrix-org/synapse#15039
Closed
@DMRobertson
Copy link
Contributor

DMRobertson commented Mar 16, 2023

(I've optimistically marked this as closing a bunch of faster join device list updates tests---hope I correctly matched up tests to this issue)

@squahtx squahtx merged commit 646d35f into main Mar 16, 2023
@squahtx squahtx deleted the squah/faster_room_joins_fix_test_flakes_2 branch March 16, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kegsay kegsay Awaiting requested review from kegsay kegsay is a code owner

1 more reviewer

@DMRobertson DMRobertson DMRobertson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_all_servers_in_partial_state_rooms is flakey Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_incorrectly_kicked_servers_once_partial_state_join_completes_even_though_remote_server_left_room is flakey Complement TestPartialStateJoin/Device_list_tracking/Device_list_no_longer_tracked_when_new_member_leaves_partial_state_room is flakey Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_newly_joined_servers_in_partial_state_rooms is flakey Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_no_longer_reach_departed_servers_after_partial_state_join_completes is flakey
Complement TestPartialStateJoin/Device_list_tracking/Device_list_tracking_for_pre-existing_members_in_partial_state_room is flakey Complement TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_incorrectly_absent_servers_once_partial_state_join_completes_even_though_remote_server_left_room is flakey? Complement flake TestPartialStateJoin/Outgoing_device_list_updates/Device_list_updates_reach_incorrectly_kicked_servers_once_partial_state_join_complete

2 participants

@squahtx @DMRobertson