TDD can make fake dependencies

Question

I am using the template implementation of Model-View-Presenter in an ASP.NET WebForms application. In my view, there are two events that signal that the user has filled in enough fields on the domain model to trigger a duplication check, and the other is the usual Save event. My pseudo code is as follows:

public class ItemNewPresenter : PresenterBase<IItemNewView> { public IItemService Service { get; private set; } public IItemNewView View { get; private set; } public ItemNewPresenter(IItemService service, IItemNewView view) { Service = service; View = view; View.OnSave += DoItemSave; View.OnItemIsDuplicateCheck+= DoItemIsDuplicateCheck; } private void DoItemIsDuplicateCheck(object sender, CheckItemDuplicateEventArgs e) { CheckForItemDuplication(e.Item); } private void CheckForItemDuplication(Item item){ if (Service.IsDuplicateItem(item)) { View.RedirectWithNotification(BuildItemUrl(item), "This item already exists"); } } private void DoItemSave(object sender, SaveItemEventArgs e) { DoItemIsDuplicateCheck(this, e.ToItemDuplicateEventArgs()); Service.Save(e.Item); } } 

Here is my test to ensure that my host behaves correctly when OnItemIsDuplicateCheck is raised from a view:

 [Test] public void presenter_checking_for_existing_item_should_call_redirect_if_found() { var service = new Mock<IItemService>(); var view = new Mock<IItemNewView>(); var presenter = new ItemNewPresenter (service.Object, view.Object); var onCheckExistingHandler = view.CreateEventHandler <CheckItemDuplicateEventArgs>(); view.Object.OnExistingDenominatorCheck += onCheckExistingHandler; var eventArgs = new CheckItemDuplicateEventArgs(); service.Setup(s => s.IsDuplicate(It.Is<CheckItemDuplicateEventArgs>(c => c.Equals(eventArgs)))).Returns(true); onCheckExistingHandler.Raise(eventArgs); view.Verify(v => v.RedirectWithNotification(It.IsAny<String>(), It.IsAny<string>()), Times.Once()); service.Verify(); } 

To ensure consistency, I would like to have the same double check when View raises the OnSave . My question is related to how I should write my test when one of the methods I want to test ( CheckForItemDuplication ) is declared in the tested class. An alternative to checking the method call for SUT (bad) would be to write my save test with a lot of duplicated code (the installation and approval of all my poppies will be copied from the above test), and also makes the unit test less focused.

  [Test] public void presenter_saving_item_should_check_for_dupe_and_save_if_not_one() { //duplicate mocks/setups/asserts from duplicate check fixture //additional mocks/setups/asserts to test save logic } 

I think that TDD will offer to bring this private method into a separate class, which will cooperate with my Master and will be introduced through DI. But adding another dependency to my Presenter for functionality that doesn't seem worthy of an independent abstraction * and * is an internal part of my Lead's implementation, it seems ... well ... crazy. Am I here from here? There must be some kind of design template or refactoring that I can apply to avoid the need to turn a private method into a dependency.

Answer 1

I would go with class testing by adding duplicate installation code. Once this test passes, and you are sure that all test cases are covered, you can then reorganize your test code to remove duplication.

You can move the dependencies (service and view) to private fields, and then add a method to create the SUT:

 private Mock<IItemService> _service; private Mock<IItemNewView> _view; private PresenterBase<IItemNewView> CreateSUT() { _service = new Mock<IItemService>(); _view = new Mock<IItemNewView>(); return new ItemNewPresenter (service.Object, view.Object); } 

(I think most people would prefer to initialize Mock objects in the installation method.)

Call CreateSUT from your tests, and now there is a bit less duplication. Then you might want to add a private method to create an event handler / create an event if it is something that is doing the same or similar in more than one case.

Having this CreateSUT method reduces the amount of test code that calls your constructor, which will simplify it in the future if you want to add / remove / change dependencies. If you process your test code like any other code and use the DRY principle when you see duplication, this can lead to a more explicit, easy to read, supported test code. Working with very similar settings and test contexts is a common problem with unit testing and does not always have to change the way the class is tested / tested.

Answer 2

I think you have fallen into the never-ending dispute between TDD and information hiding, because you agree that an injection is probably the right thing (and probably it is), but also believe that external interaction should not seem to worry minor injection.

Please do not let me dizzy for what I'm going to say :-)

Now what I did sometimes when I came across this dilemma is to extract a function, create an internal constructor with an object as an argument and without an open constructor. The public ctor is redirected to the internal one with a new object, such as:

 public class ClassThatUseInjection { private readonly SomeClass _injectedClass; public ClassThatUseInjection(): this(new SomeClass()) {} internal ClassThatUseInjection(SomeClass injectedClass) { _injectedClass = injectedClass; } } public class SomeClass { public object SomeProperty { get; set; } } 

This way you can use an empty constructor from the outside, and another constructor so that you insert an encoded argument for testpurposes. While an empty constructor only forwards the call without any logic, you can still test it, for example, it has only one constructor.

Just a little smelly, yes, but not smelly-smelly :-) Or what do you think?

Regards, Morten

Answer 3

I will be wondering if there are better answers, since I always come across this.

An alternative to checking the method call for SUT (bad) would be to write my save test with a lot of duplicated code (the setup and approval of all my poppies will be copied from the above test), and also makes the unit test less focused.

I’m not sure why you feel this makes the test less focused, but in your shoes I would do exactly what sounds like you don’t want to - duplicate the setup code to check individual cases for SUT, you test the external behavior SUT with the test you provided, which seems to me completely correct.

I personally am not a fan of disclosing more than is necessary from the class, and / or creating behavior that SUT should be responsible for addiction to facilitate testing. The "natural boundary" of class responsibility should not be violated just because you want to test it.

Answer 4

It is easier to do a single check for calculating the URL than for unit testing that a redirect occurred.

If I understood you correctly, you want to check that s-mvp CheckForItemDuplication() redirects to a specific URL-address, picking up view-mock-s event OnItemIsDuplicateCheck .

 private void CheckForItemDuplication(Item item) { if (Service.IsDuplicateItem(item)) { View.RedirectWithNotification(BuildItemUrl(item), "This item already exists"); } } 

In my opinion, you do a lot. What if you rewrite your code as

 internal protected GetErrorUrlForItem(Item item) { if (Service.IsDuplicateItem(item)) { return BuildItemUrl(item, "This item already exists"); } return null; } private void CheckForItemDuplication(Item item) { var result = GetErrorUrlForItem(item); if (result != null) { View.RedirectWithNotification(result); } } 

In unittest, just check the internal GetErrorUrlForItem() method. You must use the InternalsVisibleTo attribute to allow access to the internal method.