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

[RTC-208] Add server socket tests #33

Merged
merged 5 commits into from Apr 25, 2023
Merged

[RTC-208] Add server socket tests #33

merged 5 commits into from Apr 25, 2023

Conversation

mickel8
Copy link
Contributor

@mickel8 mickel8 commented Apr 16, 2023

No description provided.

@mickel8 mickel8 changed the title Socket tests [RTC-208] Add server socket tests Apr 16, 2023
@codecov
Copy link

codecov bot commented Apr 16, 2023

Codecov Report

Merging #33 (4499386) into main (47e181d) will increase coverage by 8.35%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##             main      #33      +/-   ##
==========================================
+ Coverage   80.76%   89.12%   +8.35%     
==========================================
  Files          31       32       +1     
  Lines         364      377      +13     
==========================================
+ Hits          294      336      +42     
+ Misses         70       41      -29     
Impacted Files Coverage Δ
lib/jellyfish_web/peer_socket.ex 86.79% <66.66%> (+9.01%) ⬆️
lib/jellyfish_web/server_socket .ex 85.71% <100.00%> (+82.14%) ⬆️
test/support/ws.ex 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47e181d...4499386. Read the comment docs.

@mickel8 mickel8 force-pushed the socket-tests branch 15 times, most recently from e9ff86a to 8418250 Compare April 21, 2023 14:19
@mickel8 mickel8 marked this pull request as ready for review April 21, 2023 14:27
@mickel8 mickel8 requested a review from LVala April 21, 2023 14:27
Copy link
Member

Choose a reason for hiding this comment

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

Would be possible to also test peer/component crashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we are not able to get engine endpoint pid at the moment

Copy link
Member

Choose a reason for hiding this comment

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

I would wrap tests about connecting to the server (valid/invalid token, no peer etc.) in one describe (describe "connecting to server"?) and the two last test in describe "sends a message" or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, I don't see any benefits from using describe here. We will just introduce more nesting 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I think is creates nice visual distinction between connection tests, receiving information from socket tests etc., but I don't care about it that much.

Copy link
Member

Choose a reason for hiding this comment

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

As we mentioned offline, it's probably possible to move some tests from peer socket unit tests here.

@mickel8 mickel8 force-pushed the socket-tests branch 4 times, most recently from a992ed5 to 7282f9e Compare April 24, 2023 23:56
@mickel8 mickel8 force-pushed the socket-tests branch 2 times, most recently from 93591ca to 3cb9587 Compare April 25, 2023 00:23
Copy link
Member

@LVala LVala left a comment

Choose a reason for hiding this comment

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

👍, but maybe it would be nice to add a comment somewhere why we don't test peer/component crashes (membrane_rtc_engine limitations).

@mickel8 mickel8 merged commit 32f5b28 into main Apr 25, 2023
5 checks passed
@mickel8 mickel8 deleted the socket-tests branch April 25, 2023 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants