6

I am abstracting the history tracking portion of a class of mine so that it looks like this:

private readonly Stack<MyObject> _pastHistory = new Stack<MyObject>();

internal virtual Boolean IsAnyHistory { get { return _pastHistory.Any(); } }

internal virtual void AddObjectToHistory(MyObject myObject)
{
  if (myObject == null) throw new ArgumentNullException("myObject");
  _pastHistory.Push(myObject);
}

internal virtual MyObject RemoveLastObject()
{
  if(!IsAnyHistory) throw new InvalidOperationException("There is no previous history.");
  return _pastHistory.Pop();
}

My problem is that I would like to unit test that Remove will return the last Added object.

  • AddObjectToHistory
  • RemoveObjectToHistory -> returns what was put in via AddObjectToHistory

However, it isn't really a unit test if I have to call Add first? But, the only way that I can see to do this in a true unit test way is to pass in the Stack object in the constructor OR mock out IsAnyHistory...but mocking my SUT is odd also. So, my question is, from a dogmatic view is this a unit test? If not, how do I clean it up...is constructor injection my only way? It just seems like a stretch to have to pass in a simple object? Is it ok to push even this simple object out to be injected?

Justin Pihony
  • 66,056
  • 18
  • 147
  • 180

5 Answers5

4

There are two approaches to those scenarios:

  1. Interfere into design, like making _pastHistory internal/protected or injecting stack
  2. Use other (possibly unit tested) methods to perform verification

As always, there is no golden rule, although I'd say you generally should avoid situations where unit tests force design changes (as those changes will most likely introduce ambiguity/unnecessary questions to code consumers).

Nonetheless, in the end it is you who has to weigh how much you want unit test code interfere into design (first case) or bend the perfect unit test definition (second case).

Usually, I find second case much more appealing - it doesn't clutter original class code and you'll most likely have Add already tested - it's safe to rely on it.

Community
  • 1
  • 1
k.m
  • 30,794
  • 10
  • 62
  • 86
  • I am going to leave this question open for a day or so, but I was leaning towards that since Add is under test, and it is not relying on external state. I just was leery of fragile tests if add changes in some capacity...but I guess the documentation of Remove is something along the lines of: returns what was last added....so it almost requires add. Thanks! – Justin Pihony Jun 24 '13 at 19:49
1

I think it's still a unit test, assuming MyObject is a simple object. I often construct input parameters to unit test methods.

I use Michael Feather's unit test criteria:

A test is not a unit test if:

  • It talks to the database
  • It communicates across the network
  • It touches the file system
  • It can't run at the same time as any of your other unit tests
  • You have to do special things to your environment (such as editing config files) to run it.

Tests that do these things aren't bad. Often they are worth writing, and they can be written in a unit test harness. However, it is important to be able to separate them from true unit tests so that we can keep a set of tests that we can run fast whenever we make our changes.

Community
  • 1
  • 1
neontapir
  • 4,698
  • 3
  • 37
  • 52
  • Hrmmm, but my problem is that I have to rely on other units of work (`AddObjectToHistory`) in order to test my actual unit of work. So, it feels to me to be more of a unitS test. If add ever changes, then my remove test fails, so it makes the test fragile against another unit...I can see both sides really...just looking for the overall consensus – Justin Pihony Jun 24 '13 at 19:29
  • That wasn't clear from your question. I think you'd want to be able to inject a `Stack` representing the initial state you're testing, which still falls under these guidelines. – neontapir Jun 24 '13 at 19:35
  • Updated my question to clarify – Justin Pihony Jun 24 '13 at 19:39
1

My 2 cents... how would the client know if remove worked or not ? How is a 'client' supposed to interact with this object? Are clients going to push in a stack to the history tracker? Treat the test as just another user/consumer/client of the test subject.. using exactly the same interaction as in real production. I haven't heard of any rule stating that you're not allowed to call multiple methods on the object under test.

To simulate, stack is not empty. I'd just call Add - 99% case. I'd refrain from destroying the encapsulation of that object.. Treat objects like people (I think I read that in Object Thinking). Tell them to do stuff.. don't break-in and enter.

e.g. If you want someone to have some money in their wallet,

  • the simple way is to give them the money and let them internally put it into their wallet.
  • throw their wallet away and stuff in a wallet in their pocket.

I like Option1. Also see how it frees you from implementation details (which induce brittleness in tests). Let's say tomorrow the person decides to use an online wallet. The latter approach will break your tests - they will need to be updated for pushing in an online wallet now - even though the object behavior is not broken.

Another example I've seen is for testing Repository.GetX() where people break-in to the DB to inject records with SQL now in the unit test.. where it would have be considerably cleaner and easier to call Repository.AddX(x) first. Isolation is desired but not to the extent that it overrides pragmatism.

I hope I didn't come on too strong here.. it just pains me to see object APIs being 'contorted for testability' to the point where it no longer resembles the 'simplest thing that could work'.

Gishu
  • 134,492
  • 47
  • 225
  • 308
  • I agree with you, and if I went to injecting, I might not pass in the stack, but instead pass in a type of repository...which still seemed overkill. That being said, this is pretty much the same as jimmy's answer – Justin Pihony Jun 25 '13 at 13:47
  • For the most part, except that the tests should exactly mimic the real consumer(s). They should *not* test via a path that eases testing but is different from the app in production mode. – Gishu Jun 25 '13 at 15:13
0

I think you're trying to be a little overly specific with your definition of a unit test. You should be testing the public behavior of your class, not the minute implementation details.

From your code snippet, it looks like all you really need to care about is whether a) calling AddObjectToHistory causes IsAnyHistory to return true and b) RemoveLastObject eventually causes IsAnyHistory to return false.

rossisdead
  • 2,102
  • 1
  • 19
  • 30
  • I was thinking about being over specific. However, I don't really believe that I am because I want to test that `RemoveLastObject` returns that last object. That does not seem like an overly specific test. It is testing the output is correct – Justin Pihony Jun 24 '13 at 19:43
  • Sorry, missed the part where RemoveLastObject returns the object. You should test that exactly as it's expected to be used. If the only way to get an object into the history is by calling AddObjectToHistory, then you should call that in your test. There's no need to add constructor injection unless you want to allow that in the first place(and then having to unit test all that too) – rossisdead Jun 24 '13 at 19:50
0

As stated in the other answers I think your options can be broken down like so.

  1. You take a dogmatic approach to your testing methodology and add constructor injection for the stack object so you can inject your own fake stack object and test your methods.

  2. You write a separate test for add and remove, the remove test will use the add method but consider it a part of the test setup. As long as your add test passes, your remove should be too.

Eogcloud
  • 1,335
  • 4
  • 19
  • 44