write a fake Replicator

[joshua-g]

Some tests would be greatly simplified by giving the Replicator interface a simple "fake" implementation, because using mocks to express the behavior of the interface is cumbersome.

[joshua-g]

Here's my thinking.

We have a pattern that comes up a lot in our unit tests like this. We want to test a class U using a unit test. And U is written using a ton of dependency injection in its constructor, because usually that's "good". So one thing U needs to do internally is to create R objects. And it does that using a dependency-injected factory F.

class U {
  public U(F factoryOfRs) { ... }
}

So now we write unit tests for U. And we end up expressing expectations on objects of R, like this:

new Expectations() {{
  ...
  oneOf(factory).getR(); will(returnValue(mockOfR));
  allowing(mockOfR).doFoo();
  allowing(mockOfR).doBar(); ...
}}

I think this is a problem, because U's use of R objects is an implementation detail of U. It's exactly the sort of thing that unit testing U should not care about. Later on, we might want to change the way it uses these objects, or we might want to change their interface -- and doing so, in an ideal world, only the unit tests of R would need to change. But this way is brittle -- all U tests would need to change if a new method was added to R, for instance. Or if a method being called once was called twice.

So I think the way around that is to create fake R's.

new Expectations() {{
  ...
  oneOf(factory).getR(); will(returnValue(fakeR));
}}

Alex pointed out that this might not be a problem with jmock; it might be a design problem which test brittleness is exposing. That's possible too.

[ryanobjc]

I'm not sure I agree, the use of the sub-object R is definitely part of how U is supposed to work. How we interact with our neighbors is part of our test specing. It's part of the external contract, that is how we call our dependencies. There is some implementation leakage, but we reduce that specification by not forcing an order to which the calls happen (jMock's default behavior).

One of the things here is that fakeR's must be exactly like the real R, so we need to test the fakeR with the R unit-test ideally.

[joshua-g]

Hmm... okay, I buy that. Either way, the contracts that Replicator obeys are nontrivial, including things like what events its users expect it to emit, what commit notices, and how the commit notices interact with the receipts returned from logData(). That logic has to be repeated each time a mock Replicator is used. So if not a fake object, then perhaps a helper method or class for setting up what Expectations a mock replicator should satisfy?

You could say that this is a smell that Repicator interface is too complex (ten methods!), and that may be, but I wonder if "simplifying it" is a good idea?

[ryanobjc]

I think the fake object is ok as is for now, but we should be on the look out for false confidence. At some level we are expressing behavior of a real replicator in this fake replicator, although maybe we dont need to meet the same concurrency/multi-threaded demands exactly.

[ryanobjc]

another way to evaluate the replicator interface is to ask if its a coherent interface or not. can we separate the concerns in to different interfaces?

[joshua-g]

1. start() probably doesn't need to be in this interface.
2. isLeader() is redundant since its information is also returned by getStateChannel()
3. getId() and getQuorumId() return information that the one who possesses the Replicator should already know.
4. getQuorumConfiguration() is currently unused, and could probably be removed as well.

Removing those all would go a long way.

[joshua-g]

Looks like isLeader() is unused too, actually.

[ryanobjc]

start() is def an implementation detail, particular to how we are using fibers. I probably wouldn't get rid of isLeader(), getId, getQuorumId though yet.

The big question is, would removing this really make the interface easier to test?

[joshua-g]

It would make it easier to implement and understand. If code were to start using isLeader and getQuorumConfiguration, that would add test complexity, so removing it preemptively simplifies those tests. :D

The mock already needs to use some kind of jmock sequence or jmock state machine to work, and this doesn't remove that complexity.

[ryanobjc]

Well I vote for simplicity every day of the week and twice on sundays. I'll leave the rest to you. File some issues if you wish.

[joshua-g]

Kk. Will file them on the jira site.

[ryanobjc]

I think we can do the C5 tickets here on Github.