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"); } } }

One improvement is to use xUnit.net instead of MSTest which integrates with AutoFixture and allows you to parameterize those tests via data theories. Once you have parameterized tests, AutoFixture can take care of the rest supplying the parameter values for you and even Auto-Mock them using FakeItEasy. :) — Nikos Baxevanis
– Nikos Baxevanis, Commented Feb 1, 2012 at 6:18
I would not use MSTest. It's slower than the alternatives and has fewer features. — Adam Dymitruk
– Adam Dymitruk, Commented Feb 1, 2012 at 6:52
You're probably better at testing than me, but shouldn't there really only be one Assert per test method? — John Ferguson
– John Ferguson, Commented Feb 1, 2012 at 10:07
Each test should test only one "thing". So having a couple of Asserts in a test is actually fine. For example, it_should_return_empty_string() would be perfectly fine to have Assert.IsNotNull(result) and Assert.Equals(String.Empty, result). — Tacoman667
– Tacoman667, Commented Feb 1, 2012 at 13:12

Mark Seemann · Accepted Answer · 2012-02-01 21:18:01Z

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()))); } } } }

Curious why you removed the TestInitialize method in favor of setting up the fixture in each test? I'm generally also of the opinion that too much DRY-aware refactoring makes tests less expressive, but would have thought that something like this that is purely infrastructural is a good candidate for moving out of the way of the actual meat of the test case (particularly since you aren't really increasing the information locality, since the important part is in TestConventions anyways). — Matt Enright
– Matt Enright, Commented Feb 1, 2012 at 21:27
I have an (irrational or subjective) dislike of Implicit Setup because I feel that the lack of proximity between a [TestInitialize] method and the last test cases in a (larger) test file makes it too hard to understand what's going on at a glance. I prefer each test case to be self-contained. — Mark Seemann
– Mark Seemann, Commented Feb 1, 2012 at 21:39
@MarkSeemann I find that harder to read because there's more AutoFixture than what I'd call regular use of the code involved. To someone new to using AutoFixture I'm not sure what Freeze() or GreedyConstructorQuery does. What does CreateAnonymous() do? I thought it created the fake but now it looks to be the concrete type. SUT was meant to be what I named "target" just to clarify. Also was adding to the catalog left out on purpose? Reason I ask is in Start() exceptions occur if no MessageChannel or MessageStore is detected in the catalog. These questions probably due to misunderstanding :) — Kelly Sommers
– Kelly Sommers, Commented Feb 2, 2012 at 4:18
AutoFixture is a DSL. Tests will be harder to read until you understand the language - as with all other DSLs. — Mark Seemann
– Mark Seemann, Commented Feb 2, 2012 at 5:53
Yes, or better yet: improve the design in order to get rid of the Temporal Coupling. — Mark Seemann
– Mark Seemann, Commented Feb 2, 2012 at 16:26

Jamal · Accepted Answer · 2014-05-04 18:07:22Z

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

For the repeating part of the fixture setup (given) I'd consider using a Standard Fixture, probably via a setup helper method.
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.

Srikanth Venugopalan · Accepted Answer · 2012-02-02 06:23:33Z

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.

Stack Exchange Network

Can these unit tests be improved?

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Can these unit tests be improved?

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions