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

Test placing a call in a group call #2593

Merged
merged 14 commits into from
Aug 16, 2022

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Aug 16, 2022

Refactors a bit of the call testing stuff

Fixes element-hq/element-call#521

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.

Refactors a bit of the call testing stuff

Fixes element-hq/element-call#521
@dbkr dbkr added the T-Task Tasks for the team like planning label Aug 16, 2022
@dbkr dbkr requested a review from a team as a code owner August 16, 2022 13:34
Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Looking awesome!

Comment on lines 93 to 96
expect(mockSendState.mock.calls[0][0]).toEqual(FAKE_ROOM_ID);
expect(mockSendState.mock.calls[0][1]).toEqual(EventType.GroupCallPrefix);
expect(mockSendState.mock.calls[0][2]["m.type"]).toEqual(GroupCallType.Video);
expect(mockSendState.mock.calls[0][2]["m.intent"]).toEqual(GroupCallIntent.Prompt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to use toHaveBeenCalledWith()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I managed to replace one with that using expect.objectContaining(), although it seemed to confuse the other ones.

}
});

it("unmutes audio", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am struggling to see what exactly this is doing/why it has this name

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fairly basic and just asserts that calling the mute API correctly disables the stream. I've updated the name to maybe make it clearer.

}
});

it("unmutes video", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am struggling to see what exactly this is doing/why it has this name

Copy link
Member Author

Choose a reason for hiding this comment

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

Have likewise updated the name, (but yes, it's still perhaps a bit of silly test, but easy to write).

spec/test-utils/webrtc.ts Show resolved Hide resolved
spec/unit/webrtc/groupCall.spec.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/groupCall.spec.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/groupCall.spec.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/groupCall.spec.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/groupCall.spec.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/groupCall.spec.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/groupCall.spec.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/groupCall.spec.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/groupCall.spec.ts Show resolved Hide resolved
@dbkr dbkr requested a review from robintown August 16, 2022 16:45
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

I still have the one comment about the assertion in disables audio stream when audio is set to muted, but otherwise looks good

@dbkr
Copy link
Member Author

dbkr commented Aug 16, 2022

Oh, so you did - github had hidden it for me :/

@dbkr dbkr merged commit e8f682f into robertlong/group-call Aug 16, 2022
@dbkr dbkr deleted the dbkr/group_call_placecall_test branch August 16, 2022 17:22
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.

3 participants