Add tests for MSC3787 #367

turt2live · 2022-05-04T01:55:38Z

Disclaimer: I have no idea what I'm doing, but tried to reduce code duplication as much as possible.

See diff.

turt2live · 2022-05-04T02:42:31Z

I don't feel it's fair to ask a contributor to also write the gomatrixserverlib changes for something like this, so sending up for review as-is.

erikjohnston · 2022-05-10T14:52:34Z

For Thursday meeting: the synapse-core team needs to figure out how to resource this.

kegsay

LGTM - re gomatrixserverlib changes: it's acceptable to have just Synapse <-> Synapse and Client <-> Synapse tests for now, but it's incomplete without Complement <-> Synapse federation tests, which implies changes to gomatrixserverlib to add support for this room version.

I feel it is unfair to just leave those changes for inevitably the Dendrite team (who use gomatrixserverlib). The onus should be on the person implementing feature X to add support for the test framework to talk feature X IMO.

Sytest also has to do this, but the difference here is that Complement actually implements the Matrix federation protocol, unlike Sytest which just does the absolute bare minimum checks in https://github.com/matrix-org/sytest/tree/develop/lib/SyTest/Federation

erikjohnston · 2022-05-17T10:12:53Z

I think this just needs matrix-org/synapse#12623 and we can merge it?

squahtx · 2022-05-18T17:42:46Z

I reran CI and it's still unhappy because the big roomVersionMeta map in gomatrixserverlib needs updating.

2022-05-18 17:37:29,895 - synapse.federation.federation_server - 612 - WARNING - GET-6 - Room version org.matrix.msc3787 not in ['4', '9', 'org.matrix.msc3667', '2', '6', '7', '1', '3', '5', '8']
2022-05-18 17:37:29,895 - synapse.http.server - 167 - INFO - GET-6 - <SynapseRequest at 0x7f41d0a32730 method='GET' uri='/_matrix/federation/v1/make_join/%21QMVXEbNUbesHiVJWag:hs1/@charlie:host.docker.internal:39159?ver=4&ver=9&ver=org.matrix.msc3667&ver=2&ver=6&ver=7&ver=1&ver=3&ver=5&ver=8' clientproto='HTTP/1.1' site='8448'> SynapseError: 400 - Your homeserver does not support the features required to interact with this room

DMRobertson · 2022-05-19T18:14:38Z

I have no idea what I'm doing either.

In this CI run by Sean. There were three failures:

❌ TestCannotSendKnockViaSendKnockInMSC3787Room (8.63s)
      federation_room_join_test.go:356: Deploy times: 8.495259ms blueprints, 4.268363051s containers
      client.go:544: POST hs1/_matrix/client/r0/createRoom => 200 OK (242.262475ms)
      federation_room_join_test.go:370: MustJoinRoom: make_join failed: contents=[123 34 101 114 114 99 111 100 101 34 58 34 77 95 73 78 67 79 77 80 65 84 73 66 76 69 95 82 79 79 77 95 86 69 82 83 73 79 78 34 44 34 101 114 114 111 114 34 58 34 89 111 117 114 32 104 111 109 101 115 101 114 118 101 114 32 100 111 101 115 32 110 111 116 32 115 117 112 112 111 114 116 32 116 104 101 32 102 101 97 116 117 114 101 115 32 114 101 113 117 105 114 101 100 32 116 111 32 105 110 116 101 114 97 99 116 32 119 105 116 104 32 116 104 105 115 32 114 111 111 109 34 44 34 114 111 111 109 95 118 101 114 115 105 111 110 34 58 34 111 114 103 46 109 97 116 114 105 120 46 109 115 99 51 55 56 55 34 125] msg=Failed to GET JSON (hostname "hs1" path "/_matrix/federation/v1/make_join/!QMVXEbNUbesHiVJWag:hs1/@charlie:host.docker.internal:39159") code=400 wrapped=M_INCOMPATIBLE_ROOM_VERSION: Your homeserver does not support the features required to interact with this room

❌ TestKnockRoomsInPublicRoomsDirectoryInMSC3787Room (4.92s)
      knocking_test.go:379: Deploy times: 8.771865ms blueprints, 4.261340298s containers
      client.go:544: POST hs1/_matrix/client/r0/createRoom => 200 OK (268.73146ms)
      client.go:544: PUT hs1/_matrix/client/r0/rooms/!QDdSjnCQiLVsmtUips:hs1/state/m.room.join_rules/ => 200 OK (48.796638ms)
      knocking_test.go:397: SendEventSynced waiting for event ID $c0hKZ7VvtUewPdBpX4GOARpiqGoyXP5tmdcCY1pqxSc
      client.go:544: GET hs1/_matrix/client/r0/sync => 200 OK (55.937387ms)
      client.go:544: PUT hs1/_matrix/client/r0/directory/list/room/!QDdSjnCQiLVsmtUips:hs1 => 200 OK (15.593207ms)
      client.go:544: GET hs1/_matrix/client/r0/publicRooms => 200 OK (7.796654ms)
      knocking_test.go:467: Room was not present in public room directory response
  2022/05/18 17:37:21 ============================================

❌ TestKnockingInMSC3787Room (11.97s)
      knocking_test.go:39: Deploy times: 8.00577ms blueprints, 6.083955172s containers
      client.go:544: POST hs1/_matrix/client/r0/createRoom => 200 OK (318.012575ms)
      server.go:101: Server.UnexpectedRequestsAreErrors=false received unexpected request to server: GET /_matrix/federation/v1/query/profile - sending 404 which may cause the HS to backoff from Complement
  2022/05/18 17:37:16 complement: Unable to unmarshal incoming /invite request: gomatrixserverlib: unsupported room version 'org.matrix.msc3787'
      client.go:544: POST hs1/_matrix/client/r0/rooms/!OepnCEOfNUHyQTojDf:hs1/invite => 400 Bad Request (64.751684ms)
      knocking_test.go:76: CSAPI.MustDo POST http://localhost:49338/_matrix/client/r0/rooms/%21OepnCEOfNUHyQTojDf:hs1/invite returned HTTP 400 : {"errcode":"M_UNKNOWN","error":"Bad Request","message":"gomatrixserverlib: unsupported room version 'org.matrix.msc3787'"}

In the most recent CI run, two of those failures now pass. The middle one remains.

❌ TestKnockRoomsInPublicRoomsDirectoryInMSC3787Room (3.41s)
      knocking_test.go:379: Deploy times: 7.369706ms blueprints, 2.903263873s containers
      client.go:544: POST hs1/_matrix/client/r0/createRoom => 200 OK (180.2697ms)
      client.go:544: PUT hs1/_matrix/client/r0/rooms/!MoNVCTnYmGfxBaSjjk:hs1/state/m.room.join_rules/ => 200 OK (25.66037ms)
      knocking_test.go:397: SendEventSynced waiting for event ID $nF2cmnFVEYT5WIa54_b1MIPjtVK3mlA7yw65O-nKwXM
      client.go:544: GET hs1/_matrix/client/r0/sync => 200 OK (40.19948ms)
      client.go:544: PUT hs1/_matrix/client/r0/directory/list/room/!MoNVCTnYmGfxBaSjjk:hs1 => 200 OK (7.753812ms)
      client.go:544: GET hs1/_matrix/client/r0/publicRooms => 200 OK (3.987357ms)
      knocking_test.go:467: Room was not present in public room directory response
  2022/05/19 18:03:32 ============================================

squahtx · 2022-05-19T18:43:18Z

In the most recent CI run, two of those failures now pass. The middle one remains.

❌ TestKnockRoomsInPublicRoomsDirectoryInMSC3787Room (3.41s)
      knocking_test.go:379: Deploy times: 7.369706ms blueprints, 2.903263873s containers
      client.go:544: POST hs1/_matrix/client/r0/createRoom => 200 OK (180.2697ms)
      client.go:544: PUT hs1/_matrix/client/r0/rooms/!MoNVCTnYmGfxBaSjjk:hs1/state/m.room.join_rules/ => 200 OK (25.66037ms)
      knocking_test.go:397: SendEventSynced waiting for event ID $nF2cmnFVEYT5WIa54_b1MIPjtVK3mlA7yw65O-nKwXM
      client.go:544: GET hs1/_matrix/client/r0/sync => 200 OK (40.19948ms)
      client.go:544: PUT hs1/_matrix/client/r0/directory/list/room/!MoNVCTnYmGfxBaSjjk:hs1 => 200 OK (7.753812ms)
      client.go:544: GET hs1/_matrix/client/r0/publicRooms => 200 OK (3.987357ms)
      knocking_test.go:467: Room was not present in public room directory response
  2022/05/19 18:03:32 ============================================

Nice job getting it working! I think you've found a legitimate test failure. I found three places in Synapse where we consider the knock join rule, but not knock_restricted:

https://github.com/matrix-org/synapse/blob/942c30b16b86cb05d2109b13bc2c1dc9ac2fea70/synapse/handlers/room_summary.py#L664-L668
https://github.com/matrix-org/synapse/blob/320186319ac4f1d16f8f964d92db8921a4b1073e/synapse/storage/databases/main/room.py#L244-L253
https://github.com/matrix-org/synapse/blob/320186319ac4f1d16f8f964d92db8921a4b1073e/synapse/storage/databases/main/room.py#L381-L394

The next step would be to figure out what those three places should do and fix Synapse?

erikjohnston · 2022-05-23T13:12:22Z

I assume @DMRobertson is working on this and its not ready yet for review? If its please just shove it back on the queue

Fixes the build which was broken in distribution/distribution#3590

DMRobertson · 2022-05-24T16:34:03Z

This passes now. I don't know what the next steps are.

For cross-referencing:

the msc
the original synapse change
the follow-up synapse change
the proposed gomatrixserverlib change, which blocks this PR
- if this is accepted, I don't know if go.mod/sum ought to be updated to point to the main branch of gomatrixserverlib rather than my one-off branch

Some tests are enabled on dendrite now, it seems

neilalexander · 2022-05-27T08:50:03Z

if this is accepted, I don't know if go.mod/sum ought to be updated to point to the main branch of gomatrixserverlib rather than my one-off branch

Yes please — since we squash commits into GMSL, it's best to pin commit IDs from the main branch where possible.

I don't know what I'm doing: MSC3787 edition

0eacdbc

turt2live mentioned this pull request May 4, 2022

Add a new room version for MSC3787's knock+restricted join rule matrix-org/synapse#12623

Merged

4 tasks

turt2live added 2 commits May 3, 2022 20:01

Add MSC3787 to synapse tests

82743d3

Don't forget to import things. It helps a lot.

7350819

turt2live marked this pull request as ready for review May 4, 2022 02:42

turt2live requested a review from a team May 4, 2022 02:43

kegsay approved these changes May 11, 2022

View reviewed changes

Merge branch 'main' into travis/combinatorial-v2

2d47301

DMRobertson self-assigned this May 19, 2022

DMRobertson mentioned this pull request May 19, 2022

Support MSC3787 matrix-org/gomatrixserverlib#309

Closed

Try gomatrixserverlib with support(?) for this MSC

95a487b

erikjohnston removed the request for review from a team May 23, 2022 13:11

DMRobertson mentioned this pull request May 24, 2022

Try to support MSC3787 matrix-org/gomatrixserverlib#310

Merged

kegsay and others added 4 commits May 24, 2022 13:53

Upgrade indirect deps

1053c4a

Fixes the build which was broken in distribution/distribution#3590

Merge commit 'refs/pullreqs/376' into travis/combinatorial-v2

2e5a5cb

Re-fetch my branch of gomatrixserverlib

5fa8d28

Merge remote-tracking branch 'origin/main' into travis/combinatorial-v2

8ce6daa

DMRobertson marked this pull request as draft May 24, 2022 13:31

DMRobertson mentioned this pull request May 24, 2022

Fixes to MSC3787 implementation matrix-org/synapse#12858

Merged

DMRobertson requested review from a team May 24, 2022 16:35

DMRobertson marked this pull request as ready for review May 26, 2022 12:43

David Robertson added 2 commits May 26, 2022 13:55

Pull latest versopm of my gomatrixserverlib branch

1ccc436

Merge remote-tracking branch 'origin/main' into travis/combinatorial-v2

1d8e313

Some tests are enabled on dendrite now, it seems

erikjohnston approved these changes May 27, 2022

View reviewed changes

Use main branch of GMSL

955df00

DMRobertson changed the title ~~Add tests for MSC3787, I think~~ Add tests for MSC3787 May 27, 2022

DMRobertson merged commit 25adcae into main May 27, 2022

DMRobertson deleted the travis/combinatorial-v2 branch May 27, 2022 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for MSC3787 #367

Add tests for MSC3787 #367

turt2live commented May 4, 2022

turt2live commented May 4, 2022

erikjohnston commented May 10, 2022

kegsay left a comment

erikjohnston commented May 17, 2022

squahtx commented May 18, 2022 •

edited

Loading

DMRobertson commented May 19, 2022

squahtx commented May 19, 2022 •

edited

Loading

erikjohnston commented May 23, 2022

DMRobertson commented May 24, 2022 •

edited

Loading

neilalexander commented May 27, 2022

Add tests for MSC3787 #367

Add tests for MSC3787 #367

Conversation

turt2live commented May 4, 2022

turt2live commented May 4, 2022

erikjohnston commented May 10, 2022

kegsay left a comment

Choose a reason for hiding this comment

erikjohnston commented May 17, 2022

squahtx commented May 18, 2022 • edited Loading

DMRobertson commented May 19, 2022

squahtx commented May 19, 2022 • edited Loading

erikjohnston commented May 23, 2022

DMRobertson commented May 24, 2022 • edited Loading

neilalexander commented May 27, 2022

squahtx commented May 18, 2022 •

edited

Loading

squahtx commented May 19, 2022 •

edited

Loading

DMRobertson commented May 24, 2022 •

edited

Loading