-
Notifications
You must be signed in to change notification settings - Fork 52
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
More test coverage to ensure we process MSC2716 events in existing room versions #209
More test coverage to ensure we process MSC2716 events in existing room versions #209
Conversation
…om versions Synapse changes in matrix-org/synapse#10962
…isting-room-version-test-msc2716 Conflicts: tests/msc2716_test.go
@@ -782,6 +782,9 @@ func TestImportHistoricalMessages(t *testing.T) { | |||
eventIdBefore := eventIDsBefore[0] | |||
timeAfterEventBefore := time.Now() | |||
|
|||
// Create eventIDsAfter to avoid the "No forward extremities left!" 500 error from Synapse | |||
createMessagesInRoom(t, alice, roomID, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poor comment wording. You reference eventIDsAfter
like it's a variable but it isn't used here?
Do you really mean:
Create events after to avoid the "No forward extremities left!" 500 error from Synapse
?
What does this have to do with room versions anyway? This just seems to be a logic error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I mean, but the goal is to match the eventIDsAfter
we use in other tests in this same place. The only reason I can't leave it there is because Go complains about it being unused.
createMessageEventsForBatchSendRequest([]string{virtualUserID}, timeAfterEventBefore, 1), | ||
// Status | ||
200, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this have to do with room versions anyway? This just seems to be a logic error?
It was a logic error only with existing room versions in Synapse.
Thanks for the review @kegsay 🦓 |
More test coverage to ensure we process MSC2716 events in existing room versions
Part of MSC2716: matrix-org/matrix-spec-proposals#2716
Synapse changes in matrix-org/synapse#10962