Stop testing unspecced POST /send by tulir · Pull Request #850 · matrix-org/complement

tulir · 2026-03-13T22:32:24Z

For element-hq/synapse#19557

Signed-off-by: Tulir Asokan <tulir@maunium.net>

tulir · 2026-03-14T08:32:13Z

Not sure what that test failure is 🤔

tests/csapi/invalid_test.go

anoadragon453 · 2026-03-17T18:30:54Z

Not sure what that test failure is 🤔

panic: BUG: Empty package name encountered. will occur when gotestfmt receives an error and isn't able to parse the output. Try temporarily removing | .ci/scripts/gotestfmt from the following line in CI:

complement/.github/workflows/ci.yaml

Line 121 in 12210e3

    
                     ${{ matrix.env }} go test -v -json -tags "${{ matrix.tags }}" -timeout "${{ matrix.timeout }}" ./tests ./tests/csapi ${{ matrix.packages }} | .ci/scripts/gotestfmt

(I fixed this in a repo somewhere to prevent this obtuse error from shadowing the real one, but currently failing to find it or the solution...)

This reverts commit ce4bdcd.

MadLittleMods · 2026-03-17T19:24:43Z

.github/workflows/ci.yaml

      - run: |
          set -o pipefail &&
-          ${{ matrix.env }} go test -v -json -tags "${{ matrix.tags }}" -timeout "${{ matrix.timeout }}" ./tests ./tests/csapi ${{ matrix.packages }} | .ci/scripts/gotestfmt
+          ${{ matrix.env }} go test -v -json -tags "${{ matrix.tags }}" -timeout "${{ matrix.timeout }}" ./tests ./tests/csapi ${{ matrix.packages }} # | .ci/scripts/gotestfmt


panic: BUG: Empty package name encountered. will occur when gotestfmt receives an error and isn't able to parse the output. Try temporarily removing | .ci/scripts/gotestfmt from the following line in CI:

-- @anoadragon453, #850 (comment)

We did this in Synapse for example, element-hq/synapse#19326

tulir · 2026-03-17T20:29:38Z

It was just a compile error, tests should pass now

MadLittleMods · 2026-03-17T20:46:57Z

tests/csapi/invalid_test.go

-			for _, testCase := range testCases {
-				res := alice.Do(t, "POST", []string{"_matrix", "client", "v3", "rooms", roomID, "send", "complement.dummy"}, client.WithJSONBody(t, testCase))
+			for i, testCase := range testCases {
+				res := alice.Do(t, "PUT", []string{"_matrix", "client", "v3", "rooms", roomID, "send", "complement.dummy", fmt.Sprintf("invalidnum-%d", i)}, client.WithJSONBody(t, testCase))


Spec for reference: PUT /_matrix/client/v3/rooms/{roomId}/send/{eventType}/{txnId}

MadLittleMods · 2026-03-17T20:50:18Z

tests/csapi/invalid_test.go

-			for _, testCase := range testCases {
-				res := alice.Do(t, "POST", []string{"_matrix", "client", "v3", "rooms", roomID, "send", "complement.dummy"}, client.WithJSONBody(t, testCase))
+			for i, testCase := range testCases {
+				res := alice.Do(t, "PUT", []string{"_matrix", "client", "v3", "rooms", roomID, "send", "complement.dummy", fmt.Sprintf("invalidnum-%d", i)}, client.WithJSONBody(t, testCase))


The "invalidnum" makes me think this a particular detail that matters. But it's actually the invalid number testCases above that matter.

Can we just use txn-xxx instead? I'm guessing we're just trying to namespace them to avoid tests colliding with each other. Perhaps we can just do this like we have in other tests:

complement/tests/room_timestamp_to_event_test.go

Lines 256 to 264 in 12210e3

var txnCounter int64 = 0

func getTxnID(prefix string) (txnID string) {

txnId := fmt.Sprintf("%s-%d", prefix, atomic.LoadInt64(&txnCounter))

atomic.AddInt64(&txnCounter, 1)

return txnId

}

Extracted all the transaction ids (this one, the comment below and the old getTxnID) into a new method in the helpers package

tests/csapi/room_messages_test.go

MadLittleMods · 2026-03-17T20:52:01Z

tests/csapi/room_messages_test.go

 )

-// sytest: POST /rooms/:room_id/send/:event_type sends a message
+// sytest: PUT /rooms/:room_id/send/:event_type/:txn_id sends a message


The xxx part of sytest: xxx part might matter and need to match the name in Sytest?

yeah sytest has that test: https://github.com/matrix-org/sytest/blob/1be04cce46c0f84abac736390dc3fab17f35a756/tests/10apidoc/34room-messages.pl#L23

Given this test is being removed in matrix-org/sytest#1419, I think we can just remove this comment entirely

Oh right, there's already another test below for the transaction ID version

MadLittleMods

Looks good, just a few clean-up items

MadLittleMods · 2026-03-18T14:57:53Z

helpers/txnid.go

+var txnCounter atomic.Int64
+var start = time.Now().Unix()
+
+func GetTxnID(prefix string) (txnID string) {


We should add a doc comment for what this does and is useful for

The txn send endpoint is tested below, so TestSendAndFetchMessage is only relevant for testing /messages

Stop testing unspecced POST /send

172d3f1

Signed-off-by: Tulir Asokan <tulir@maunium.net>

tulir requested review from a team as code owners March 13, 2026 22:32

tulir mentioned this pull request Mar 13, 2026

Remove undocumented transaction ID variants of various room endpoints element-hq/synapse#19557

Open

3 tasks

tulir commented Mar 14, 2026

View reviewed changes

tests/csapi/invalid_test.go Outdated Show resolved Hide resolved

Update tests/csapi/invalid_test.go

c67b4b2

tulir added 3 commits March 17, 2026 20:41

Disable gotestfmt

ce4bdcd

Fix format int call

d3f58e7

Revert "Disable gotestfmt"

000e60f

This reverts commit ce4bdcd.

MadLittleMods reviewed Mar 17, 2026

View reviewed changes

Add helper for generating unique transaction IDs

02f111c

tulir requested a review from MadLittleMods March 17, 2026 22:15

MadLittleMods reviewed Mar 18, 2026

View reviewed changes

tulir added 2 commits March 18, 2026 17:33

Add doc for GetTxnID

5de6121

Remove sytest comment

8ee78ab

The txn send endpoint is tested below, so TestSendAndFetchMessage is only relevant for testing /messages

	var txnCounter int64 = 0

	func getTxnID(prefix string) (txnID string) {
	txnId := fmt.Sprintf("%s-%d", prefix, atomic.LoadInt64(&txnCounter))

	atomic.AddInt64(&txnCounter, 1)

	return txnId
	}

Conversation

tulir commented Mar 13, 2026

Uh oh!

tulir commented Mar 14, 2026

Uh oh!

Uh oh!

anoadragon453 commented Mar 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tulir commented Mar 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants