chore: test more room versions in tests, from "9" to "12"#129
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
=======================================
Coverage 95.61% 95.61%
=======================================
Files 7 7
Lines 798 798
Branches 118 118
=======================================
Hits 763 763
Misses 22 22
Partials 13 13 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Just over 30 minutes to complete each test round. That is stretching what I consider acceptable into the "no" category. There is a possibility of refactor into splitting our test runs for each python version into two: One that uses SQLite and one that uses Postgres(see the hatch matrix used in pyproject.toml). This will have to be a separate work though |
88464c5 to
4e54e02
Compare
There was a problem hiding this comment.
Pull request overview
This PR expands the test suite to validate behavior across multiple Matrix room versions (including newer versions) by parameterizing many existing tests and updating shared test helpers to support configurable default/allowed room versions.
Changes:
- Parameterize multiple test case classes to run against room versions 9–12.
- Refactor test helper APIs (
create_local_room, room-version config) and update call sites accordingly. - Expand room-version-specific tests to cover upgrade/downgrade limits and invalid version inputs.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/base.py | Introduces DEFAULT_ROOM_VERSION / ALLOWED_ROOM_VERSIONS, updates join helpers, refactors create_local_room, and adds time-advancing logic during room creation. |
| tests/test_room_versions.py | Reworks room-version tests to be class-parameterized across different default/allowed/failure sets. |
| tests/test_scheduled_tasks.py | Parameterizes scheduled-task tests across room versions; updates create_local_room call signature. |
| tests/test_local_joins.py | Parameterizes local join tests across room versions; updates create_local_room call signature. |
| tests/test_local_invites.py | Parameterizes local invite tests across room versions; updates create_local_room call signature. |
| tests/test_createrooms_local.py | Parameterizes local create-room tests across room versions; updates invitee handling via invitee_list. |
| tests/test_createrooms_remote.py | Parameterizes remote create-room tests across room versions; updates invitee handling via invitee_list; fixes a typo in a comment. |
| tests/test_remote_joins.py | Updates create_local_room call signature. |
| tests/test_redaction.py | Parameterizes redaction tests across room versions; updates create_local_room call signature. |
| tests/test_reactions.py | Updates create_local_room call signature. |
| tests/test_room_message_timestamp.py | Updates create_local_room call signature. |
| tests/test_disable_epa_communication.py | Updates create_local_room call signature. |
Comments suppressed due to low confidence (2)
tests/base.py:366
send_joinacceptsroom_version_str, but it isn’t passed through to_make_join, so the?ver=query is always built fromDEFAULT_ROOM_VERSION. This can lead to mismatches between the room version used for the make_join template and the version used for signing/hashing below.
def send_join(
self,
joining_user: str,
room_id: str,
make_join_expected_code: int = HTTPStatus.OK,
send_join_expected_code: int = HTTPStatus.OK,
room_version_str: str | None = None,
) -> None:
"""
Join a remote user to a local server. Should be a complete make_join/send_join
handshake
"""
# First create the make_join that will need to be signed by the 'remote server'
join_result_channel = self._make_join(joining_user, room_id)
assert (
tests/base.py:704
- Catching and swallowing
AssertionErrorfromcreate_room_aswill hide unexpected HTTP statuses (including regressions and 5xx errors) and can let negative tests pass even when the server behavior is wrong. Instead, let the assertion propagate (or re-raise with more context) so that unexpected response codes fail the test run.
# Hide the assertion from create_room_as() when the error code is unexpected. It
# makes errors for the tests less clear when all we get is the http response,
# because then we are not sure which exact test used is the failure(especially
# when creating many rooms). Instead, use a simple binary condition; either we
# get a room_id or None. This allows the test itself to let us know which test
# failed.
try:
return self.helper.create_room_as(
creating_user,
is_public=is_public,
room_version=room_version or self.DEFAULT_ROOM_VERSION,
tok=self.map_user_id_to_token[creating_user],
extra_content=extra_content,
expect_code=expected_code,
)
except AssertionError as e:
logger.warning(
"create_room_as failed to create room, this may have been expected: %r",
e,
)
return None
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
88c882a to
2281f7d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/base.py:702
- The broad
except AssertionErroraroundhelper.create_room_as(...)will also swallow the assertion that checks the HTTP status code. That means calls likecreate_local_room(..., expected_code=HTTPStatus.BAD_REQUEST)can incorrectly returnNoneeven if the server unexpectedly returns 200 (or any other code), causing negative tests to pass when they should fail. Consider only swallowing assertions whenexpected_codeisHTTPStatus.OK, and re-raise (or otherwise assert) whenexpected_codeis non-OK so mismatched status codes don't get masked.
# Hide the assertion from create_room_as() when the error code is unexpected. It
# makes errors for the tests less clear when all we get is the http response,
# because then we are not sure which exact test used is the failure(especially
# when creating many rooms). Instead, use a simple binary condition; either we
# get a room_id or None. This allows the test itself to let us know which test
# failed.
try:
return self.helper.create_room_as(
creating_user,
is_public=is_public,
room_version=room_version or self.DEFAULT_ROOM_VERSION,
tok=self.map_user_id_to_token[creating_user],
extra_content=extra_content,
expect_code=expected_code,
)
except AssertionError as e:
logger.warning(
"create_room_as failed to create room, this may have been expected: %r",
e,
)
return None
FrenchGithubUser
left a comment
There was a problem hiding this comment.
nice, hopefully this will catch some issues in the future!
e6bdc47 to
a00b119
Compare
SYN-31
Refactor a bunch of tests to be parameterized and check previous and newer room versions. This does have the "less-than-nice" effect of multiplying the number of tests ran
Because the
FakeRoomdoes not support room version "11" and "12" yet, this does not touch:OutgoingEPARemoteJoinTestCaseOutgoingPRORemoteJoinTestCaseInactiveRoomScanTaskV1_1TestCaseStateOnlyPurgeRoomScanTaskV1_2TestCaseThese an be handled in a follow-up work, as that is more invasive and this was large enough to look at.
May be best reviewed commit-by-commit