Convert Sliding Sync tests to use higher-level `compute_interested_rooms` #18399

devonh · 2025-05-05T21:56:39Z

This updates some sliding sync tests to use a higher level function in order to move test coverage to cover both fallback & new tables. Important when #18375 is merged.

In other words, adjust tests to target compute_interested_room(...) (relevant to both new and fallback path) instead of the lower level get_room_membership_for_user_at_to_token(...) that only applies to the fallback path.

Dev notes

SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync.ComputeInterestedRoomsTestCase_new

SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.rest.client.sliding_sync

SYNAPSE_POSTGRES=1 SYNAPSE_POSTGRES_USER=postgres SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync.ComputeInterestedRoomsTestCase_new.test_display_name_changes_leave_after_token_range

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct
(run the linters)

changelog.d/18399.misc

…ange

This reverts commit d2a4179960e266dc35a06e28c08015570c9a4b21.

…eave

MadLittleMods · 2025-05-06T19:36:15Z

synapse/handlers/sliding_sync/room_lists.py

+                    if filter_membership_for_sync(
+                        user_id=user_id,
+                        room_membership_for_user=room_for_user,
+                        newly_left=room_id in newly_left_room_map,
+                    ):
+                        room_membership_for_user_map[room_id] = room_for_user
+                    else:
+                        room_membership_for_user_map.pop(room_id, None)


Before adding to the room_membership_for_user_map, we now double-check it should be added with the filter_membership_for_sync(...). If it shouldn't be in the map, we also make sure to pop it off.

This is the same thing we're doing in the fallback path just with a lighter touch.

Fallback path (filter_rooms_relevant_for_sync(...) just calls filter_membership_for_sync(...) on each room):

synapse/synapse/handlers/sliding_sync/room_lists.py

Lines 596 to 600 in d0873d5

sync_room_map = await self.filter_rooms_relevant_for_sync(

user=sync_config.user,

room_membership_for_user_map=room_membership_for_user_map,

newly_left_room_ids=newly_left_room_ids,

)

The new path avoids the filter_membership_for_sync(...) call because the results should already be filtered since we already avoided all of that work up-front by only pulling out what we need.

synapse/synapse/handlers/sliding_sync/room_lists.py

Line 417 in 2e520f5

sync_room_map = room_membership_for_user_map

synapse/storage/databases/main/roommember.py

MadLittleMods · 2025-05-06T19:38:40Z

synapse/storage/databases/main/roommember.py

+                    AND (
+                        (m.membership != 'leave' OR m.user_id != m.sender)
+                        OR (m.event_stream_ordering > ?)
+                    )


Other important change is here. Previously, we always filtered out all self-leave events.

Now, we still almost always filter them out except for when they occur after our to_token. This is just a quirk so we can properly rewind to a potentially non-self-leave that should be included in the sync response.

This logic has moved to get_sliding_sync_self_leave_rooms_after_to_token(...)

MadLittleMods · 2025-05-06T19:40:23Z

tests/handlers/test_sliding_sync.py

                from_token=now_token,
                to_token=now_token,
            )
        )
+        room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids)


@devonh Heads-up, from your refactor, I changed our comparison to actually look at the list of room ID's (interested_rooms.lists["foo-list"].ops[0].room_ids) instead of the interested_rooms.room_membership_for_user_map.

The list of rooms between the new and fallback path will be slightly different (the fallback path includes more rooms in interested_rooms.room_membership_for_user_map, more context in #18399 (comment)) but the list of room ID's being returned will be consistent between both.

This also matches what we're doing in the REST layer tests

synapse/tests/rest/client/sliding_sync/test_sliding_sync.py

Lines 664 to 668 in 740fc88

self.assertIncludes(

set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]),

{room_id},

exact=True,

)

Ah - good to know. That makes sense

MadLittleMods · 2025-05-06T19:45:59Z

tests/handlers/test_sliding_sync.py

+        # `room_id1` should not show up because it was left before the token range.
+        # `room_id2` should show up because it is `newly_left` within the token range.
+        self.assertIncludes(
+            room_id_results,
+            {room_id2},


Some of these tests have materially changed since we are looking at the actual list of room ID's being returned instead of the room_membership_for_user_map which was accurate but included self-leave rooms.

Since we're avoiding pulling self-leave events in the new path, it doesn't make sense to care about that now. We only care about the list of rooms being returned to the user.

There should be a comment explaining any change here though. And I think it all makes sense.

synapse/storage/databases/main/roommember.py

See #18399 (comment)

MadLittleMods · 2025-05-06T21:46:41Z

changelog.d/18399.misc

@@ -0,0 +1 @@
+Refactor [MSC4186](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) Simplified Sliding Sync room list tests to cover both new and fallback logic paths.


@devonh You should give this a review pass and then we should potentially put this PR in the review queue for a third party to review.

Good call. I'll take a look through it.
But we should definitely also get some other eyes on it.

@MadLittleMods Thanks for digging into this!

reivilibre

otherwise seems sensible

reivilibre · 2025-05-07T14:27:12Z

tests/handlers/test_sliding_sync.py

+    class_name_func=lambda cls,
+    num,
+    params_dict: f"{cls.__name__}_{'new' if params_dict['use_new_tables'] else 'fallback'}",


wonder if we can convince the formatter to format this differently? This formatting is rather obfuscating

Suggested change

class_name_func=lambda cls,

num,

params_dict: f"{cls.__name__}_{'new' if params_dict['use_new_tables'] else 'fallback'}",

class_name_func=(

lambda cls, num, params_dict:

f"{cls.__name__}_{'new' if params_dict['use_new_tables'] else 'fallback'}"

),

While the formatter seems to be happy with the alternate formatting, running the tests is not.
I get what you're saying about the formatting being unclear, but these shouldn't stick around too much longer since we can remove the fallback functionality of sliding sync now.

…oms` (element-hq#18399) Spawning from element-hq#18375 (comment), This updates some sliding sync tests to use a higher level function in order to move test coverage to cover both fallback & new tables. Important when element-hq#18375 is merged. In other words, adjust tests to target `compute_interested_room(...)` (relevant to both new and fallback path) instead of the lower level `get_room_membership_for_user_at_to_token(...)` that only applies to the fallback path. ### Dev notes ``` SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync.ComputeInterestedRoomsTestCase_new ``` ``` SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.rest.client.sliding_sync ``` ``` SYNAPSE_POSTGRES=1 SYNAPSE_POSTGRES_USER=postgres SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync.ComputeInterestedRoomsTestCase_new.test_display_name_changes_leave_after_token_range ``` ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Eric Eastwood <[email protected]>

Convert tests to use compute_interested_rooms

92b53e4

devonh requested a review from a team as a code owner May 5, 2025 21:56

Add changelog entry

cd4520e

devonh changed the base branch from develop to erikj/ss_reject_invites May 5, 2025 22:09

github-actions bot deployed to PR Documentation Preview May 5, 2025 22:10 Active

devonh changed the base branch from erikj/ss_reject_invites to develop May 5, 2025 22:28

devonh changed the base branch from develop to erikj/ss_reject_invites May 5, 2025 22:29

devonh changed the base branch from erikj/ss_reject_invites to develop May 5, 2025 22:30

devonh changed the base branch from develop to erikj/ss_reject_invites May 5, 2025 22:31

devonh mentioned this pull request May 5, 2025

Pass leave from remote invite rejection down Sliding Sync #18375

Merged

3 tasks

MadLittleMods added the A-Sync label May 5, 2025

MadLittleMods changed the title ~~Convert tests to use compute_interested_rooms~~ Convert Sliding Sync tests to use higher-level compute_interested_rooms May 5, 2025

MadLittleMods reviewed May 5, 2025

View reviewed changes

changelog.d/18399.misc Outdated Show resolved Hide resolved

MadLittleMods force-pushed the devon/ss_test_refactor branch from 8ff7993 to 6dbaf93 Compare May 6, 2025 00:58

MadLittleMods reviewed May 6, 2025

View reviewed changes

changelog.d/18399.misc Outdated Show resolved Hide resolved

Rename test class

a434892

devonh force-pushed the devon/ss_test_refactor branch from 6dbaf93 to a434892 Compare May 6, 2025 15:43

devonh changed the base branch from erikj/ss_reject_invites to develop May 6, 2025 15:43

devonh and others added 12 commits May 6, 2025 09:45

Remove leave membership filter when getting rooms for user

de80574

Merge branch 'develop' into devon/ss_test_refactor

02d7657

Add extra test for not newly_joined or newly_left display name ch…

1b4eb2b

…ange

Remove extra copies

1a046bf

Revert "Remove extra copies"

1794c55

This reverts commit d2a4179960e266dc35a06e28c08015570c9a4b21.

Fix missing self-leave rooms when looking at token range before the l…

a980e10

…eave

Add better notes on how why we do this specific logic

6c4e877

Make state reset test more real and passes with new Sliding Sync path

235a52e

Align other test with real state reset

4ad9671

Fix test in fallback path (more loose criteria)

2fe8e35

Refactor to look at room ID's in actual list

e4b9d01

Fix changes after token showing up in new path

2e2b8bf

MadLittleMods added 4 commits May 6, 2025 14:16

Fix tests

94efd8b

Fix sharded event persisting test case

59b9cff

Align more tests to use assertIncludes

5fadd61

Remove debug logs

2e520f5

MadLittleMods reviewed May 6, 2025

View reviewed changes

Better changelog

addb43c

MadLittleMods self-assigned this May 6, 2025

MadLittleMods reviewed May 6, 2025

View reviewed changes

synapse/storage/databases/main/roommember.py Outdated Show resolved Hide resolved

MadLittleMods added 3 commits May 6, 2025 15:59

Fix cache messing up our self-leave after to_token fetching

dd4104c

See #18399 (comment)

Fixup function

d6eb04a

Fixup reversed filter logic

9cd2098

MadLittleMods removed their assignment May 6, 2025

MadLittleMods reviewed May 6, 2025

View reviewed changes

devonh requested a review from a team May 6, 2025 22:58

reivilibre approved these changes May 7, 2025

View reviewed changes

devonh merged commit ae877aa into develop May 7, 2025
39 checks passed

devonh deleted the devon/ss_test_refactor branch May 7, 2025 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Convert Sliding Sync tests to use higher-level `compute_interested_rooms` #18399

Convert Sliding Sync tests to use higher-level `compute_interested_rooms` #18399

Uh oh!

devonh commented May 5, 2025 •

edited by MadLittleMods

Loading

Uh oh!

Uh oh!

Uh oh!

MadLittleMods May 6, 2025

Uh oh!

Uh oh!

MadLittleMods May 6, 2025

Uh oh!

MadLittleMods May 6, 2025

Uh oh!

MadLittleMods May 6, 2025 •

edited

Loading

Uh oh!

devonh May 6, 2025

Uh oh!

MadLittleMods May 6, 2025 •

edited

Loading

Uh oh!

Uh oh!

MadLittleMods May 6, 2025

Uh oh!

devonh May 6, 2025

Uh oh!

devonh May 6, 2025

Uh oh!

reivilibre left a comment

Uh oh!

reivilibre May 7, 2025

Uh oh!

devonh May 7, 2025

Uh oh!

Uh oh!

Uh oh!

	sync_room_map = await self.filter_rooms_relevant_for_sync(
	user=sync_config.user,
	room_membership_for_user_map=room_membership_for_user_map,
	newly_left_room_ids=newly_left_room_ids,
	)

	self.assertIncludes(
	set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]),
	{room_id},
	exact=True,
	)

		@@ -0,0 +1 @@
		Refactor [MSC4186](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) Simplified Sliding Sync room list tests to cover both new and fallback logic paths.

Convert Sliding Sync tests to use higher-level compute_interested_rooms #18399

Convert Sliding Sync tests to use higher-level compute_interested_rooms #18399

Uh oh!

Conversation

devonh commented May 5, 2025 • edited by MadLittleMods Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev notes

Pull Request Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods May 6, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods May 6, 2025 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reivilibre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Convert Sliding Sync tests to use higher-level `compute_interested_rooms` #18399

Convert Sliding Sync tests to use higher-level `compute_interested_rooms` #18399

devonh commented May 5, 2025 •

edited by MadLittleMods

Loading

MadLittleMods May 6, 2025 •

edited

Loading

MadLittleMods May 6, 2025 •

edited

Loading