Convert Sytest Name/topic keys are correct to Complement
#811
Conversation
This test has been migrated to Complement, see matrix-org/complement#811
…est_name_topic_keys_are_correct
As 15s would be quite long to wait until the test failed.
This allows us to collect all incorrect data about a room and print it. A side-effect is that one doesn't need to re-run the tests to see all the broken fields. In addition, the `must.MatchGJSON` was changed to a `should`, which now re-polls `/publicRooms`. This prevents the test from exiting early upon receiving an entry that's missing some keys.
tests/csapi/public_rooms_test.go
Outdated
| // Define room configurations matching the Sytest | ||
| roomConfigs := []struct { | ||
| alias string | ||
| name string | ||
| topic string | ||
| }{ | ||
| {"publicroomalias_no_name", "", ""}, | ||
| {"publicroomalias_with_name", "name_1", ""}, | ||
| {"publicroomalias_with_topic", "", "topic_1"}, | ||
| {"publicroomalias_with_name_topic", "name_2", "topic_2"}, | ||
| {"publicroom_with_unicode_chars_name", "un nom français", ""}, | ||
| {"publicroom_with_unicode_chars_topic", "", "un topic à la française"}, | ||
| {"publicroom_with_unicode_chars_name_topic", "un nom français", "un topic à la française"}, | ||
| } |
There was a problem hiding this comment.
A different way to tackle this would be to just split this up over multiple tests. Move the for-loop upwards and call t.Run(...) multiple times.
Splitting up the tests is probably better anyway.
There was a problem hiding this comment.
By
This is probably better anyway.
Do you mean that you prefer the current changes, or would prefer splitting up into multiple tests?
I realise that we should be careful parallelising all of those, as we'd then see "unexpected rooms" appear in the room list.
There was a problem hiding this comment.
Things would be much simpler if we split and had a test for each of these room config variations.
There was a problem hiding this comment.
After doing this, the test is much nicer and has much less book-keeping. Great suggestion!
A more accurate name for it.
This is indeed much easier to follow, and less book-keeping.
Otherwise we were (correctly) seeing the warning about unexpected rooms in every time other than the first.
| must.MatchGJSON( | ||
| t, | ||
| body, | ||
| match.JSONKeyPresent("chunk"), | ||
| match.JSONKeyTypeEqual("chunk", gjson.JSON), | ||
| ) |
There was a problem hiding this comment.
I don't think we need this duplication with the checks below which have better assertion messages anyway
| }) | ||
| } | ||
|
|
||
| func parsePublicRoomsResponse(t *testing.T, res *http.Response) []gjson.Result { |
There was a problem hiding this comment.
| func parsePublicRoomsResponse(t *testing.T, res *http.Response) []gjson.Result { | |
| func parsePublicRoomsResponse(t *testing.T, res *http.Response) []gjson.Result { | |
| t.Helper() |
| t.Run("Name/topic keys are correct", func(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
I don't think either of these tests are resilient to being run in parallel.
The first test assumes there will be one and only room in the results.
This test keeps track of unexpectedRooms. These other rooms are not unexpected if the tests are running in parallel. They are not the expected room though.
Could be resolved a few different ways like making a new deployment for each test.
| func TestPublicRooms(t *testing.T) { | ||
| deployment := complement.Deploy(t, 1) | ||
| defer deployment.Destroy(t) | ||
| hostname := deployment.GetFullyQualifiedHomeserverName(t, "hs1") |
There was a problem hiding this comment.
This isn't just a hostname. It's the "server name" of the homeserver.
(could use a better variable name)
| }), | ||
| ) | ||
|
|
||
| t.Logf("Warning: Found unexpected rooms in public rooms list: %v", unexpectedRooms) |
There was a problem hiding this comment.
We should only log this if len(unexpectedRooms) > 0
| "generic_search_term": "wombles", | ||
| }, | ||
| }), | ||
| client.WithRetryUntil(15*time.Second, func(res *http.Response) bool { |
There was a problem hiding this comment.
| client.WithRetryUntil(15*time.Second, func(res *http.Response) bool { | |
| client.WithRetryUntil(authedClient.SyncUntilTimeout, func(res *http.Response) bool { |
| t.Errorf("Validation error for room %s: %s", roomID, e.Error()) | ||
| } | ||
|
|
||
| t.Fail() |
There was a problem hiding this comment.
Using t.Fatalf(...) with some message looks better in the output.
Current t.Fail():
=== RUN TestPublicRooms
2025/10/30 11:42:46 Sharing [SERVER_NAME=hs1 SYNAPSE_LOG_TESTING=1 SYNAPSE_COMPLEMENT_USE_WORKERS= SYNAPSE_COMPLEMENT_DATABASE=sqlite] host environment variables with container
=== RUN TestPublicRooms/parallel
=== RUN TestPublicRooms/parallel/Name/topic_keys_are_correct
=== PAUSE TestPublicRooms/parallel/Name/topic_keys_are_correct
=== CONT TestPublicRooms/parallel/Name/topic_keys_are_correct
client.go:784: [CSAPI] POST hs1/_matrix/client/v3/register => 200 OK (117.527638ms)
=== RUN TestPublicRooms/parallel/Name/topic_keys_are_correct/Creating_room_with_alias_publicroomalias_no_name
=== NAME TestPublicRooms/parallel/Name/topic_keys_are_correct
client.go:784: [CSAPI] POST hs1/_matrix/client/v3/createRoom => 200 OK (411.951625ms)
=== NAME TestPublicRooms/parallel/Name/topic_keys_are_correct/Creating_room_with_alias_publicroomalias_no_name
public_rooms_test.go:103: Created room !RqNTiptKqCqmtWHWXC:hs1 with alias publicroomalias_no_name
=== NAME TestPublicRooms/parallel/Name/topic_keys_are_correct
client.go:784: [CSAPI] GET hs1/_matrix/client/v3/publicRooms => 200 OK (19.299404ms)
=== NAME TestPublicRooms/parallel/Name/topic_keys_are_correct/Creating_room_with_alias_publicroomalias_no_name
public_rooms_test.go:137: Warning: Found unexpected rooms in public rooms list: []
public_rooms_test.go:200: Validation error for room !RqNTiptKqCqmtWHWXC:hs1: Room !RqNTiptKqCqmtWHWXC:hs1 has unexpected name '', expected no name
=== NAME TestPublicRooms/parallel/Name/topic_keys_are_correct
client.go:784: [CSAPI] PUT hs1/_matrix/client/v3/directory/list/room/!RqNTiptKqCqmtWHWXC:hs1 => 200 OK (21.216743ms)
2025/10/30 11:42:54 ============================================
Using t.Fatalf(...) instead:
=== RUN TestPublicRooms
2025/10/30 11:41:17 Sharing [SERVER_NAME=hs1 SYNAPSE_LOG_TESTING=1 SYNAPSE_COMPLEMENT_USE_WORKERS= SYNAPSE_COMPLEMENT_DATABASE=sqlite] host environment variables with container
=== RUN TestPublicRooms/parallel
=== RUN TestPublicRooms/parallel/Name/topic_keys_are_correct
=== PAUSE TestPublicRooms/parallel/Name/topic_keys_are_correct
=== CONT TestPublicRooms/parallel/Name/topic_keys_are_correct
client.go:784: [CSAPI] POST hs1/_matrix/client/v3/register => 200 OK (121.73885ms)
=== RUN TestPublicRooms/parallel/Name/topic_keys_are_correct/Creating_room_with_alias_publicroomalias_no_name
=== NAME TestPublicRooms/parallel/Name/topic_keys_are_correct
client.go:784: [CSAPI] POST hs1/_matrix/client/v3/createRoom => 200 OK (436.070145ms)
=== NAME TestPublicRooms/parallel/Name/topic_keys_are_correct/Creating_room_with_alias_publicroomalias_no_name
public_rooms_test.go:103: Created room !lKZeexiHPJLOnCUMOl:hs1 with alias publicroomalias_no_name
=== NAME TestPublicRooms/parallel/Name/topic_keys_are_correct
client.go:784: [CSAPI] GET hs1/_matrix/client/v3/publicRooms => 200 OK (20.858944ms)
=== NAME TestPublicRooms/parallel/Name/topic_keys_are_correct/Creating_room_with_alias_publicroomalias_no_name
public_rooms_test.go:137: Warning: Found unexpected rooms in public rooms list: []
public_rooms_test.go:200: Validation error for room !lKZeexiHPJLOnCUMOl:hs1: Room !lKZeexiHPJLOnCUMOl:hs1 has unexpected name '', expected no name
public_rooms_test.go:204: <your fatal message here>
2025/10/30 11:41:25 ============================================
This PR converts the "Name/topic keys are correct" sytest to Complement.
This was spawned from said test failing in element-hq/synapse#19071, and not wanting to debug perl.
The test was first converted to go via Claude Code, then verified manually. Finally, the test failed when modifying what Synapse returns for
/publicRooms, which is what we want to see.