Can these unit tests be improved?

Question

I'm not the greatest at writing tests yet and I'm starting a new OSS project for learning and as part of it I want to tackle being more effective at writing tests, more specifically quality tests. I think I write a lot of tests that don't bring much value except maintenance pain sometimes.

Can I improve these tests?

using System;
using System.Text;
using System.Collections.Generic;
using System.Linq;
using FakeItEasy;
using HaywireMQ.Server.Channel;
using HaywireMQ.Server.MessageStore;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Ploeh.AutoFixture;
using Ploeh.AutoFixture.AutoFakeItEasy;

namespace HaywireMQ.Server.Tests
{
    /// <summary>
    /// Tests for HaywireServer
    /// </summary>
    [TestClass]
    public class HaywireServerTests
    {
        private IFixture fixture;

        public HaywireServerTests()
        {
        }

        [TestInitialize]
        public void Initialize()
        {
            fixture = new Fixture().Customize(new AutoFakeItEasyCustomization());
        }

        [TestMethod]
        public void Should_use_defaults_without_ModuleCatalog()
        {
            // Given
            var target = new HaywireServer();

            // When
            target.Start();

            // Then
            Assert.AreEqual<Type>(target.MessageStore.GetType(), typeof(InMemoryMessageStore));
            Assert.AreEqual<Type>(target.MessageChannel.GetType(), typeof(InMemoryMessageChannel));
        }

        [TestMethod]
        public void Should_use_ModuleCatalog()
        {
            // Given
            var catalog = new ModuleCatalog();
            var messageStore = fixture.CreateAnonymous<IMessageStore>();
            var messageChannel = fixture.CreateAnonymous<IMessageChannel>();
            catalog.MessageStores.Add(messageStore);
            catalog.MessageChannels.Add(messageChannel);
            var target = new HaywireServer(catalog);

            // When
            target.Start();

            // Then
            Assert.AreEqual<Type>(target.MessageStore.GetType(), messageStore.GetType());
            Assert.AreEqual<Type>(target.MessageChannel.GetType(), messageChannel.GetType());
        }

        [TestMethod]
        public void Should_create_MessageQueue()
        {
            // Given
            var catalog = new ModuleCatalog();
            var messageStore = fixture.CreateAnonymous<IMessageStore>();
            var messageChannel = fixture.CreateAnonymous<IMessageChannel>();
            catalog.MessageStores.Add(messageStore);
            catalog.MessageChannels.Add(messageChannel);
            var target = new HaywireServer(catalog);

            List<string> ids = new List<string>() {"test"};

            A.CallTo(() => messageStore.GetQueues()).Returns(ids);

            // When
            target.Start();

            // Then
            A.CallTo(() => messageStore.GetQueues()).MustHaveHappened();
            Assert.AreEqual<int>(target.MessageQueues.Count, 1);
            Assert.AreEqual<string>(target.MessageQueues[0].Id, "test");
        }
    }
}

Answer 1

Assuming that I've correctly managed to extrapolate the SUT and friends from the question, I'd reduce the tests to the following. Please note that this focuses only on AutoFixture mechanics, and not on the general design of neither test nor SUT API.

AFAICT, the following tests state the same as the tests in the OP, but reduced to only the necessary statements. Still, I agree with Nikos Baxevanis that it would be possible to reduce these tests dramatically with xUnit.net instead of MSTest.

using System.Collections.Generic;
using System.Linq;
using FakeItEasy;
using FirstAutoFixtureReviewForKellySommers;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Ploeh.AutoFixture;
using Ploeh.AutoFixture.AutoFakeItEasy;
using Ploeh.AutoFixture.Kernel;

namespace HaywireMQ.Server.Tests
{
    /// <summary>
    /// Tests for HaywireServer
    /// </summary>
    [TestClass]
    public class HaywireServerTests
    {
        [TestMethod]
        public void Should_use_defaults_without_ModuleCatalog()
        {
            // Given
            var fixture = new Fixture().Customize(new TestConventions());

            // When
            var target = fixture.CreateAnonymous<HaywireServer>();

            // Then
            Assert.IsInstanceOfType(target.MessageStore, typeof(InMemoryMessageStore));
            Assert.IsInstanceOfType(target.MessageChannel, typeof(InMemoryMessageChannel));
        }

        [TestMethod]
        public void Should_use_ModuleCatalog()
        {
            // Given
            var fixture = new Fixture().Customize(new TestConventions());

            var catalog = fixture.Freeze<ModuleCatalog>();
            fixture.AddManyTo(catalog.MessageStores, 1);
            fixture.AddManyTo(catalog.MessageChannels, 1);

            // When
            var target = fixture.CreateAnonymous<HaywireServer>();

            // Then
            Assert.AreEqual(catalog.MessageStores.Single(), target.MessageStore);
            Assert.AreEqual(catalog.MessageChannels.Single(), target.MessageChannel);
        }

        [TestMethod]
        public void Should_create_MessageQueue()
        {
            // Given
            var fixture = new Fixture().Customize(new TestConventions());

            var catalog = fixture.Freeze<ModuleCatalog>();
            fixture.AddManyTo(catalog.MessageStores, 1);

            List<string> ids = fixture.CreateMany<string>(1).ToList();
            A.CallTo(() => catalog.MessageStores.Single().GetQueues()).Returns(ids);

            var target = fixture.CreateAnonymous<HaywireServer>();

            // When
            target.Start();

            // Then
            A.CallTo(() => catalog.MessageStores.Single().GetQueues()).MustHaveHappened();
            Assert.AreEqual(1, target.MessageQueues.Count);
            Assert.AreEqual(ids.First(), target.MessageQueues[0].Id);
        }

        private class TestConventions : CompositeCustomization
        {
            public TestConventions()
                : base(
                    new AutoFakeItEasyCustomization(),
                    new GreedyHaywireServerCustomization())
            {
            }
        }

        private class GreedyHaywireServerCustomization : ICustomization
        {
            public void Customize(IFixture fixture)
            {
                fixture.Customize<HaywireServer>(c =>
                    c.FromFactory(new MethodInvoker(new GreedyConstructorQuery())));
            }
        }
    }
}

Answer 2

I think it's quite well written, would have only two ideas.

1. For the repeating part of the fixture setup (given) I'd consider using a Standard Fixture, probably via a setup helper method.

2. I'd consider splitting the state and behaviour verification parts of Should_create_MessageQueue() into two separate test methods. The behaviour verification of MustHaveHappened() deals with a different issue than the state verification with the asserts.

Answer 3

A couple of points that I can see:

In Should_use_ModuleCatalog,

• I see that the test is around the HaywireServer since the assert is after the action on target. However, the asserts are on properties of ModuleCatalog. I would prefer this test to be a catalog's test rather than a target's test.
• What does this testcase ensure? It tells me that the properties are set with the appropriate types. I wouldn't test this out explicitly unless there are branching workflows that could set different types based on context.
• Catalog seems to have a List of MessageStore, however when catalog is injected into the target, the target seems to have just one MessageStore - Is there a logic around this? If so I would test that out.

Something that I noted (not related to the testing aspect): this line:

catalog.MessageStores.Add(messageStore);

tells me that Catalog has a List (or a similar collection) - which isn't necessarily something to be published as a contract to consumers. I would prefer something like:

catalog.AddMessageStore(messageStore)

so that I am free to refactor ModuleCatalog to use any internal mechanism of holding this.