Injecting entire node into sessions is sloppy and impedes unit testing

[sanity]

cc: @ravisvi @jacksingleton @nomel7 @torcellite

Opening this issue initially for discussion.

One principle of object oriented design is that when you inject dependencies into a class through its constructor, you should only inject what it strictly requires.

The primary reason for this is that when you're unit testing the class, you don't need to mock a bunch of dependencies that aren't even used.

Currently we inject the entire node into sessions when they are created, because we don't know what specifically each session will require. This is bad for the above reasons.

One solution may be a dependency injection framework like Google Guice. It should allow each session to only request the specific dependencies they use, which will make it much easier to unit test the session implementations.

thoughts?

[jacksingleton]

I agree, see the amount of setup needed in TopologyMaintenanceSessionImplTest.

Heard good things about Guice - I don't think a framework is strictly necessary, pure constructor injection can work, but it might help encourage a more modular style.

[sanity]

Note that sessions are created dynamically, which is why all session implementations must currently take the same constructor parameters.

One option might be to make TrSessionManager smarter, behaving more like a dependency injection framework itself. The action happens on line 104 of TrSessionManager.java. Currently it requires a constructor which takes exactly 3 parameters, which must be an integer, a TrNode, and a TrSessionManager.

We could change it such that the first parameter must be an integer (this is the session id), the remaining parameters can be selected from a range of classes. So, for example, the constructor could have a TrPeerManager parameter type, TrSessionManager will inject the TrPeerManager when it sees this.

There are certain classes required by all TrSessionImpls, however, for example a ConnectionManager is required to establish new connections. We could mandate that any TrSessionImpl constructor must include these required objects, in addition to the session id.

Or perhaps there is a better way, that makes it easy to mock attempts to connect to remote sessions. We should discuss further.

[jacksingleton]

Once I've added a few more test cases to TopologyMaintenanceSessionImplTest my plan is to see how I can refactor TopologyMaintenanceSessionImpl itself to simplify the test setup.

We could use this to drive out some strategies for the rest of the code...

Will get back to you later this week once I've started to get into it. I might try some stuff out on a branch then ask for feedback as I'm pretty new to the codebase.

[sanity]

That sounds great, let me know once you'd like some feedback.

On Mon, Oct 21, 2013 at 12:31 PM, Jack Singleton
[email protected]:

Once I've added a few more test cases to
TopologyMaintenanceSessionImplTest my plan is to see how I can refactor
TopologyMaintenanceSessionImpl itself to simplify the test setup.

We could use this to drive out some strategies for the rest of the code...

Will get back to you later this week once I've started to get into it. I
might try some stuff out on a branch then ask for feedback as I'm pretty
new to the codebase.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/48#issuecomment-26738068
.

Ian Clarke
Blog: http://blog.locut.us/

[sanity]

@jacksingleton just wanted to check in and see if you've had a chance to make any progress on this?

If not, I might try to take a whack at it today.

[jacksingleton]

Nah not yet - go for it. I did complete the unit testing, will push that now.