Remove messy `Destroy` test clean-up in `TestPartialStateJoin` by MadLittleMods · Pull Request #895 · matrix-org/complement

MadLittleMods · 2026-07-13T22:28:28Z

Remove messy Destroy test clean-up in TestPartialStateJoin

Follow-up to #894 which removes another part of the clean-up code in this file and #880 which introduces a better way to avoid the test pollution and obsoletes the need for all of this cleanup logic. See the PR description there for a more complete context on why.

Destroy(...) was originally introduced in this context:

Since the partial state join tests reuse the same homeserver deployment, it is important that each test leaves the homeserver in an acceptable state for subsequent tests. In particular, we must allow in-progress partial-state joins to complete before tearing down Complement servers, otherwise the homeserver may mark Complement hostname:port combinations as offline and refuse to contact them in subsequent tests.

-- #570

But thanks to #880 we no longer use the same hostname:port for engineered homeservers anymore.

Todo

Wait for Remove messy WithWaitForLeave test clean-up in TestPartialStateJoin #894 to merge

Pull Request Checklist

Pull request includes a sign off

All of this code has been obsoleted by #880

MadLittleMods · 2026-07-13T22:30:00Z

 		defer cancel()
 		serverRoom := createTestRoom(t, server, alice.GetDefaultRoomVersion(t))
 		psjResult := beginPartialStateJoin(t, server, serverRoom, alice)
-		defer psjResult.Destroy(t)


Removing all of the Destroy usage since it's no longer necessary

MadLittleMods · 2026-07-13T22:33:20Z

-
-	// The caller is about to tear down the Complement homeserver. Leave the room, so
-	// that the homeserver under test stops sending it presence updates.
-	psj.Server.WithWaitForLeave(


#894 removes the WithWaitForLeave(...) part of Destroy(...)

This PR removes the need for finishing the waiters.

Which means Destroy(...) doesn't do anything anymore and we can remove it altogether. Less to think about.

MadLittleMods · 2026-07-13T22:47:51Z

-// wait is skipped when `user` had already left the room (per their own
-// homeserver, so the action produces no new leave) or when this server isn't in
-// the room (so the leave won't be federated to us).
-func (s *server) WithWaitForLeave(


The WithWaitForLeave changes in the diff disappear once #894 is merged

This PR builds on that one so they are also included here for now.

…n` (#894) Follow-up to #880 which introduces a better way to avoid the test pollution and obsoletes the need for all of this cleanup logic. See the PR description there for a more complete context on why. `WithWaitForLeave` was originally introduced in this context: > 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. > > *-- #626 But thanks to #880 we no longer use the same `hostname:port` for engineered homeservers anymore. And we can go even further with cleaning this up by removing the need for `Destroy(...)` altogether, see #895

…oin-cleanup2 Conflicts: tests/msc3902/federation_room_join_partial_state_test.go

MadLittleMods · 2026-07-15T20:38:52Z

+							"to serve /state_ids response for event %s", queryParams["event_id"],
+					)
+				}
+


This whole handler is the same as handleStateIdsRequests(...) except that it's serving an invalid response.

It's very tempting to dry this out but it's prior art ⏩

See #895 (comment) > Before Go 1.19, headings were identified implicitly by single-line paragraphs satisfying certain conditions, most notably the lack of any terminating punctuation. > > Gofmt reformats [lines treated as implicit headings](https://github.com/golang/proposal/blob/master/design/51082-godocfmt.md#headings) by earlier versions of Go to use # headings instead. If the reformatting is not appropriate—that is, if the line was not meant to be a heading—the easiest way to make it a paragraph is to introduce terminating punctuation such as a period or colon, or to break it into two lines. > > *-- https://go.dev/doc/comment#headings*

MadLittleMods · 2026-07-15T21:16:53Z

+				select {
+				case <-sendResponseWaiter.Done():
+					// Happy-path now that we're done waiting, continue serving the request now
+				case <-req.Context().Done():


Instead of relying on Destroy to finish the waiter to allow this to continue, we instead rely on engineered homeserver being closed and all of the in-flight requests being cancelled (req.Context().Done()).

We still have a 60 second timeout like before.

This also fixes the goroutine panic that can happen when the waiter finishes after the test and t is used after the test finishes, see #894 (comment)

See #895 (comment) Here is what an LLM said: > `awaitPartialStateJoinCompletion` has the given user make a /members call, which blocks until the partial-state resync completes — a way to keep the homeserver from still trying to contact the Complement server after teardown. `beginPartialStateJoin` defaults `psjResult.User` to the joining user (alice), but in these two subtests alice leaves the room mid-test, so she couldn't be the one to make that /members call at cleanup time. Bob stays joined, so the tests swapped in bob: `psjResult.User = bob`. That's what the comment "At the end of the test, keep Bob in the room. Have him make a `/members` call..." was referring to. And that it's "now dead code, orphaned by your own cleanup"

MadLittleMods added 4 commits July 13, 2026 17:15

Remove WithWaitForLeave

8924c7a

Remove awaitPartialStateJoinCompletion from Destroy

a68ec08

Remove the need to Destroy

49e76c8

Remove Destroy usage

4e36144

All of this code has been obsoleted by #880

MadLittleMods mentioned this pull request Jul 13, 2026

Remove messy WithWaitForLeave test clean-up in TestPartialStateJoin #894

Merged

1 task

MadLittleMods commented Jul 13, 2026

View reviewed changes

Comment thread tests/msc3902/federation_room_join_partial_state_test.go

MadLittleMods commented Jul 13, 2026

View reviewed changes

Update docstring to indicate it no longer needs to be destroyed

ea5943f

MadLittleMods commented Jul 13, 2026

View reviewed changes

MadLittleMods marked this pull request as ready for review July 13, 2026 22:49

MadLittleMods requested review from a team as code owners July 13, 2026 22:49

MadLittleMods added 2 commits July 15, 2026 15:29

Merge branch 'main' into madlittlemods/remove-messy-TestPartialStateJ…

ea9749d

…oin-cleanup2 Conflicts: tests/msc3902/federation_room_join_partial_state_test.go

Apply pattern to another handler

0bc0466

MadLittleMods commented Jul 15, 2026

View reviewed changes

Comment thread tests/msc3902/federation_room_join_partial_state_test.go Outdated

MadLittleMods commented Jul 15, 2026

View reviewed changes

Comment thread tests/msc3902/federation_room_join_partial_state_test.go Outdated

MadLittleMods commented Jul 15, 2026

View reviewed changes

MadLittleMods mentioned this pull request Jul 16, 2026

Fix cross-test pollution from engineered homeservers re-using the same server names (fix TestPartialStateJoin flakes) #880

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove messy `Destroy` test clean-up in `TestPartialStateJoin`#895

Remove messy `Destroy` test clean-up in `TestPartialStateJoin`#895
MadLittleMods wants to merge 9 commits into
mainfrom
madlittlemods/remove-messy-TestPartialStateJoin-cleanup2

MadLittleMods commented Jul 13, 2026 •

edited

Loading

Uh oh!

MadLittleMods Jul 13, 2026

Uh oh!

Uh oh!

MadLittleMods Jul 13, 2026

Uh oh!

MadLittleMods Jul 13, 2026 •

edited

Loading

Uh oh!

MadLittleMods Jul 15, 2026

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Jul 15, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

MadLittleMods commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo

Pull Request Checklist

Uh oh!

MadLittleMods Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MadLittleMods Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

MadLittleMods commented Jul 13, 2026 •

edited

Loading

MadLittleMods Jul 13, 2026 •

edited

Loading

MadLittleMods Jul 15, 2026 •

edited

Loading