My conversations with gullible machines...: My solution for UncleBob's Mark IV CoffeeMaker

Part 3: Beware of threads

The CoffeeMaker opens the Relief valve in case someone takes the coffee pot off the plate to prevent coffee from falling onto the plate
From the seq dia #2,the CoffeeMaker just needs to listen for changed events from the WarmerPlate.
If new status is WarmerEmpty, open the valve to reduce pressure/stop water flow. Else keep it closed.


[TestFixture]
public class TestCoffee_Maker
{
 private const int I_WAIT_FOR_EVENTS_TO_FIRE = 300;
 [Test]
 public void Test_ValveIsOpened_IfCoffeePotIsRemoved_ElseClosed()
 {
     Coffee_Maker obCoffeeMaker = new Coffee_Maker();
     FakeCoffeeMakerDevice obDevice = new FakeCoffeeMakerDevice();
     obCoffeeMaker.Monitor(obDevice);
     obDevice.WarmerPlateStatus = WarmerPlateStatus.POT_NOT_EMPTY;
     Thread.Sleep(I_WAIT_FOR_EVENTS_TO_FIRE);

     Assert.AreEqual(ReliefValveState.CLOSED, obDevice.ReliefValveState,
         "Relief valve should be closed when Pot With Coffee is on the plate");
     obDevice.WarmerPlateStatus = WarmerPlateStatus.WARMER_EMPTY;
     Thread.Sleep(I_WAIT_FOR_EVENTS_TO_FIRE);

     Assert.AreEqual(ReliefValveState.OPEN, obDevice.ReliefValveState,
         "Relief valve should be open since the warmer plate is now empty");

 }
}

Red! make it pass like this.


public class Coffee_Maker
{
 SensorObserver m_obObserver;
 CoffeeMakerAPI m_obDevice;

 public void Monitor(CoffeeMakerAPI obDevice)
 {
     m_obObserver = new SensorObserver();
     m_obDevice = obDevice;

     m_obObserver.Observe(m_obDevice);
     m_obObserver.WarmerPlateStatusChanged += new EventHandler(On_WarmerPlateStatusChanged);

 }

 void On_WarmerPlateStatusChanged(object sender, EventArgs obEvtArgs)
 {
     if (WarmerPlateStatus.WARMER_EMPTY == m_obDevice.GetWarmerPlateStatus())
     {
         m_obDevice.SetReliefValveState(ReliefValveState.OPEN);
     }
 }
}

Next we extend to check if it is closed when the pot is placed back on it. On second thoughts.. let me write 4 test cases for each transition… be thorough. One at at time.. Improve the name of the first one


public void Test_ValveIsOpened_IfPotWithCoffeeIsRemovedFromThePlate() {..}
[Test]
public void Test_ValveIsClosed_IfPotWithCoffeeIsReplaced ()
{
 Coffee_Maker obCoffeeMaker = new Coffee_Maker();
 FakeCoffeeMakerDevice obDevice = new FakeCoffeeMakerDevice();
 obCoffeeMaker.Monitor(obDevice);
 obDevice.WarmerPlateStatus = WarmerPlateStatus.WARMER_EMPTY;
 Thread.Sleep(I_WAIT_FOR_EVENTS_TO_FIRE);

 Assert.AreEqual(ReliefValveState.OPEN, obDevice.ReliefValveState,
     "Relief valve should be open since the warmer plate is now empty");
 obDevice.WarmerPlateStatus = WarmerPlateStatus.POT_NOT_EMPTY;
 Thread.Sleep(I_WAIT_FOR_EVENTS_TO_FIRE);

 Assert.AreEqual(ReliefValveState.CLOSED, obDevice.ReliefValveState,
     "Relief valve should be closed when Pot With Coffee is on the plate");
}

Make it green by


void On_WarmerPlateStatusChanged(object sender, EventArgs obEvtArgs)
{
 if (WarmerPlateStatus.WARMER_EMPTY == m_obDevice.GetWarmerPlateStatus())
 {
     m_obDevice.SetReliefValveState(ReliefValveState.OPEN);
 }
 else
 {
     m_obDevice.SetReliefValveState(ReliefValveState.CLOSED);
 }
}

Still red ??!! aha.. hit upon a special case. If the coffee maker is started with no pot on the plate, the valve should be open. Since no ‘change’ occurs.. event is never fired. We can fix that by calling the above function at the end of Monitor() to initialize the valve correctly.. Done
Refactoring time: Move common code to a Setup method and extract local variables to members
The second test fails intermittently… bad omen. Is it because two threads are accessing the same variable.? think the variable needs to be marked as volatile. Still no luck.
Looks like my TDD badge is going to be revoked. I put in some consoles traces to see if the event is being raised.

A detour into 'Thrashing around in a multi-threaded environment'
I find that when the test fails, the LastPlateStatus is WARMER_EMPTY to begin with.. hence the event is never raised. The consoles/traces are:
Start Listener thread
Change to WE
Listener thread active
Check WARMER_EMPTY with WARMER_EMPTY
Check WARMER_EMPTY with WARMER_EMPTY
Check WARMER_EMPTY with WARMER_EMPTY
Check WARMER_EMPTY with WARMER_EMPTY

And when it works
Start Listener thread
Listener thread active
Check POT_EMPTY with POT_EMPTY
Change to WE
Check WARMER_EMPTY with POT_EMPTY
WARMER_EMPTY <= POT_EMPTY Check WARMER_EMPTY with WARMER_EMPTY Check WARMER_EMPTY with WARMER_EMPTY Check POT_NOT_EMPTY with WARMER_EMPTY POT_NOT_EMPTY <= WARMER_EMPTY Check POT_NOT_EMPTY with POT_NOT_EMPTY Check POT_NOT_EMPTY with POT_NOT_EMPTY

Batman to the drawing board. I trace out the seq of events. (Unnecessary in hind sight..I can see that the issue is clear from the above traces).
Aha! In the RED Case, The listener thread starts after the WarmerPlateStatus has been changed to WE.. i.e. the listener thread is slow off the blocks.
From my doodling, I see that it’d be better if I ensure that the listener thread is started before the SensorObserver.Observe method exits. Let’s see if that is the problem.


public void Observe(CoffeeMakerAPI obDevice)
{
 m_obDevice = obDevice;
 obListenerThread = new Thread(new ThreadStart(ListenForChanges));
 obListenerThread.Start();

 WaitTillListenerThreadHasStarted();
 Console.WriteLine("Start Listener thread ");
}
private void WaitTillListenerThreadHasStarted()
{
 while (obListenerThread.ThreadState != ThreadState.Running) ;
}

private void ListenForChanges()
{
 Console.WriteLine("Listener thread active");
 ...

Aaarghh!! Still intermittent red. When in doubt.. I add more traces.. (expert TDDer at work here.. NOT)


public void Observe(CoffeeMakerAPI obDevice)
{
 m_obDevice = obDevice;
 obListenerThread = new Thread(new ThreadStart(ListenForChanges));
 obListenerThread.Start();

 Console.WriteLine("Start Listener thread ");
 WaitTillListenerThreadIsRunning();
 Console.WriteLine("Out of Observe!");
}

private void ListenForChanges()
{
 Console.WriteLine("Listener thread active");
 BoilerStatus eLastBoilerStatus = m_obDevice.GetBoilerStatus();
 WarmerPlateStatus eLastPlateStatus = m_obDevice.GetWarmerPlateStatus();
 Console.WriteLine("First Status Read");
 while ( !m_bStopWatching )
 {
 …

Now when it fails.. the log is
Start Listener thread
Out of Observe!
Listener thread active
Change to WE
First Status Read
Check WARMER_EMPTY with WARMER_EMPTY
Check WARMER_EMPTY with WARMER_EMPTY
Check WARMER_EMPTY with WARMER_EMPTY
Check WARMER_EMPTY with WARMER_EMPTY

Aha! The thread has started but just before it could initialize the LastXXX status variables, there is a thread switch and the value is changed to WE. If we promote the LastXXX variables to volatile local variables and ensure that they are always initialized before the listener thread is started.. hmm. lets see if this works…
Hallelujah! Console.WriteLines rock()


private volatile BoilerStatus m_eLastBoilerStatus;
private volatile WarmerPlateStatus m_eLastPlateStatus;

public void Observe(CoffeeMakerAPI obDevice)
{
 m_obDevice = obDevice;
 m_eLastBoilerStatus = m_obDevice.GetBoilerStatus();
 m_eLastPlateStatus = m_obDevice.GetWarmerPlateStatus();
 Console.WriteLine("First Status Read");
 obListenerThread = new Thread(new ThreadStart(ListenForChanges));
 obListenerThread.Start();

 Console.WriteLine("Start Listener thread ");
 Console.WriteLine("Out of Observe!");
}

private void ListenForChanges()
{
 Console.WriteLine("Listener thread active");
 while ( !m_bStopWatching )
 {
 …

Now the logs show.
First Status Read
Start Listener thread
Out of Observe!
Listener thread active
Check POT_EMPTY with POT_EMPTY
Change to WE
Check WARMER_EMPTY with POT_EMPTY
WARMER_EMPTY <= POT_EMPTY

Turn the crank another 20 times.. All green and good. I’ll remove all the logging lines.. Goodbye friends!
More duplication I see that we have repeating occurrences of
m_obDevice.Property = value
Thread.Sleep(I_WAIT_FOR_EVENTS_TO_FIRE);
How about we move the delay into the FakeCoffeeMaker property.. then we don’t need to define the constant in each test fixture.


public class FakeCoffeeMakerDevice : CoffeeMakerAPI
{
 private const int I_WAIT_FOR_EVENTS_TO_FIRE = 300;

 public WarmerPlateStatus WarmerPlateStatus
 {
     get { return m_eWarmerPlateStatus; }
     set
     {
         m_eWarmerPlateStatus = value;
         Thread.Sleep(I_WAIT_FOR_EVENTS_TO_FIRE);
     }
 }

Look how pretty my tests are now.. Act-Arrange-Assert appears again to knight ‘em as SirGoodTest.


[Test]
public void Test_ValveIsOpen_IfPotWithCoffeeIsRemovedFromThePlate()
{
 m_obDevice.WarmerPlateStatus = WarmerPlateStatus.POT_NOT_EMPTY;
 Assert.AreEqual(ReliefValveState.CLOSED, m_obDevice.ReliefValveState,
     "Relief valve should be closed when Pot With Coffee is on the plate");

 m_obDevice.WarmerPlateStatus = WarmerPlateStatus.WARMER_EMPTY;

 Assert.AreEqual(ReliefValveState.OPEN, m_obDevice.ReliefValveState,
     "Relief valve should be open since the warmer plate is now empty");
}

Watch in awe ... haphazard refactoring ninja on steroids
Repeat the procedure for TestSensorObserver fixture.. add delays to PressBrewButton() and SetBoilerStatus(). Cool Green!
Exposed! The Missing Dispose! Hey we’re missing SensorObserver.Dispose() in the third test. I should have mistake-proofed that by adding a Teardown method that does that.. never too late.

Which brings me to… I’m not doing SensorObserver.Dispose() in our TestCoffeeMaker tests. Lets add a Dispose to CoffeeMaker that delegates the call and call it from Teardown. Green!
I’ll just add two tests to complete the set.