Sliding Sync: Move filters tests to rest layer#17703
Conversation
| # Remove rooms if we can't figure out what room type it is | ||
| if not sync_room_map[room_id].has_known_state: | ||
| filtered_room_id_set.remove(room_id) | ||
| continue |
There was a problem hiding this comment.
Fixing filter_rooms_using_tables (the new path using the new sliding sync tables) to take into account has_known_state.
The _no_stripped_state tests exercise these scenarios.
Found this bug because we're actually testing the new path now with this PR.
| response_body["rooms"][room_id]["bump_stamp"], invite_pos.stream | ||
| ) | ||
|
|
||
| def test_rooms_meta_is_dm(self) -> None: |
There was a problem hiding this comment.
This test is "new" because I moved the "Ensure DM's are correctly marked" checking from one of the filtering tests to it's own separate thing here.
| ) | ||
|
|
||
| def test_filters_is_dm(self) -> None: | ||
| """ |
There was a problem hiding this comment.
These tests existed before as unit tests against filter_rooms(...) directly but now that we also have filter_rooms_using_tables(...) they've been migrated to integration tests at the REST layer operating against the Sliding Sync API itself. This way we can test both code paths with the same tests.
They are just refactored to be at this new layer and interact with the API.
| # There should be no room sent down. | ||
| self.assertFalse(channel.json_body["rooms"]) | ||
|
|
||
| def test_filter_list(self) -> None: |
There was a problem hiding this comment.
These filter tests already at the REST layer were just moved to tests/rest/client/sliding_sync/test_lists_filters.py now that we have a specific file for it.
| message=str(actual_required_state), | ||
| ) | ||
|
|
||
| def _add_new_dm_to_global_account_data( |
There was a problem hiding this comment.
Just moving some helpers to the SlidingSyncBase so we can use them across the Sliding Sync tests more easily.
| self.assertTrue(room_id1 in newly_left) | ||
|
|
||
|
|
||
| class FilterRoomsTestCase(HomeserverTestCase): |
There was a problem hiding this comment.
These tests we're moved to tests/rest/client/sliding_sync/test_lists_filters.py and refactored to interact with the Sliding Sync API directly.
devonh
left a comment
devonh
left a comment
There was a problem hiding this comment.
Overall looks good to me!
Just a few comments.
| } | ||
| response_body, _ = self.do_sync(sync_body, tok=user1_tok) | ||
| self.assertIncludes( | ||
| set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), |
There was a problem hiding this comment.
Not an issue here, but the ops field in sync responses is completely omitted from the MSC. ie. https://github.com/matrix-org/matrix-spec-proposals/blob/erikj/sss/proposals/4186-simplified-sliding-sync.md#response-format
There was a problem hiding this comment.
We're thinking about removing it in favor of just a straight list like room_ids: [] but until then I'll use it (context #17671 (comment))
MadLittleMods
deleted the
madlittlemods/sliding-sync-move-filters-tests
branch
|
Thanks for the review @devonh 🦌 |
2 participants
Move filters tests to rest layer in order to test the new (with sliding sync tables) and fallback paths that Sliding Sync can use.
Also found a bug in the new path because it's not being tested which is also fixed in this PR. We now take into account
has_known_statewhen filtering.Spawning from #17662 (comment). This should have been done when we started using the new sliding sync tables in #17630
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.(run the linters)