Refactoring Unit Tests

Llewellyn Falco and I paired on an introduction to his AcceptanceTests tool. I really like that tool for evaluating objects during testing in an easy, flexible and evolutionary way. It’s a great tool, but that’s not what this post is about.

Rob Hughes (@rhughesjr ) heard via Twitter that Llewellyn and I also refactored a bunch of RoslynDom tests to remove redundant code, and asked that I do a blog post about this aspect of our pairing. That’s what this post is about.

I wrote this about some later refactoring that Llewellyn inspired – so he should get all of the credit and none of the blame. I don’t think there is anything groundbreaking here. Just a detailed walkthrough of refactoring a set of unit tests, along with the logic behind the changes.

Removing redundant code from unit tests

When I write normal, non-unit test code, I think about refactoring common code from the very beginning. A lack of redundancy and flexibility/extensibility are primary forces I think about in software.

But not in unit tests. I believe that unit test creation should be a bit more like an amoeba eating up everything it can touch. A rigid shape caused by code reuse can hide a reduction in logical coverage and in LOC coverage. So, when Llewellyn and I began there were almost no helper methods in the RoslynDom tests.

RoslynDom is very simple in goal – load C# code into a language agnostic structure, allow interrogation and changes to the structure (yes, it’s mutable), and output C# code which looks like the original code. Oh, and you can ask whether two code structures have the same intent.

Because it does a few things across a mid-size number of different elements, there are a lot of very similar tests.

I believe it is best to discover where your tests are redundant, by refactoring them at a later date, and after you have a big pile of tests.

I do not recommend strategizing ahead of time about how to maximize code reuse in unit tests (been there, done that). It’s the only place in your code where I think the copy-paste-fix-differences cycle is OK. I’m referring only to strategizing and designing around code reuse too early.

Strategizing isolation of unit tests very early in the process is extremely helpful. I would say it is necessary, but if you have no tests, I don’t really care if you isolate your first ten tests.

So, why ever remove the redundant code?

Too often unit tests are a static pile that rots during our application development process. If we’re deeply committed, we fix all the broken tests. If we aren’t tests are disabled or removed as the schedule requires. Regardless, unit tests generally become rotten and stinky.

If tests aren’t isolated, rotting tests may become impossible to run. In the days before mocking, I saw a team toss >1,000 tests because of a database change. But RoslynDom tests are isolated because of the nature of the problem.

Beyond maintaining the ability to run your tests, the universal problem is that rotting tests become impossible to read and understand. Your unit tests are the best view into your system. It’s why we have test names that explain why we wrote the test (RoslynDom test naming is mediocre and adequate, not brilliant).

As you project the technical changes in the rest of this post to tests onto your own projects, think about how to increase clarity in what each test is accomplishing.

OK, already! What are the changes?

Here’s a simple test before refactoring

[TestMethod, TestCategory(SimpleAttributeCategory)]
public void Can_get_attributes_on_class()
{
var csharpCode = @"
[Serializable]
public class MyClass
{ }
"
;
var root = RDomFactory.GetRootFromString(csharpCode);
var class1 = root.Classes.First();
var attributes = class1.Attributes;
Assert.AreEqual(1, attributes.Count());
Assert.AreEqual("Serializable", attributes.First().Name);
}


RoslynDom has almost 600 unit tests. I like test categories and use constants to avoid mistyping them.

There are eight tests nearly identical to this that change what the attribute is placed on (class, structure, method, parameter, etc), and thus different code strings. There are also variations with different numbers of attributes. So, clearly there is a lot of redundant code (about 32 tests).

The two lines that retrieve the attributes are problematic. They will be different for every test. That’s a job for our mild-mannered super-hero: the delegate!

Refactoring the test part of the code with a delegate results in this method:


private static void VerifyAttributes(string csharpCode,
Func<IRoot, IEnumerable<IAttribute>> makeAttributes,
int count, params string[] names)
{
var root = RDomCSharp.Factory.GetRootFromString(csharpCode);
var attributes = makeAttributes(root).ToArray();
Assert.AreEqual(count, attributes.Count());
for (int i = 0; i < attributes.Count(); i++)
{
Assert.AreEqual(names[i], attributes[i].Name);
}
}


The things that change between tests are the input code (csharpCode), how the attributes are retrieved (makeAttributes), the count of attributes expected (count) and the expected parameter names (names).

The test calls this method with:


[TestMethod, TestCategory(SimpleAttributeCategory)]
public void Can_get_attributes_on_class()
{
var csharpCode = @"
[Serializable]
public class MyClass
{ }
"
;
VerifyAttributes(csharpCode, root => root.Classes.First().Attributes,
1, "Serializable");
}


 

The value of this call isn’t removing five lines of code – it’s making it more clear what those five lines of code did.

This change simplified 32 tests and made them more readable.

All tests aren’t that simple


The next set of tests looked at attribute values. The initial test was:


[TestCategory(AttributeValuesCategory)]
public void Can_get_attribute_values_on_class()
{
var csharpCode = @"
[LocalizationResources("
"Fred"", ""Joe"", Cats=42)]
[Name("
"KadGen-Test-Temp"")]
[SemanticLog]
public class MyClass
{ }
"
;
var root = RDomFactory.GetRootFromString(csharpCode);
var attributes = root.Classes.First().Attributes;
Assert.AreEqual(3, attributes.Count());
var first = attributes.First();
Assert.AreEqual("LocalizationResources", first.Name);
Assert.AreEqual(3, first.AttributeValues.Count());
var current = first.AttributeValues.First();
Assert.AreEqual("LocalizationResources", current.Name);
Assert.AreEqual("Fred", current.Value);
Assert.AreEqual(LiteralKind.String, current.ValueType);
current = first.AttributeValues.Skip(1).First();
Assert.AreEqual("LocalizationResources", current.Name);
Assert.AreEqual("Joe", current.Value);
Assert.AreEqual(LiteralKind.String, current.ValueType);
current = first.AttributeValues.Last();
Assert.AreEqual("Cats", current.Name);
Assert.AreEqual(42, current.Value);
Assert.AreEqual(LiteralKind.Numeric, current.ValueType);
Assert.AreEqual("Name", attributes.Skip(1).First().Name);
Assert.AreEqual("SemanticLog", attributes.Last().Name);
}


 

I doubt you can glance at that and understand what it does.

One approach would be to pass a complex data structure to the previous Verify method. I could probably have created something slightly readable with JSON, or XML literals if I was in Visual Basic. But unit tests demand a KISS (Keep it Simple Silly) approach.

If the VerifyAttributes method returns the IEnumerable of IAttribute it’s already creating, the first five lines (and a couple of others) can be replaced with:


var attributes = VerifyAttributes(csharpCode, 
root => root.Classes.First().Attributes,
3, "LocalizationResources", "Name", "SemanticLog")
.ToArray();



Making it an array simplifies accessing individual elements.

For the rest of the test, it makes sense to apply the same refactoring approach that worked on attributes. But here, there’s a name, a value, and a literal kind. Again, one approach is a complex structure, but a simpler approach is to test the count and return the IEnumerable of IAttributeValue for more testing:


private IEnumerable<IAttributeValue> VerifyAttributeValues(

IAttribute attribute, int count)

{

var attributeValues = attribute.AttributeValues;

Assert.AreEqual(count, attributeValues.Count());

return attributeValues;

}



 

An additional method simplifies the testing of individual attribute values:


private void VerifyAttributeValue(IAttributeValue attributeValue, string name, object value, LiteralKind kind)

{

Assert.AreEqual(name, attributeValue.Name);

Assert.AreEqual(value, attributeValue.Value);

Assert.AreEqual(kind, attributeValue.ValueType);

}



 

Calling these methods is a great opportunity for named parameters. Take a minute to compare the readability of this code to the same test at the start of this section (and yep, I wish I’d also used named parameters for the VerifyAttributes calls):


[TestMethod, TestCategory(AttributeValuesCategory)]
public void Can_get_simple_attribute_values_on_property()
{
var csharpCode = @"
public class MyClass
{
[Version(2)]
[Something(3, true)]
public string foo {get; set; }
}
"
;
var attributes = VerifyAttributes(csharpCode,
root => root.Classes.First().Properties.First().Attributes,
2, "Version", "Something")
.ToArray();
var attributeValues = VerifyAttributeValues(attributes[0], count: 1)
.ToArray();
VerifyAttributeValue(attributeValues[0], name: "", value: 2, kind: LiteralKind.Numeric);
attributeValues = VerifyAttributeValues(attributes[1], count: 2)
.ToArray();
VerifyAttributeValue(attributeValues[0], name: "", value: 3, kind: LiteralKind.Numeric);
VerifyAttributeValue(attributeValues[1], name: "", value: true, kind: LiteralKind.Boolean);
}


Does that really fit every circumstance of the area you’re testing?


Rarely will there be such a large number of tests doing such trivial comparisons. In this same test file/test topic, there are also tests of passing types, instead of literals, to attributes. This only appears three places in the file:


[TestMethod, TestCategory(AttributeValuesCategory)]
public void Can_get_attribute_value_of_typeof_identifier_only_on_class()
{
var csharpCode = @"
[Test(TypeTest = typeof(Foo))]
public class MyClass
{ }
"
;
var attributes = VerifyAttributes(csharpCode,
root => root.Classes.First().Attributes,
1, "Test")
.ToArray();
var current = VerifyAttributeValues(attributes[0], count: 1)
.First();
Assert.AreEqual(LiteralKind.Type, current.ValueType);
var refType = current.Value as RDomReferencedType;
Assert.IsNotNull(refType);
Assert.AreEqual("Foo", refType.Name);
}


 

Honestly, I might not bother refactoring this if it was in a test file that was full of variations and refactoring opportunities with more payback. But in this nice clean test file, it’s jarring.

Using a different name, rather than an overload, clarifies that something different is being checked:


private static void VerifyTypeOfAttributeValue(IAttributeValue current, string name)

{

Assert.AreEqual(LiteralKind.Type, current.ValueType);

var refType = current.Value as RDomReferencedType;

Assert.IsNotNull(refType);

Assert.AreEqual(name, refType.Name);

}



 

Making the call:


[TestMethod, TestCategory(AttributeValuesCategory)]

public void Can_get_attribute_value_of_typeof_referenced_on_class()

{

var csharpCode = @"

[Test(TypeTest = typeof(DateTime))]

public class MyClass

{ }

"
;

var attributes = VerifyAttributes(csharpCode,

root => root.Classes.First().Attributes,

1, "Test")

.ToArray();

var current = VerifyAttributeValues(attributes[0], count: 1)

.First();

VerifyTypeOfAttributeValue(current, name: "DateTime");

}



 


Yes, you could make it smaller


The actual change with all these refactorings was about 130 lines of code, 860 to 730 vertical lines in this test class. Because the same set of tests were repeated multiple times, and the C# code I’m testing is so similar for different contexts, I could have reduced the code much further, maybe even to half the size.

But reducing the code size in unit tests beyond the point of maximum clarity is not helpful. The main driving forces for tests are that they be stand-alone and readable. Each test in the resulting file is stand-alone and more readable than without the refactoring. Each should be less than a screen in size, but once you reach this point, clarity trumps size.

Write clear verify tests and allow the reader to correctly assume that each verify method tests the parameters passed, and nothing else.

And then there’s change…


RoslynDom does a handful of things. One of the tricky things that is not tested by these unit tests is round-tripping of attributes, which has some tests in another part of the test suite.

While I’m curious how well the code in this test file runs, I know there are presently some low-priority issues roundtripping attributes. Before creating the common code, it would have been a lot of bother to experiment with round-tripping to see how serious these issues might be. After the changes, I just need to add a couple lines of code to the VerifyAttributes method.

When I actually did this, I got a lot of the expected messages. I know I read any kind of attribute layout, but am opinionated (for now) on outputting as separate attributes:

Result Message:
Assert.AreEqual failed. 
Expected:<
[Serializable, TestClass]
public interface MyInterface
{ }>. 
Actual:<
[Serializable]
[TestClass]
public interface MyInterface
{ }>.



What was unexpected was that 3 tests – those typeof tests –crashed on outputting the code. I get excited anytime I find and fix a problem, because it’s quite challenging to test the myriad of code possibilities that RoslynDom is intended to support.

I liked this test enhancement so much I left it in. I have a rule that all tests that crash RoslynDom should result in new tests – so I had to work it into the test suite one way or another. I added a Boolean value to the VerifyAttributes method to skip the BuildSyntax test where I know it will fail just because of attribute layout.

Here I used a refactoring trick.

I added the new Boolean value as the first parameter – even though that’s a sucky place for it.

I did a replace of “VerifyAttributes(csharpCode,” with “VerifyAttributes(false, csharpCode,” with a Replace In Files for just the current document so I could check the changes. That was good because I initially had a space at the end, which missed occurrences where I wrapped the delegate to the next line.

Once everything built with the new parameter, I refactored with a signature change to put the Boolean where I wanted it, and then changed the Boolean value to true on the tests where I wanted to skip the assertion that the input and output match. I always call BuildSyntax to ensure it doesn’t crash, but I don’t expect to roundtrip the code perfectly when attributes are combined (at present).

This will also make it dirt simple to find these tests if/when I decided to support round-tripping multiple layouts of attributes. I’ll just ignore and then remove the parameter.

Take a look at your tests and see whether you can make some of them easier to understand with some tactically applied helper methods.

Leave a Reply

Your email address will not be published. Required fields are marked *


*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>