Give Me An Assertion Vasily. One Assertion Only, Please

I was working through some broken unit tests this morning for the project I’m currently on and the second one in my list looked like this:

 [TestMethod]
        public void SelectTest()
        {
            Item item = session.SelectSingle(a => a.SkuNo == _item.SkuNo);

            Assert.IsTrue(item.Id > 0);
            Assert.AreEqual(item.Id, _item.Id);
            Assert.AreEqual(item.IsAutoReplenished, _item.IsAutoReplenished);
            Assert.AreEqual(item.Status, _item.Status);
            Assert.AreEqual(item.MaintenanceLevel, _item.MaintenanceLevel);
            Assert.AreEqual(item.Description, _item.Description);
            Assert.AreEqual(item.Type, _item.Type);
            Assert.IsTrue(item.LastAdjustmentDate.HasValue);
            Assert.AreEqual(item.LastAdjustmentDate.Value.Date,
               _item.LastAdjustmentDate.Value.Date);
            Assert.IsTrue(item.LastReceivedDate.HasValue);
            Assert.AreEqual(item.LastReceivedDate.Value.Date,
              _item.LastReceivedDate.Value.Date);
            Assert.IsTrue(item.LastReturnToVendorDate.HasValue);
            Assert.AreEqual(item.LastReturnToVendorDate.Value.Date,
              _item.LastReturnToVendorDate.Value.Date);

            Assert.IsTrue(item.Upcs.Count == 1);
            Assert.AreEqual(item.Upcs[0].Id, _item.Upcs[0].Id);
            Assert.AreEqual(item.Upcs[0].IsPrimary, _item.Upcs[0].IsPrimary);
            Assert.AreEqual(item.Upcs[0].UpcNo, _item.Upcs[0].UpcNo);
        }
}

An Item gets hooked up in the Initialize method of the test suite and then this test does 17 assertions on the item it gets back from the session. There are enough things wrong with this scenario to make me write a blog post about it. Which is saying something these days, if I’m not drunk blogging VPILFs and whining about briskets, I’m pretty much not writing. I digress.

First of all, it’s not really a unit test because it goes outside the boundary of the object under test by setting up an item, saving it to the database and then selecting it back out of the database for 17 assertions. The current project has a section for Functional/Integration tests and in all reality, this test probably belongs there.

Second, it’s poorly named. The names of test methods should read like documentation for the object under test. “SelectTest” tells me next to nothing about what is going on here and in fact, isn’t really what’s being tested at all. I have to read the entire test to know what’s being tested. In truth, this test is trying to verify certain properties are set on an Item. A correct name for this test is “IdIsGreaterThanZeroAndIdIsSameAsOriginalItemIdAndAutoReplenishedIsSameAsOriginalAutoReplenished
AndStatusIsSameAsOriginalStatusAndMaintenanceLevelIsSameAsOriginalMaintenanceLevel
AndDescriptionIsSameAsOriginalDescriptionAndTypeIsSameAsOriginalType
AndLastAdjustmentDateHasValueAndLastAdjustmentDateValueIsSameAsLastAdjustmentDateValue
AndLastReceivedDateHasValueAndLastReceivedDateValueIsSameAsOriginalLastReceivedDateValue
AndLastReturnToVendorDateHasValue
AndLastReturnToVendorDateValueIsSameAsLastReturnToVendorDateValue
AndHasOneUpcAndThatUPCsIdIsSameAsOriginalItemsUPCId
AndUPCIsPrimaryIsSameAsOriginalItemsUPCIsPrimary
AndUPCNoIsSameAsOriginalItemsUPCNo”

Whew. Deep breath. In through the nose, out through the mouth. Back with me? Good. This brings us to the final problem of this test and that is 17 @#$% ASSERTIONS IS TOO MANY FOR ONE @#$%^ TEST! Tests should be like objects, they should have one function and one function only. It’s amazing how many OO gurus there are out there who think tests like this are completely normal.

There are several reasons why you’d want one assertion per test. First, it makes for a very clean documentation of the object under test. Assuming all the test names are well-written, you can just read the tests and have a great idea what the object can and cannot do. Second, most unit test frameworks fail a test at the first failed assertion. If you have 17 of them, you don’t really know what’s broken about your object when the first one fails. You have to fix that first before running the test again. Finally, you’re really running multiple tests, you’re just doing it implicitly. Any time you have the option of doing something explicitly or implicitly, you should ALWAYS choose the former. Even if your framework runs all assertions, you’re doing so on object state that is no longer clean after the first assert failed. This isn’t giving you the kind of testing that you’re looking for.

In the end, test should have a single piece of functionality to test, they should be well-named and they should have a single assert. This will prevent some jerk from writing a blog post about your test that’s he’s having to fix 5 months after you wrote it. Even if you’re that jerk.

References: Write Maintainable Unit Tests That Will Save You Time And Tears
Avoid multiple asserts in a single unit test: revisited
Testing: One Assertion Per Test