7

I've neglected unit testing for some time. I wrote unit tests, but they were fairly poor. I'm now reading through "The art of unit Testing" to bring myself up to scratch.

If I have an interface such as:

public interface INotificationService
{
    void AddError(string _error);
    void AddIssue(string _issue);

    IEnumerable<string> FetchErrors();
    IEnumerable<string> FetchIssues();
}

A concrete implementation of this interface contains:

    private readonly ICollection<Message> messages;

Adding an error or issue creates a new message with an enum denoting it's type and adds it to the collection. Calling FetchErrors() / FetchIssues() returns messages of that type from the collection.

Would the following test be valid?:

    [Test]
    public void FetchErrors_LoggingEnabledAddErrorFetchErrors_ReturnsError()
    {
        notificationService = new NotificationService();
        notificationService.AddError("A new error");

        Assert.AreEqual(new []{"A new error"}, notificationService.FetchErrors());
    }

My concern is that I'm first calling AddError(), then testing the result of FetchErrors(). So I'm calling two functions. Is this incorrect?

Should I make the collection public and directly assert that it contains a message of the appropriate type containing the logged error message?

What would be the best practice in this scenario?

Improve this question

4 Answers 4

Reset to default
3

Your approach looks OK - testing the implementation through the public methods is the only access made available in the design.

In order to test the methods independently of one another, you would need to use a white-box testing backdoor like reflection to access the private messages to ensure that the respective Add* methods work. This isn't a good idea at all, since your tests will break (at run time) every time the underlying CUT implementation changes.

A common, better alternative to reflection is to expose a non-interface internal method on the concrete class under test which allowed you some kind of 'maintenance hatch' access to the ICollection<Message> messages; implementation field, and then allow InternalsVisibleTo access to the Unit test assembly.

One note - you'll find that Assert.AreEqual on an the string enumerable will do a reference comparison and thus fail (because you are comparing it to a new array).

You'll probably have to change this to something like:

Assert.IsTrue(notificationService.FetchErrors().Contains("A new error"));

Edit

IMO you don't need to go as far as making the field public in .Net. Since you have the interface abstraction in place already which should in theory restrict the means of access to the CUT, it is quite common to use internal scope and InternalsVisibleTo to allow the UT special access. Jon Skeet et al have mentioned this here

#region Unit Testing side-door
internal ICollection<Message> Messages
{
   get { return _messages };
   // No Setter
}
#endregion
Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Hi StuartLC, thanks for the reply. The reason for asking is that in Roy Osherove's book, he suggests doing things like instantiating a new instance of a class under test, but making the private members public as to make testing of single pieces of functionality easier. I wasn't sure how far to take this in regards to the example in my OP.
2

Your approach is perfectly ok. A unit test should follow the Arrange-Act-Assert structure - and your call to notificationService.AddError(..) simply is part of the test's Arrange section.

It's not that you cannot call more than one method in a test (in many cases you will have to), the point is that you should only assert/test for one single fact. That's what you do, so everything looks good.

Improve this answer

2 Comments

Hi Thomas, It seemed ok to me, but then I thought: what happens if there's a bug in the AddError function and/or FetchErrors function - by calling both and having the test fail, it wouldn't be obvious exactly where bug was. I guess it's about into what detail should one go before considering the test 'complete'.
Well, strictly spoken you're right. To be absolutely proper on that you would have to assert directly against the messages field and also write a second test that checks the FetchErrors() method. To do that, you would have to make the field accessible, either by making it public (ugly,bad design) or by making it 'internal` and allow your test assembly access to it via the InternalsVisib≤To attribute.
1

As others pointed out, it's OK to use both methods. You are testing the behavior of the class, and it is that whenever an error is published, you can read it.

If the implementation is more complicated, then probably you may need to rethink the design, as your method(s) do too much. Looking at what you describe, why actually you need the getter to be a method?

In general, I do not like the idea of exposing internal method only for testing purposes. This send a wrong message to whoever is going to maintain that code in the future (even you, after you forget). It says - yeah, go ahead, access this collection directly from any class in this assembly.

Improve this answer

Comments

1

We can't say if your test is valid or not based on the code you posted. We don't know what AddError() does so is not possible to know if the unit test is correct.

If AddError just add an error into the collection and do anything else then your test is good and call FetchErrors() is valid because it has to have a separated unit test which verifies it. In any case if your AddError method does anything else you have to change the test and verify all the steps that method does.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.