NetworkReachabilityManager subclassing issue #1809

Closed
SlashDevSlashGnoll opened this Issue Nov 28, 2016 · 3 comments

Projects

None yet

3 participants

@SlashDevSlashGnoll

Hi There,

We're trying to write unit tests around our usage of NetworkReachabilityManager by subclassing it to mock out some behavior for the purposes of tests. We seem to have identified an issue because the class is marked as open but because of the design of the initializers there doesn't seem to be a way to subclass it in a way that compiles. Currently there are 3 inits for NetworkReachabilityManager:

  1. A private designated initializer
  2. A convenience initializer that takes a hostname
  3. A convenience initializer that takes no parameters

This is a problem because if we create a subclass there is no way to properly init it as we cannot call the base class designated initializer which is required by the compiler.

Either the initializers need to be adjusted to allow for this or perhaps the class should not be marked as open.

@jshier
Contributor
jshier commented Nov 29, 2016

While I question the need to do you own testing on a class Alamofire provides, since we already have testing for it, thanks for the report. We'll investigate. Cheers! 🍻

@jshier jshier added the enhancement label Nov 29, 2016
@jshier jshier self-assigned this Nov 29, 2016
@SlashDevSlashGnoll
SlashDevSlashGnoll commented Nov 29, 2016 edited

To clarify, it wasn't to test the ReachabilityManager, it was a way to mock out its behavior to test OUR code that utilizes the ReachabilityManager 😄

The goal was to make it mockable so you could define a protocol for this which we could mock or adjust the initializers for subclassing. Either would meet the need.

@cnoon cnoon self-assigned this Dec 16, 2016
@cnoon cnoon added this to the 4.3.0 milestone Dec 16, 2016
@cnoon cnoon added a commit that referenced this issue Dec 19, 2016
@cnoon cnoon [Issue #1809] Moved the NetworkReachabilityManager back to public ACL.
This class was never intended to be subclassable. It was incorrectly made open and should have always been public instead. Proxying can be done at the app levels using protocol rather than subclassing.
779930c
@cnoon
Member
cnoon commented Dec 19, 2016

Thanks for letting us know @SlashDevSlashGnoll. You are absolutely right, it was not set up correctly at all. Upon further investigation, we realized we never should have made the class open in the first place. This is not a feature that we really want people customizing. Therefore, I moved it back to a public ACL in 779930c.

If you need to "proxy" the functionality for testing, you'll want to create a proxy protocol and have the NetworkReachabilityManager conform to it. Then you can customize everything you need for your own internal testing.

This change will go out as part of the 4.3.0 release here shortly.

Thanks again! 🍻

@cnoon cnoon closed this Dec 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment