4311 stripped state create event#871
Draft
FrenchGithubUser wants to merge 1 commit intomatrix-org:mainfrom
Draft
4311 stripped state create event#871FrenchGithubUser wants to merge 1 commit intomatrix-org:mainfrom
FrenchGithubUser wants to merge 1 commit intomatrix-org:mainfrom
Conversation
3 tasks
but extra fields shouldn't be there
FrenchGithubUser
force-pushed
the
4311-stripped-state-create-event
branch
from
787b2fc to
3653b4b
Compare
Comment on lines
+1360
to
+1361
| // find the create event: MSC4311 requires it to be present, but clients | ||
| // still receive it in stripped state format (no extra fields) |
Collaborator
There was a problem hiding this comment.
I think that there was a small error in
TestMSC4311FullCreateEventOnStrippedState,
Yes, this test is flawed but there are two distinct pieces to MSC4311:
- Ensuring the
m.room.createevent is included in the client API (stripped state) - Ensuring all state events in the federation API are using full PDU including the
m.room.createwhich is required.
#796 fixes the second issue.
The first issue doesn't have a good test yet and I don't think this test is necessarily a "good" test. It looks like you've adapted what's here 👍 but it could be better. For example, even the test name here needs updating. And we could some of the same must.MatchGJSON(...) assertions here instead of these for loops. Also can be simplified to not use federation at all, etc, etc, etc.
Collaborator
There was a problem hiding this comment.
Overall, this is probably something we can tackle in #796
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I think that there was a small error in
TestMSC4311FullCreateEventOnStrippedState, the test was checking for extra fields in a stripped event while they shouldn't be thereComes with element-hq/synapse#19749
Pull Request Checklist
Signed-off-by: Thomas Traineau t.traineau@famedly.com