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

Send update_track_metadata also for pending subscriptions #237

Conversation

Rados13
Copy link
Contributor

@Rados13 Rados13 commented Feb 9, 2023

No description provided.

@Rados13 Rados13 requested a review from mickel8 as a code owner February 9, 2023 12:44
@Rados13 Rados13 self-assigned this Feb 10, 2023
@@ -101,8 +102,11 @@ defmodule Membrane.RTC.EngineTest do
)

assert_receive {:track_metadata_updated, %Track{id: ^track_id, metadata: "new-metadata"}}

:ok = Engine.terminate(rtc_engine)
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 put this in on_exit as assert :ok == Engine.terminate(rtc_engine, blocking?: true)

@@ -90,6 +90,7 @@ defmodule Membrane.RTC.EngineTest do
describe ":update_track_metadata" do
setup :setup_for_metadata_tests

@tag :debug
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed

@@ -19,8 +19,6 @@ defmodule Membrane.RTC.HLSEndpointTest do

Engine.register(pid, self())

on_exit(fn -> Engine.terminate(pid, blocking?: true) end)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
on_exit(fn -> Engine.terminate(pid, blocking?: true) end)
on_exit(fn -> assert :ok == Engine.terminate(pid, blocking?: true) end)

Comment on lines 198 to 228
Engine.message_endpoint(
rtc_engine,
"track-endpoint",
{:execute_actions, [notify: {:track_ready, track.id, hd(track.variants), track.encoding}]}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you create a separate story in Jira for adding tests for track_ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #237 (4058601) into master (8d38d8a) will decrease coverage by 0.56%.
The diff coverage is 100.00%.

❗ Current head 4058601 differs from pull request most recent head 7647c01. Consider uploading reports for the commit 7647c01 to get more accurate results

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage   57.63%   57.07%   -0.56%     
==========================================
  Files          38       35       -3     
  Lines        1787     1589     -198     
==========================================
- Hits         1030      907     -123     
+ Misses        757      682      -75     
Impacted Files Coverage Δ
lib/membrane_rtc_engine/engine.ex 77.25% <100.00%> (+2.96%) ⬆️
test/support/track_sender.ex 84.61% <0.00%> (-8.72%) ⬇️
test/support/file_endpoint.ex 88.23% <0.00%> (-7.22%) ⬇️
...gine/endpoints/webrtc/noop_connection_allocator.ex 28.57% <0.00%> (-4.77%) ⬇️
test/support/webrtc/test_connection_allocator.ex 25.00% <0.00%> (-3.58%) ⬇️
.../membrane_rtc_engine/endpoints/webrtc/forwarder.ex 56.52% <0.00%> (-3.48%) ⬇️
test/support/test_source.ex 51.85% <0.00%> (-1.72%) ⬇️
...ne_rtc_engine/endpoints/webrtc/variant_selector.ex 83.07% <0.00%> (-1.54%) ⬇️
test/support/test_sink.ex 78.57% <0.00%> (-1.43%) ⬇️
...mbrane_rtc_engine/endpoints/webrtc/track_sender.ex 71.13% <0.00%> (-0.30%) ⬇️
... and 17 more

Continue to review full report at Codecov.

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

@Rados13 Rados13 force-pushed the RTC-134-some-tests-in-the-rtc-engine-dont-terminate-engine-correctly branch from 4058601 to 2d40e63 Compare March 2, 2023 13:47
@Rados13 Rados13 force-pushed the RTC-134-some-tests-in-the-rtc-engine-dont-terminate-engine-correctly branch from bdf5485 to 7647c01 Compare March 2, 2023 13:51
@Rados13 Rados13 merged commit 247a3c3 into master Mar 3, 2023
@Rados13 Rados13 deleted the RTC-134-some-tests-in-the-rtc-engine-dont-terminate-engine-correctly branch March 3, 2023 08:02
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.

2 participants