Use correct start and stop to get the message #1083
Conversation
|
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. |
tests/10apidoc/34room-messages.pl
Outdated
| @@ -151,7 +152,8 @@ sub matrix_send_room_text_message | |||
| # With no params this does "forwards from END"; i.e. nothing useful | |||
| params => { | |||
| dir => "b", | |||
| from => $token, | |||
| from => $next_batch, | |||
There was a problem hiding this comment.
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 aprev_batchtoken returned for each room by the sync API, or from astartorendtoken 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.
|
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.
|
No, this is a test of the I would expect:
No, I don't think so. If the test message is in the 1st sync, then the 2nd sync will be empty. The |
|
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.
|
Ugh, I see, yes. Perhaps you'll have to send another message to force an entry for that room in the second /sync result. |
The test depended on a bug in Synapse where prev_batch is set to an incorrect value in case /sync is called with since parameter. Signed-off-by: Kurt Roeckx <[email protected]>
|
I've changed things and updated the commit message. |
|
The test suite failure does not look related to this PR |
| # This first sends a message, then does a /sync without since parameter. | ||
| # That /sync will most likely return the whole timeline for the room | ||
| # so we can't use it's prev_batch in the /messages API. So we send | ||
| # a 2nd message, and then do a /sync with since set to the previous | ||
| # /sync's next_batch. We can then fetch messages with the 2nd /sync's | ||
| # prev_batch that are before first /sync. |
There was a problem hiding this comment.
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?
richvdh
added
the
X-Awaiting-Changes
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 [email protected]