patterncsharpMinor
Unit test if an event has been raised or not
Viewed 0 times
raisedbeenhastestnotunitevent
Problem
Given the following event dispatcher code
```
public interface IApplicationEvent { }
public delegate void ApplicationEventHandlerDelegate(TEvent @event) where TEvent : IApplicationEvent;
public class ApplicationEventDispatcher
{
private bool _disposed;
private Dictionary _applicationEventHandlers;
public ApplicationEventDispatcher()
{
_applicationEventHandlers = new Dictionary();
}
~ApplicationEventDispatcher()
{
Dispose(false);
}
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
private void Dispose(bool disposing)
{
if (_disposed) return;
if (disposing)
{
// free other managed objects that implement IDisposable only
RemoveAllListeners();
}
// release any unmanaged objects
// set the object references to null
_applicationEventHandlers = null;
_disposed = true;
}
public void AddListener(ApplicationEventHandlerDelegate handler)
where TEvent : IApplicationEvent
{
Delegate @delegate;
if (_applicationEventHandlers.TryGetValue(typeof(TEvent), out @delegate))
{
_applicationEventHandlers[typeof(TEvent)] = Delegate.Combine(@delegate, handler);
}
else
{
_applicationEventHandlers[typeof(TEvent)] = handler;
}
}
public void RemoveListener(ApplicationEventHandlerDelegate handler)
where TEvent : IApplicationEvent
{
Delegate @delegate;
if (_applicationEventHandlers.TryGetValue(typeof(TEvent), out @delegate))
{
Delegate currentDel = Delegate.Remove(@delegate, handler);
if (currentDel == null)
{
_applicationEventHandlers.Remove(typeof(TEvent));
}
else
{
_applicationEventHandlers[typeof(TEvent)] = currentDel;
}
}
}
```
public interface IApplicationEvent { }
public delegate void ApplicationEventHandlerDelegate(TEvent @event) where TEvent : IApplicationEvent;
public class ApplicationEventDispatcher
{
private bool _disposed;
private Dictionary _applicationEventHandlers;
public ApplicationEventDispatcher()
{
_applicationEventHandlers = new Dictionary();
}
~ApplicationEventDispatcher()
{
Dispose(false);
}
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
private void Dispose(bool disposing)
{
if (_disposed) return;
if (disposing)
{
// free other managed objects that implement IDisposable only
RemoveAllListeners();
}
// release any unmanaged objects
// set the object references to null
_applicationEventHandlers = null;
_disposed = true;
}
public void AddListener(ApplicationEventHandlerDelegate handler)
where TEvent : IApplicationEvent
{
Delegate @delegate;
if (_applicationEventHandlers.TryGetValue(typeof(TEvent), out @delegate))
{
_applicationEventHandlers[typeof(TEvent)] = Delegate.Combine(@delegate, handler);
}
else
{
_applicationEventHandlers[typeof(TEvent)] = handler;
}
}
public void RemoveListener(ApplicationEventHandlerDelegate handler)
where TEvent : IApplicationEvent
{
Delegate @delegate;
if (_applicationEventHandlers.TryGetValue(typeof(TEvent), out @delegate))
{
Delegate currentDel = Delegate.Remove(@delegate, handler);
if (currentDel == null)
{
_applicationEventHandlers.Remove(typeof(TEvent));
}
else
{
_applicationEventHandlers[typeof(TEvent)] = currentDel;
}
}
}
Solution
Test naming
Given/when/then and arrange/act/assert are redundant and both are mostly noise. One would put comments like
Also method names are misleading. Most obvious one is whereas the method name says
Instead the method name should say
Similarly
Instead the method name should say
I like
My suggestion is below, I omitted some of the noise, but note the noisy parts are separated and pushed further down:
Basic idea is you see this, if you look at he public declarations, e.g. in the class outline panels:
Other points if your test case says
Is the application event dispatch disposable or not? If you are implementing a
After you do this you can eliminate the
Given/when/then and arrange/act/assert are redundant and both are mostly noise. One would put comments like
// ARRANGE etc in a test hundreds of lines long, usually legacy; as a reading help, before refactoring it.Also method names are misleading. Most obvious one is whereas the method name says
WhenAHandlerIsAttached the relevant snippet says: // ACT
dispatcher.Dispatch(new SimpleEvent1());
dispatcher.Dispose();Instead the method name should say
when_an_event_is_dispatched.Similarly
GivenAnEventToDispatch is also misleading:// ARRANGE
... @delegate = ...;
var dispatcher = ...
dispatcher.AddListener(@delegate);Instead the method name should say
given_an_event_dispatcher_and_an_event_handler_attached_to_itI like
CallsHandler and DoesNotCallHandler better, as you'll see further down; but in the name of consistency, given you have used given/when in the first part of the name, assertion part of the name should say then_the_handler_is_called then_the_handler_is_not_called.My suggestion is below, I omitted some of the noise, but note the noisy parts are separated and pushed further down:
public class A_dispatcher {
public void calls_a_handler_after_it_is_attached()
{
using(var dispatcher = aDispatcher())
{
dispatcher.AddListener(recordCall);
dispatcher.Dispatch(anEvent());
}
Assert.IsTrue(callRecorder.IsCalled());
}
public void does_not_call_a_handler_after_it_is_detached()
{
using(var dispatcher = aDispatcher())
{
dispatcher.AddListener(recordCall);
dispatcher.RemoveListener(recordCall);
dispatcher.Dispatch(anEvent());
}
Assert.IsFalse(callRecorder.IsCalled());
}
private aDispatcher() {
return new ApplicationEventDispatcher();
}
class CallRecorder
{
private bool called = false;
public bool IsCalled() {return called;}
public void Call()
{
this.Called = true;
}
}
... callRecorder = new CallRecorder();
... recordCall = callRecorder.Call;
}Basic idea is you see this, if you look at he public declarations, e.g. in the class outline panels:
A_dispatcher
calls_a_handler_after_it_is_attached
does_not_call_a_handler_after_it_is_detachedOther points if your test case says
AHandler your local variable name should not be @delegate.Is the application event dispatch disposable or not? If you are implementing a
Dispose method make the class implement IDisposable, or rename the method, if the class does not follow IDisposable contract.After you do this you can eliminate the
dispathcer.Dispose noise by using using. Or test set-up and clean-up methods.Code Snippets
// ACT
dispatcher.Dispatch(new SimpleEvent1());
dispatcher.Dispose();// ARRANGE
... @delegate = ...;
var dispatcher = ...
dispatcher.AddListener(@delegate);public class A_dispatcher {
public void calls_a_handler_after_it_is_attached()
{
using(var dispatcher = aDispatcher())
{
dispatcher.AddListener(recordCall);
dispatcher.Dispatch(anEvent());
}
Assert.IsTrue(callRecorder.IsCalled());
}
public void does_not_call_a_handler_after_it_is_detached()
{
using(var dispatcher = aDispatcher())
{
dispatcher.AddListener(recordCall);
dispatcher.RemoveListener(recordCall);
dispatcher.Dispatch(anEvent());
}
Assert.IsFalse(callRecorder.IsCalled());
}
private aDispatcher() {
return new ApplicationEventDispatcher();
}
class CallRecorder
{
private bool called = false;
public bool IsCalled() {return called;}
public void Call()
{
this.Called = true;
}
}
... callRecorder = new CallRecorder();
... recordCall = callRecorder.Call;
}A_dispatcher
calls_a_handler_after_it_is_attached
does_not_call_a_handler_after_it_is_detachedContext
StackExchange Code Review Q#119689, answer score: 2
Revisions (0)
No revisions yet.