mmillerbe commented 4 years ago

Previous code casted to the expected type directly, which succeeded in the case monitorEvent.Arg was null. The current is check will be false for null values, causing an exception to be thrown which the attached Poller doesn't handle.

codecov[bot] commented 4 years ago

Codecov Report

Merging #938 (984d563) into master (4a80a4a) will increase coverage by 0.54%. The diff coverage is 62.50%.

:exclamation: Current head 984d563 differs from pull request most recent head 3f4a1ec. Consider uploading reports for the commit 3f4a1ec to get more accurate results
@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
+ Coverage   65.70%   66.25%   +0.54%     
==========================================
Files         148      148              
Lines        9071     8993      -78     
Branches     1445     1477      +32     
==========================================
- Hits         5960     5958       -2     
+ Misses       2506     2427      -79     
- Partials      605      608       +3     

Impacted Files	Coverage Δ
src/NetMQ/Monitoring/NetMQMonitorEventArgs.cs	`55.00% <0.00%> (ø)`
src/NetMQ/Monitoring/NetMQMonitor.cs	`67.25% <50.00%> (+0.58%)`	:arrow_up:
src/NetMQ/Core/MonitorEvent.cs	`83.54% <100.00%> (+5.16%)`	:arrow_up:
src/NetMQ/NetMQConfig.cs	`45.09% <0.00%> (-9.81%)`	:arrow_down:
src/NetMQ/Core/Patterns/Gather.cs	`66.66% <0.00%> (-4.77%)`	:arrow_down:
src/NetMQ/Core/Utils/Poller.cs	`75.60% <0.00%> (-3.66%)`	:arrow_down:
src/NetMQ/RoutingKey.cs	`6.00% <0.00%> (-3.62%)`	:arrow_down:
src/NetMQ/Core/Patterns/Client.cs	`61.29% <0.00%> (-3.23%)`	:arrow_down:
src/NetMQ/Core/Transports/Tcp/TcpAddress.cs	`54.54% <0.00%> (-3.04%)`	:arrow_down:
src/NetMQ/Core/Patterns/Server.cs	`59.32% <0.00%> (-1.70%)`	:arrow_down:
... and 12 more

mmillerbe commented 3 years ago

I pushed up a slight refactoring here that added some tests so that the code coverage wouldn't decrease. Is that more acceptable? I'm happy to make whatever changes you'd like if there's some other way you think this should be handled. (To suppress warnings, I needed to add a nullable indicator on a property in a public-facing API. I think it's "correct" in that the socket can be null, but I'm not sure if that's desirable from your perspective.)

Thanks! Mike

mmillerbe commented 3 years ago

@drewnoakes, apologies for the tag, but do you think we could get this merged in? I'd love to get back on the main branch, but I'm currently unable to because of this. I'd be happy make whatever changes to the PR you think are necessary. Thanks very much!