Remove messy WithWaitForLeave test clean-up in TestPartialStateJoin#894
Remove messy WithWaitForLeave test clean-up in TestPartialStateJoin#894MadLittleMods wants to merge 4 commits into
WithWaitForLeave test clean-up in TestPartialStateJoin#894Conversation
| // considered offline and interfere with subsequent tests. | ||
| t.Log("Cleaning up after test...") | ||
|
|
||
| awaitPartialStateJoinCompletion(t, psj.ServerRoom, psj.User) |
There was a problem hiding this comment.
LLM review likes to call out that awaitPartialStateJoinCompletion(...) is necessary in order for the server handlers in handleStateIdsRequests(...)/handleStateRequests(...) to finish before the test does but I don't think it really gives any guarantee of anything.
What it's trying to say is that calling psj.fedStateIdsSendResponseWaiter.Finish() wakes up the waiter so the handler goroutine can keep running but we don't wait for that handler to finish here in Destroy(...). The LLM claims that awaitPartialStateJoinCompletion(...) (requests /members on the homeserver) makes sure that the /state_ids or /state goroutine handlers finish because those requests are necessary to resync the room (partial -> full state) which is necessary in order to serve /members.
That seems like a very tenuous connection. I see the race condition it's explaining but using this is the wrong mechanism.
And the only thing it's trying to protect from is t being used after the test finishes which would panic.
In any case, all of this isn't necessary with the second step of this cleanup in a follow-up PR: #895
And the tests still pass in the mean-time (theoretical flake)
| leaveSharedRoom = func() { | ||
| server2.WithWaitForLeave(t, server2Room, alice, func() { | ||
| alice.MustLeaveRoom(t, roomID) | ||
| }) | ||
| } |
There was a problem hiding this comment.
This in the function called leaveSharedRoom but the leave already happens above.
For context, this was added in #627 which was addressing the flakes caused by homeservers still participating in rooms after each test.
Which as the PR description explains, we have a better solution for now.
| // It is idempotent and must be called once the test is finished. | ||
| // Specifically, it ensures that the partial state join completes and makes the joining user leave | ||
| // the room. | ||
| func (psj *partialStateJoinResult) Destroy(t *testing.T) { |
There was a problem hiding this comment.
As a follow-up, we're removing this entire need for this function in #895
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
I'm a little confused by the commit history. Did you mean to revert the first commit via the second @MadLittleMods?
|
@anoadragon453 Yes. I tried removing everything, This is a first step which just removes |
2 participants
Remove messy
WithWaitForLeavetest clean-up inTestPartialStateJoinFollow-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.
WithWaitForLeavewas originally introduced in this context:But thanks to #880 we no longer use the same
hostname:portfor engineered homeservers anymore.And we can go even further with cleaning this up by removing the need for
Destroy(...)altogether, see #895Pull Request Checklist