Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use correct start and stop to get the message #1083

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

kroeckx
Copy link
Contributor

@kroeckx kroeckx commented Jul 17, 2021

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]

@kroeckx
Copy link
Contributor Author

kroeckx commented Jul 17, 2021

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.

@@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

kroeckx commented Aug 5, 2021 via email

@richvdh
Copy link
Member

richvdh commented Aug 6, 2021

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
Copy link
Contributor Author

kroeckx commented Aug 6, 2021 via email

@richvdh
Copy link
Member

richvdh commented Aug 9, 2021

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]>
@kroeckx
Copy link
Contributor Author

kroeckx commented Aug 10, 2021

I've changed things and updated the commit message.

@kroeckx
Copy link
Contributor Author

kroeckx commented Aug 10, 2021

The test suite failure does not look related to this PR

tests/10apidoc/34room-messages.pl Outdated Show resolved Hide resolved
tests/10apidoc/34room-messages.pl Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +131 to +136
# 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants