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

Add unit tests for hangup / reject #2606

Merged
merged 2 commits into from
Aug 22, 2022
Merged

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Aug 19, 2022

Fixes element-hq/element-call#537

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@dbkr dbkr added the T-Task Tasks for the team like planning label Aug 19, 2022
@dbkr dbkr requested a review from a team as a code owner August 19, 2022 17:28
@SimonBrandner
Copy link
Contributor

Except the tests actually don't seem to be passing :/

Comment on lines +1572 to +1575

// make sure we're still going
if (this.callHasEnded()) return;

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be in setState()

Copy link
Contributor

@SimonBrandner SimonBrandner Aug 19, 2022

Choose a reason for hiding this comment

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

I should go to sleep, anyway...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I did wonder about this and it seems quite elegant to basically just disallow state transitions out of 'ended', at the price of having to wrap all our setState calls in a try/catch (or some way to make the caller stop what it's doing).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so what I meant by I should go to sleep, anyway... is that there is no simple way to do this, so I think this is actually fine (same for the other comment)

Comment on lines +1583 to +1585
// make sure the call hasn't ended before we continue
if (this.callHasEnded()) return;

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be in sendAnswer()

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly - I went for here as it's straight after it comes back from await-ing.

@dbkr dbkr merged commit 92cd84f into robertlong/group-call Aug 22, 2022
@dbkr dbkr deleted the dbkr/test_hangup branch August 22, 2022 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants