Sytest: Add 30rooms/60version_upgrade tests
#554
Conversation
ignore the rest due to non-formalisation also alter MustSyncUntil to number checks in error output
|
CI is failing because of #550, will merge/rebase after that is merged |
CI seems to still be failing @ShadowJonathan , is that expected? |
|
@clokep those are unrelated flakes, could you kick off a rerun? |
|
I did another rerun and this is green. |
| _, tombstoneSinceToken := alice.MustSync(t, client.SyncReq{TimeoutMillis: "0"}) | ||
| _, sinceToken := alice.MustSync(t, client.SyncReq{TimeoutMillis: "0"}) |
There was a problem hiding this comment.
A bit odd to sync twice rather than copy the token into a new var
There was a problem hiding this comment.
Might be clearer if this was a MustSyncUntil which waits for Alice to see her membership of the new room?
There was a problem hiding this comment.
Using the same since-token creates - for my feeling - Undefined Behaviour, and these exist to detect two different things in disjoint "timelines";
- One is looking for the tombstone event, which can happen before or after creating the new room.
- One is looking for the new room, which can happen before or after the tombstone event is emitted.
This might look like a clear case of using the same since-token twice, but I've encountered subtle weird behaviour in the past with using the same sync token twice, and as this kind of behaviour is not explicitly defined in the spec (it only talks about and assumes that one uses a since token exactly once), i'm being conservative here, creating two different since tokens to use for different "spans of time" to test.
There was a problem hiding this comment.
Okay. Can you please spell this out with a comment: something to the effect of "the tombstone and new room can appear down /sync in either order, so we check for these using separate /sync tracks". Can we also have a more descriptive name for the second sinceToken which describes what it is being used to wait for, please.
| }, | ||
| }) | ||
|
|
||
| _, sinceToken := alice.MustSync(t, client.SyncReq{TimeoutMillis: "0"}) |
There was a problem hiding this comment.
What are syncing for here? Can we make this a MustSyncUntil and make the thing we want to see?
There was a problem hiding this comment.
We are syncing explicitly over the span of time of the room upgrade, I find this cleaner to test for.
There was a problem hiding this comment.
We are syncing explicitly over the span of time of the room upgrade
What guarantees that this /sync response includes Alice's join to the room and the canonical alias event she sent?
tests/csapi/room_upgrade_test.go
Outdated
| "POST", | ||
| []string{"_matrix", "client", "v3", "rooms", roomID, "upgrade"}, | ||
| client.WithJSONBody(t, map[string]string{ | ||
| "new_version": "6", |
There was a problem hiding this comment.
Any particular reason for 6? Sytest used to have 2 AFAICS
There was a problem hiding this comment.
No particular reason, but I think that we should really start to use a more modern room number than 2.
| }) | ||
|
|
||
| user.JoinRoom(t, roomID, []string{"hs1"}) | ||
| alice.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(user.UserID, roomID)) |
There was a problem hiding this comment.
Why is alice syncing? Presumably this should be user.MustSyncUntil?
There was a problem hiding this comment.
If user asserts that it has joined the room, the join might not have propagated to alice's homeserver, so when alice asserts it, it must've registered on both user and alice's homeserver, which means its safe to assume this going forward, whereas its not when only user sees it.
It might look small, but it could result in a flake if alice's homeserver is just slow enough.
There was a problem hiding this comment.
when alice asserts it, it must've registered on both user and alice's homeserver,
while this is true, it's not obvious to the reader at first glance.
Can we either
- have a comment along the lines of your own above, or
- change (2) to have
userwait for the join, and then change alice's sync on +43 to wait foruser's join before upgrading the room.
| }), | ||
| ) | ||
|
|
||
| _, sinceToken := alice.MustSync(t, client.SyncReq{TimeoutMillis: "0"}) |
There was a problem hiding this comment.
Why is alice syncing here? Please make the condition explicit
There was a problem hiding this comment.
This is because alice upgrades the room, and will be the first to see the new events and its results. IIRC this is also what the sytest did, incrementally syncing from a room's upgrader to observe its results there.
If it were user, they would have do a sync in the meantime to assert the room has upgraded, join the room, and then do another sync (which might miss the room creation event due to gaps), and then have that be observed. Its cleaner to have the upgrader do this with one sync, where its assumed they join the room they're upgrading, and that they also get all the room creation events nicely frontloaded.
There was a problem hiding this comment.
My objection isn't that the user that's syncing is Alice; it's that I don't know what we are waiting for Alice to see down /sync before upgrading the room. That's why I suggest splitting out user and alice's syncs in https://github.com/matrix-org/complement/pull/554/files#r1047211314 --- it makes it easier to see what each client is waiting to see before the scenario progresses.
ShadowJonathan
requested review from
DMRobertson
and removed request for
kegsay
| /upgrade restricts power levels in the old room | ||
| /upgrade restricts power levels in the old room when the old PLs are unusual | ||
| /upgrade should preserve room visibility for $vis rooms | ||
|
|
||
| # Tests deprecated endpoints | ||
| Tags appear in the v1 /initialSync | ||
| Tags appear in the v1 /events stream |
There was a problem hiding this comment.
@ShadowJonathan What's the status with this other unresolved review feedback?
|
Closing as stale. |
This adds 6 sytests:
./tests/30rooms/60version_upgrade.pl: test "$user_type user has push rules copied to upgraded room",./tests/30rooms/60version_upgrade.pl:test "/upgrade creates a new room",./tests/30rooms/60version_upgrade.pl:test "/upgrade moves aliases to the new room",./tests/30rooms/60version_upgrade.pl:test "/upgrade is rejected if the user can't send state events",./tests/30rooms/60version_upgrade.pl:test "/upgrade of a bogus room fails gracefully",./tests/30rooms/60version_upgrade.pl:test "/upgrade to an unknown version is rejected",However, this also ignores the remaining 13 sytests, due to non-formalisation of what they're testing for in the spec, see matrix-org/matrix-spec#1324 and matrix-org/matrix-spec#456 for more details on that.
This also adds some extra logging in
MustSyncUntilto make it clear which checker exactly it is printing errors for, this is useful when the errors inside the checker itself are ambiguous or not helpful with identifying the checker.