A Mock Too Far?

I’ve long felt that the use of a lot of mock objects and other test doubles was a sign of a design smell. I would occasionally hand-build a test double for the rare case when I couldn’t find a better way, but I’d always look for a different option.

More recently, I’ve read Growing Object-Oriented Software, Guided By Tests by Steve Freeman and Nat Pryce, and Practical Object-Oriented Design in Ruby: An Agile Primer by Sandi Metz. Both books are excellent, and I highly recommend reading them. Both books also advocate the use of mocks and other doubles, and both make a compelling case. So, I’ve decided to re-evaluate my position and apply some of the techniques I’ve learned to some of the code I write.

In particular, I’ve been trying to use Sandi’s “rules” for testing, roughly paraphrased as:

• Test incoming messages for state.
• Don’t test outgoing query messages; use a stub to return any values needed to make the tests pass.
• Test outgoing command messages using mocks.
• Don’t test messages sent to self (a.k.a. private messages).

I’ve been using these rules to good effect in some code I was developing. I’m definitely encouraged to keep applying them and learning from them. However, I ran into one case where I think this approach went a little too far.

When learning a new idea or technique, I often find it helpful to over-use it for a brief time. It’s hard to know where the line is if I don’t cross it, and I can often push a new technique much further than I originally expected.

I was developing a Smalltalk interface to a piece of hardware. This hardware exposes a number of registers. Control registers are used to ask the hardware to do something, or to set a value that the hardware will use. The hardware reports information back to the software in status registers.

Sometimes, these registers are paired such that a request to a control register is acknowledged by a response in a corresponding status register. The typical pattern with these paired registers is to make a “synchronous call” to the hardware. That is, to make a request and wait for the hardware to respond in the status register before proceeding. There needs to be a timeout to keep the software from blocking indefinitely waiting for a response that never comes.

Because this is a common pattern, I decided to encapsulate it in a class, RegisterPair. I’d really like a better name, but no one on our team has come up with one yet. My best option so far is Controlinator, but that comes from watching too much Phineas and Ferb with my kids, and doesn’t really communicate the intent of the class.

I made use of ControlRegister and StatusRegister classes I’d already developed. These classes are implemented as valuables; that is, they make use of the common Smalltalk idiom of using #value: to set a value, and #value to get a value.

Given those classes, I test-drove an implementation of RegisterPair using mocks and stubs. In our code, we use SUnitToo and a locally-customized version of the SmallMock package from the Cincom Public Store Repository.

I ended up with the following tests:

Smalltalk.Hardware defineClass: #RegisterPairTest
	superclass: #{SUnit.TestCase}
	indexedType: #none
	private: false
	instanceVariableNames: ''
	classInstanceVariableNames: ''
	imports: ''
	category: ''
 
 
requestsWithSameResponse
	<test>
	| statusRegister |
	statusRegister := StatusRegister stub: #value with: [42].
	ControlRegister mock: [:controlRegister | controlRegister value: 42]
		verify:
			[:controlRegister |
			| pair |
			pair := RegisterPair control: controlRegister status: statusRegister.
			pair request: 42]
 
 
requestsWithDifferentResponse
	<test>
	| statusRegister |
	statusRegister := StatusRegister stub: #value with: [24].
	ControlRegister mock: [:controlRegister | controlRegister value: 42]
		verify:
			[:controlRegister |
			| pair |
			pair := RegisterPair control: controlRegister status: statusRegister.
			pair request: 42 expecting: 24]
 
 
waitsForStatusRegisterToChange
	<test>
	ControlRegister mock: [:controlRegister | controlRegister value: 42]
		verify:
			[:controlRegister |
			StatusRegister mock:
					[:register |
					register
						value;
						mockReturn: 0;
						value;
						mockReturn: 42]
				verify:
					[:register |
					pair := RegisterPair control: controlRegister status: register.
					pair request: 42]]
 
 
timesOutIfStatusRegisterDoesntRespond
	<test>
	| statusRegister |
	statusRegister := StatusRegister stub: #value with: [0].
	ControlRegister mock: [:controlRegister | controlRegister value: 42]
		verify:
			[:controlRegister |
			| pair |
			pair := RegisterPair control: controlRegister status: statusRegister.
			pair timeout: 1 milliseconds.
			self should: [pair request: 42 expecting: 24] raise: RegisterTimeout]
 
 
reportsStatus
	<test>
	| statusRegister pair |
	statusRegister := StatusRegister stub: #value with: [24].
	pair := RegisterPair control: nil status: statusRegister.
	self assert: pair status equals: 24

When I was finished, I looked at the tests. It seemed like there was too much noise, and they weren’t as clear as I liked. There is certainly some duplication that could be extracted. That might help, but it might also make the tests less understandable. I could also do some additional work on SmallMock to make it less verbose. Having played with some Ruby mocking libraries lately, I have some ideas on how to go about that, but I didn’t want to shave that yak here.

So, I took another run at it, this time using the existing ObservedValue as a duck-typed stand-in for the registers. Since ObservedValue is also a valuable, this works pretty well. I kept one of the mocks in #waitsForStatusRegisterToChange, where the value returned by the status register has to change in the middle of the test.

Here are the new tests:

Smalltalk.Hardware defineClass: #RegisterPairTest
	superclass: #{SUnit.TestCase}
	indexedType: #none
	private: false
	instanceVariableNames: 'controlRegister statusRegister pair '
	classInstanceVariableNames: ''
	imports: ''
	category: ''
 
 
setUp
	controlRegister := 0 asObservedValue.
	statusRegister := 0 asObservedValue.
	pair := RegisterPair control: controlRegister status: statusRegister
 
 
requestsExpectingSameResponse
	<test>
	statusRegister value: 42.
	pair request: 42.
	self assert: controlRegister value equals: 42
 
 
requestsExpectingDifferentResponse
	<test>
	statusRegister value: 24.
	pair request: 42 expecting: 24.
	self assert: controlRegister value equals: 42
 
 
waitsForStatusRegisterToChange
	<test>
	StatusRegister mock:
			[:register |
			register
				value;
				mockReturn: 0;
				value;
				mockReturn: 42]
		verify:
			[:register |
			pair := RegisterPair control: controlRegister status: register.
			pair request: 42]
 
 
timesOutIfStatusRegisterDoesntRespond
	<test>
	statusRegister value: 42.
	pair timeout: 1 milliseconds.
	self should: [pair request: 42 expecting: 24] raise: RegisterTimeout
 
 
reportsStatus
	<test>
	statusRegister value: 24.
	self assert: pair status equals: 24

I’m happier with this latest version. I’m still using test doubles; they just happen to be instances of an existing class in the system that shares the interface I need.

Since I wrote this code, I’ve done some work to make our mocking library less verbose. I have more to do, but I’m finding that I like the tests better when less of their surface area is taken up by the mock code.

I expect I’ll have more to say on this topic as I gain more experience with it.