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

Allow IEndpointSource to return null Endpoint #172

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Allow IEndpointSource to return null Endpoint #172

merged 1 commit into from
Apr 8, 2019

Conversation

yousifh
Copy link
Contributor

@yousifh yousifh commented Apr 8, 2019

Let IEndpointSource return a null Endpoint during sending metrics
to allow them to be skipped.

Resolves #171

Let IEndpointSource return a null Endpoint during sending metrics
to allow them to be skipped.

Resolves #171
@yousifh yousifh requested a review from a team as a code owner April 8, 2019 01:50
@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #172 into master will decrease coverage by 1.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage   89.28%   88.24%   -1.04%     
==========================================
  Files          19       19              
  Lines         448      451       +3     
  Branches      121      122       +1     
==========================================
- Hits          400      398       -2     
- Misses         18       22       +4     
- Partials       30       31       +1
Impacted Files Coverage Δ
src/JustEat.StatsD/SocketTransport.cs 90.9% <100%> (+0.9%) ⬆️
src/JustEat.StatsD/ConnectedSocketPool.cs 72.5% <0%> (-5%) ⬇️
src/JustEat.StatsD/Buffered/BufferExtensions.cs 73.91% <0%> (-4.35%) ⬇️
src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs 76.19% <0%> (-2.39%) ⬇️

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 ae0cf6a...b114ee3. Read the comment docs.

@yousifh
Copy link
Contributor Author

yousifh commented Apr 8, 2019

I'm a little confused by the code coverage results. I didn't expect files that I didn't modify to be impacted by this change. I haven't used Codecov before so I hope I'm not missing something obvious if there are some more tests that I need to add.

@martincostello martincostello added this to the v4.0.1 milestone Apr 8, 2019

using (var transport = new SocketTransport(endpointSource, SocketProtocol.IP))
{
transport.Send("teststat:1|c");
Copy link
Member

Choose a reason for hiding this comment

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

So this test fails when your change is absent right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, without my change it would throw a null reference exception when the sockets pool is being created.

@martincostello
Copy link
Member

There's some level of randomness in the tests that we haven't eliminated, so sometimes the coverage results vary for code unrelated to a change.

It looks to me that you've added the appropriate test case to handle this change.

@martincostello martincostello merged commit d72b96c into justeattakeaway:master Apr 8, 2019
@martincostello
Copy link
Member

@yousifh Thanks for your contribution 👍

I'll look at publishing 4.0.1 to NuGet.org with this change later this afternoon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants