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.

Sign up for GitHub

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

Jump to bottom

MatrixRTC: MembershipManager test cases #4713

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

MatrixRTC: MembershipManager test cases #4713

wants to merge 29 commits into from

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Feb 17, 2025 •
edited
Loading

Part of element-hq/element-call#2972.

This shouldn't introduce any behaviour changes, but refactor the tests to be based on the MembershipManager abstraction. It includes some tests which would fail on the current LegacyMembershipManager implementation which will be handled by a new implementation.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

hughns
hughns requested changes Feb 17, 2025
View reviewed changes
Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

Just a few quick things. I haven't looked at the main test cases yet.

@toger5 toger5 force-pushed the hughns/membershipmanager-test-cases branch from a2702a3 to 987863a Compare February 17, 2025 14:16
toger5 added a commit that referenced this pull request Feb 17, 2025
@toger5
typos in client.ts
127231b 
Pulled out from #4713
@toger5 toger5 mentioned this pull request Feb 17, 2025
Merged
4 tasks
github-merge-queue bot pushed a commit that referenced this pull request Feb 17, 2025
@toger5
typos in client.ts (#4715)
0efeac9 
Pulled out from #4713
@toger5 toger5 force-pushed the hughns/membershipmanager-test-cases branch from 8926281 to d9192f3 Compare February 17, 2025 15:05
@toger5 toger5 changed the title Hughns/membershipmanager test cases MembershipManager test cases Feb 17, 2025
@toger5 toger5 force-pushed the hughns/membershipmanager-test-cases branch from 8788ce4 to 3d46d05 Compare February 17, 2025 16:53
hughns
hughns requested changes Feb 19, 2025
View reviewed changes
@hughns hughns mentioned this pull request Feb 17, 2025
Open
@hughns hughns added T-Task Tasks for the team like planning and removed T-Enhancement labels Feb 19, 2025
@hughns hughns changed the title MembershipManager test cases MatrixRTC: MembershipManager test cases Feb 19, 2025
@toger5 toger5 requested a review from dbkr February 24, 2025 11:26
hughns
hughns requested changes Feb 25, 2025
View reviewed changes
Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

This review is for the new leave() test cases added by 4e60a35

Aside from this I have added in some comments on the open threads.

@toger5
Copy link
Contributor Author

toger5 commented Feb 26, 2025 •
edited
Loading

@hughns @dbkr I updated the flushPromise topic in the NewMembershipManager PR. Should I backport the change or should we keep it for now since we know all flushPromises will be removed once that is merged?

Back porting was easy enough. So I will create a commit very soon with the backport.

@toger5
Cleanup awaits and Make mock types more correct.
72994f2 
Make every mock return a Promise if the real implementation does return a pormise.
@toger5
remove flush promise dependency
ce24845
dbkr
dbkr approved these changes Feb 26, 2025
View reviewed changes
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, looks good.

@toger5 toger5 added this pull request to the merge queue Feb 26, 2025
@toger5 toger5 removed this pull request from the merge queue due to a manual request Feb 26, 2025
@toger5 toger5 added this pull request to the merge queue Feb 26, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hughns hughns hughns approved these changes

@dbkr dbkr dbkr approved these changes

@t3chguy t3chguy Awaiting requested review from t3chguy t3chguy was automatically assigned from matrix-org/element-web-team

Assignees
No one assigned
Labels
T-Task Tasks for the team like planning
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@toger5 @dbkr @hughns