Use correct start and stop to get the message

[kroeckx]

The output of /sync without since parameter has the most recent
message events, and so should include at least the message we
just sent.

The tests used prev_batch, but this should point before the message
that was returned in /sync. Starting backward from an event before
the test message should not have received any message.

We now test between next_batch and prev_batch, we should get the
message.

Signed-off-by: Kurt Roeckx kurt@roeckx.be

[kroeckx]

I believe that Synapse is not following the specification and so the test failure is expected. I'm unsure why Dendrite passes with both the old and new test, I suspect there is a different bug in Dendrite.

[richvdh]

the spec could be clearer about this, but it's not intended that you can pass a next_batch token in from. Per https://matrix.org/docs/spec/client_server/r0.6.1#get-matrix-client-r0-rooms-roomid-messages:

from: ... this token can be obtained from a prev_batch token returned for each room by the sync API, or from a start or end token returned by a previous request to this endpoint.

I think that to do what this test wants, we need to do a second sync, and then paginate backwards from the prev_batch of the second sync. Which, incidentally, will also work around the synapse bug.

[kroeckx]

I think what you're saying is that Sytest should act more like a client should act. So what I would expect is: /sync /send/m.room.message /sync&since=next_batch And get the message from the 2nd /sync call. But maybe the intention of this test is to use the /messages API, and see if that works. From your comment, I understand that if we add a since to the 2nd sync call, calling the messages API with a from set to the 2nd sync's next_batch should work around Synapse's bug, but would also not really follow the spec. My understanding is that a client will use that API in case the /sync didn't return all the messages. So I think to properly test that, we need to to send enough messages that they don't fit in a /sync. I'm not sure if there is a test like that. I'm currently also not sure what other tests expect this test to test. I think your comment about using the prev_batch from the 2nd sync is just the same as the current test, it would still ask for messages before the test message.

[richvdh]

I think what you're saying is that Sytest should act more like a client should act. So what I would expect is: /sync /send/m.room.message /sync&since=next_batch And get the message from the 2nd /sync call.

No, this is a test of the /message API.

I would expect:

• /send
• /sync <- results include test message
• /sync?since=next_batch <- empty result
• /messages?from=prev_batch <- includes test message

I think your comment about using the prev_batch from the 2nd sync
is just the same as the current test, it would still ask for
messages before the test message.

No, I don't think so. If the test message is in the 1st sync, then the 2nd sync will be empty. The prev_batch from the 2nd sync will be after the test message.

[kroeckx]

I would expect: * `/send` * `/sync` <- results include test message * `/sync?since=next_batch` <- empty result * `/messages?from=prev_batch` <- includes test message

That 2nd sync doesn't return a prev_batch since it's an empty result. It just contains a next_batch.

[richvdh]

That 2nd sync doesn't return a prev_batch since it's an empty result. It just contains a next_batch.

Ugh, I see, yes. Perhaps you'll have to send another message to force an entry for that room in the second /sync result.

[kroeckx]

I've changed things and updated the commit message.

[kroeckx]

The test suite failure does not look related to this PR

[richvdh]

the change looks generally fine now. Please could you update the summary and description in the PR?

[richvdh]

normally we put this sort of comment inside the body of the test - see https://github.com/kroeckx/sytest/blob/room_message/tests/50federation/33room-get-missing-events.pl#L287-L303 for example. Please could you move it?