Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
==========================================
- Coverage 86.87% 86.84% -0.04%
==========================================
Files 64 64
Lines 1227 1247 +20
==========================================
+ Hits 1066 1083 +17
- Misses 161 164 +3
Continue to review full report in Codecov by Sentry.
|
| test "renders room when max_peers isn't present", %{conn: conn} do | ||
| conn = post(conn, ~p"/room") | ||
| assert %{"id" => id} = json_response(conn, :created)["data"]["room"] | ||
|
|
||
| conn = get(conn, ~p"/room/#{id}") | ||
| response = json_response(conn, :ok) | ||
| assert_response_schema(response, "RoomDetailsResponse", @schema) | ||
|
|
||
| assert %{ | ||
| "id" => ^id, | ||
| "config" => %{}, | ||
| "components" => [], | ||
| "peers" => [] | ||
| } = response["data"] | ||
| end |
There was a problem hiding this comment.
Why did you remove this test?
There was a problem hiding this comment.
It doesn't add anything of value, the checks performed here are already done in "renders room when data is valid" and "renders room when data is valid, custom room_id"
There was a problem hiding this comment.
Practically speaking, yes; philosophically, no. It is not the responsibility of these tests to verify that action X occurs, especially if it's not mentioned in their names. However, it is easy to accidentally refactor these tests and unintentionally remove coverage for this edge case. If you insist on removing this test, I suggest adding a note to one of the other tests indicating that it also covers the situation previously handled by the removed test.
sgfn
force-pushed
the
sgfn/RTC-385-autodelete-rooms
branch
from
4df32c1 to
573c34f
Compare
| "Removing room because it was peerless for #{state.config.peerless_purge_timeout} seconds" | ||
| ) | ||
|
|
||
| {:stop, :normal, state} |
There was a problem hiding this comment.
Stopping with normal won't cause other processes that are linked to the room process to exit. I am not sure if that's correct. I think I would go for {:shutdown, :peerless_purge}? See https://hexdocs.pm/elixir/1.16.0/Supervisor.html#module-exit-reasons-and-restarts
There was a problem hiding this comment.
But that's exactly how RoomService removes rooms -- here
We're manually removing all of the linked processes in the terminate callback, no?
| test "renders room when max_peers isn't present", %{conn: conn} do | ||
| conn = post(conn, ~p"/room") | ||
| assert %{"id" => id} = json_response(conn, :created)["data"]["room"] | ||
|
|
||
| conn = get(conn, ~p"/room/#{id}") | ||
| response = json_response(conn, :ok) | ||
| assert_response_schema(response, "RoomDetailsResponse", @schema) | ||
|
|
||
| assert %{ | ||
| "id" => ^id, | ||
| "config" => %{}, | ||
| "components" => [], | ||
| "peers" => [] | ||
| } = response["data"] | ||
| end |
There was a problem hiding this comment.
Practically speaking, yes; philosophically, no. It is not the responsibility of these tests to verify that action X occurs, especially if it's not mentioned in their names. However, it is easy to accidentally refactor these tests and unintentionally remove coverage for this edge case. If you insist on removing this test, I suggest adding a note to one of the other tests indicating that it also covers the situation previously handled by the removed test.
sgfn
changed the title
|
Bumped Elixir to 1.15 because we use Map.intersect which was added in that version EDIT: disregard, restored 1.14 because test_load_balancing uses 1.14. Removed calls to Map.intersect instead |
This reverts commit 573c34f.
sgfn
force-pushed
the
sgfn/RTC-385-autodelete-rooms
branch
from
171c5fa to
9262b6c
Compare
3 participants
Acknowledging the stipulations set forth: