Skip to content

Better assert when TestDelayedEvents/delayed_state_events_are_kept_on_server_restart take too long to run for our assumptions to be correct #829

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.

Sign up for GitHub

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

Merged
MadLittleMods merged 2 commits into from
Dec 19, 2025
Merged

Better assert when TestDelayedEvents/delayed_state_events_are_kept_on_server_restart take too long to run for our assumptions to be correct #829

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42df759
Better assert when the test takes too long to run
MadLittleMods Dec 17, 2025
f21e3df
Remove extra `has`
MadLittleMods Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions tests/msc4140/delayed_event_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@ func TestDelayedEvents(t *testing.T) {
client.WithJSONBody(t, map[string]interface{}{}),
getDelayQueryParam("900"),
)
beforeScheduleStateTimestamp2 := time.Now()
user.MustDo(
t,
"PUT",
Expand All @@ -466,6 +467,15 @@ func TestDelayedEvents(t *testing.T) {
time.Sleep(1 * time.Second)
deployment.StartServer(t, hsName)

// The rest of the test assumes the second delayed event (10 second delay) still
// hasn't been sent yet.
if time.Now().Sub(beforeScheduleStateTimestamp2) > 10*time.Second {
t.Fatalf(
"Test took too long to run, cannot guarantee delayed event timings. " +
"More than 10 seconds elapsed between scheduling the delayed event and now when we're about to check for it.",
)
}
Copy link
Collaborator Author

@MadLittleMods MadLittleMods Dec 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually fixing this and making this kind of test consistent is tricky.

If we just increase the delay time, then the test will just take longer to run even in the fast scenarios. Ideally, the second delayed event should be delayed by at-least COMPLEMENT_SPAWN_HS_TIMEOUT_SECS (defaults to 30 seconds) since we're stopping and starting servers here 🤔 - although we use COMPLEMENT_SPAWN_HS_TIMEOUT_SECS=120 with the Synapse workers variant and two minutes sounds painful to wait for.

Copy link
Collaborator Author

@MadLittleMods MadLittleMods Dec 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We could setup a series of delayed events at 10 second intervals up to the COMPLEMENT_SPAWN_HS_TIMEOUT_SECS. Then we just want to look for a decrement after the server restart. This keeps the test as fast as before while being lenient for however long it takes to restart.

I'm going to address this in a follow-up PR.

Copy link
Collaborator Author

@MadLittleMods MadLittleMods Dec 18, 2025

Choose a reason for hiding this comment

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

Updated in #830


matchDelayedEvents(t, user, 1)
user.MustDo(t, "GET", getPathForState(roomID, eventType, stateKey1))

Expand Down
Loading