Skip to content

Conversation

@mickel8
Copy link
Contributor

@mickel8 mickel8 commented Apr 3, 2024

No description provided.

@codecov
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Merging #34 (c2e41ab) into relay-cand (d51ceae) will decrease coverage by 7.47%.
The diff coverage is 44.40%.

❗ Current head c2e41ab differs from pull request most recent head 7e49dd4. Consider uploading reports for the commit 7e49dd4 to get more accurate results

Additional details and impacted files
@@              Coverage Diff               @@
##           relay-cand      #34      +/-   ##
==============================================
- Coverage       77.73%   70.27%   -7.47%     
==============================================
  Files              25       25              
  Lines            1132     1265     +133     
==============================================
+ Hits              880      889       +9     
- Misses            252      376     +124     
Files Coverage Δ
lib/ex_ice/priv/candidate.ex 87.50% <ø> (ø)
lib/ex_ice/priv/candidate/host.ex 71.42% <ø> (-3.58%) ⬇️
lib/ex_ice/priv/candidate/prflx.ex 57.14% <ø> (+7.14%) ⬆️
lib/ex_ice/priv/candidate/srflx.ex 42.85% <ø> (-19.65%) ⬇️
lib/ex_ice/priv/checklist.ex 100.00% <100.00%> (ø)
lib/ex_ice/priv/gatherer.ex 94.87% <100.00%> (+0.27%) ⬆️
lib/ex_ice/ice_agent.ex 48.88% <0.00%> (-1.12%) ⬇️
lib/ex_ice/priv/candidate_pair.ex 59.09% <62.50%> (+1.94%) ⬆️
lib/ex_ice/priv/candidate/relay.ex 0.00% <0.00%> (ø)
lib/ex_ice/priv/ice_agent.ex 71.65% <45.13%> (-9.88%) ⬇️

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 d51ceae...7e49dd4. Read the comment docs.

@mickel8 mickel8 force-pushed the ex_turn branch 17 times, most recently from c2e41ab to 78145b0 Compare April 10, 2024 06:59
@mickel8 mickel8 marked this pull request as ready for review April 10, 2024 07:27
@mickel8
Copy link
Contributor Author

mickel8 commented Apr 10, 2024

I am gonna add some tests in the next PR

@mickel8 mickel8 requested a review from LVala April 10, 2024 07:27
Copy link
Contributor

Choose a reason for hiding this comment

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

😠

optional(:credential) => String.t()
}
],
ice_transport_policy: :all | :relay | nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why nil is a valid value? The typedoc a bit above does not list it. If this is to indicate that the value is optional, I don't think this is the correct way to spec it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍

@type t() :: %__MODULE__{
id: integer(),
local_cand: Candidate.t(),
local_cand_id: Candidate.id(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to comment that the typespec of candidate ids in ExICE.Candidate and ExICE.Priv.Candidate differs (integer() in the first one, Candidate.id()) which is a super minor issue, but, for the love of god, I cannot find the place where Candidate.id is defined and I'm so dumbfounded, where is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I have deleted it 🤔 Moving to integer everywhere

@mickel8 mickel8 merged commit 80487bb into relay-cand Apr 12, 2024
@mickel8 mickel8 deleted the ex_turn branch April 12, 2024 16:23
@mickel8 mickel8 changed the title Add support for relay candidates Gather and handle relay candidates Apr 12, 2024
mickel8 added a commit that referenced this pull request Apr 15, 2024
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.

3 participants