Skip to content
This repository was archived by the owner on Sep 19, 2024. It is now read-only.

Integration tests for MembraneRTC client #45

Merged
merged 6 commits into from
Mar 8, 2022
Merged

Conversation

Qizot
Copy link
Contributor

@Qizot Qizot commented Nov 3, 2021

Integration tests with the use of Playwright

Because of the problems described below tests were rewrote to Playwright. The creation of Dockerfile and docker-compose.yml is aimed to help in future debugging problems inside CI docker.

closes #5

Old issue about tests with the use of Wallaby

Problem with putting the integration tests inside docker:

It happens that libnice does not want to cooperate when it is being put inside docker, no matter if it is docker or host network. The results are unpredictable, when the first peer joins then he will get connected successfully but when the second peer joins then the libnice gets stuck in CONNECTING state (just for the first peer, the second one can see the first one without any problems).

The whole problem happens when the first peer is in a READY state and ice restart gets triggered. It automatically gets into CONNECTING state awaiting a new nominated ICE pair but none gets nominated therefore the agent is stuck in CONNECTING state forever. This is unpredictable but sometimes happens to work, this situation does not happen when developing locally without docker. Due to such thing happening it does not make any sense to put integration tests inside docker as it will fail most of the time.

The problem may be connected with lower resources available docker compared to those of an operating system when using locally, probably happened before on a weaker macbook without docker.

@Qizot Qizot requested a review from Rados13 November 3, 2021 13:12
@Qizot Qizot requested a review from Rados13 November 4, 2021 09:29
Copy link
Contributor

@Rados13 Rados13 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 think we should check the situation when multiple people leave the room at once and the room is still alive.

@Qizot
Copy link
Contributor Author

Qizot commented Nov 5, 2021

I still think we should check the situation when multiple people leave the room at once and the room is still alive.

But the code doing that is basically there
https://github.com/membraneframework/membrane_rtc_engine/pull/45/files#diff-05b8aa2fc28a875508015a4d44a16f2f3907a27df5f0f51ad7f1dcdc797e8661R110

@Qizot Qizot requested a review from Rados13 November 5, 2021 13:32
@Rados13
Copy link
Contributor

Rados13 commented Nov 5, 2021

I still think we should check the situation when multiple people leave the room at once and the room is still alive.

But the code doing that is basically there https://github.com/membraneframework/membrane_rtc_engine/pull/45/files#diff-05b8aa2fc28a875508015a4d44a16f2f3907a27df5f0f51ad7f1dcdc797e8661R110

Maybe I don't understand something but I don't see there any code responsible for leaving room by peers.

@Qizot
Copy link
Contributor Author

Qizot commented Nov 5, 2021

I still think we should check the situation when multiple people leave the room at once and the room is still alive.

But the code doing that is basically there https://github.com/membraneframework/membrane_rtc_engine/pull/45/files#diff-05b8aa2fc28a875508015a4d44a16f2f3907a27df5f0f51ad7f1dcdc797e8661R110

Maybe I don't understand something but I don't see there any code responsible for leaving room by peers.

Ohh, I misunderstood you... I will add the burst leaving

@Rados13 Rados13 requested review from mickel8 and removed request for Rados13 February 24, 2022 14:39
@Rados13 Rados13 requested a review from mickel8 March 2, 2022 14:53
Comment on lines 17 to 28
# command: bash -c "mix deps.update --all && ls /root/.cache/ms-playwright && mix integration"
environment:
# VIRTUAL_HOST: "videoroom.membraneframework.org"
# LETSENCRYPT_HOST: "videoroom.membraneframework.org"
# USE_INTEGRATED_TURN: "true"
# INTEGRATED_TURN_IP: "135.181.203.255"
# INTEGRATED_TURN_PORT_RANGE: "50000-50500"
# INTEGRATED_TCP_TURN_PORT: "50500"
MIX_ENV: test
ports:
# - "50000-50500:50000-50500/tcp"
# - "50000-50500:50000-50500/udp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I see. I think we should turn them on

@Rados13 Rados13 requested a review from mickel8 March 3, 2022 14:37
Comment on lines 59 to 61
# turn_servers: [],
# dtls_pkey: Application.get_env(:test_videoroom, :dtls_pkey),
# dtls_cert: Application.get_env(:test_videoroom, :dtls_cert)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's delete this

Comment on lines 129 to 130
# stun_servers: state.network_options[:stun_servers] || [],
# turn_servers: state.network_options[:turn_servers] || [],
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

@Rados13 Rados13 force-pushed the integration-tests branch from 4e2af8a to ea3247e Compare March 8, 2022 08:12
@Rados13 Rados13 force-pushed the integration-tests branch from ea3247e to fdf7397 Compare March 8, 2022 15:38
@Rados13 Rados13 merged commit f8c053e into master Mar 8, 2022
@Rados13 Rados13 deleted the integration-tests branch March 8, 2022 16:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup very simple integration tests
3 participants