Unit Testing - A Better Solution

Question

Following-up on the Automagic testing framework for VBA review, I've refactored much of the TestEngine static class, and introduced a TestMethod class.

Refactoring part of the TestEngine into its own class introduced new problems that needed new solutions; because the client code is using the Assert class default instance, each TestMethod instance had to be listening for the same instance's events - VBA events cannot be registered/unregistered at will, so I introduced a ListenAssertEvents flag in each instance, that would only be True when that instance is running.

I never thought I'd say this, but it's a good thing VBA doesn't support multithreaded execution. I'd have gone mad if it were the case.

Here's the output (in the immediate pane) for a TestClass1 class in a client VBA project named VBAProject:

TestEngine.RunAllTests "VBAProject", new TestClass1
2014年09月21日 20:19:44 MagicCommentWorks: [INCONCLUSIVE] - No assertions made.
2014年09月21日 20:19:44 TestAreEqual: [PASS]
2014年09月21日 20:19:44 TestAreNotEqual: [PASS]
2014年09月21日 20:19:44 TestAreSame: [FAIL] - AreSame failed: Objects should be same reference.
2014年09月21日 20:19:44 TestAreNotSame: [PASS]
2014年09月21日 20:19:44 TestFail: [FAIL] - Fail failed: This wasn't meant to be.
2014年09月21日 20:19:44 TestInconclusive: [INCONCLUSIVE] - No idea.
2014年09月21日 20:19:44 TestIsFalse: [PASS]
2014年09月21日 20:19:44 TestIsNothing: [PASS]
2014年09月21日 20:19:44 TestIsNotNothing: [PASS]
2014年09月21日 20:19:44 TestIsTrue: [PASS]
2014年09月21日 20:19:44 TestBlowUp: [FAIL] - AreEqual failed: This failed assertion prevents reporting the division by zero that follows.
2014年09月21日 20:19:44 TestNoAssert: [INCONCLUSIVE] - No assertions made.

This time I've thoroughly verified the output, it's exactly what's expected - in case I missed anything, here's the TestClass1 client code:

Option Explicit
Public Sub ThisIsNoTest()
 Err.Raise 5
End Sub
'@TestMethod
Public Sub MagicCommentWorks()
End Sub
Private Sub TestPrivateMethod()
End Sub
Public Sub TestAreEqual()
 Assert.AreEqual 12, 12, "Values should be equal."
End Sub
Public Sub TestAreNotEqual()
 Assert.AreNotEqual 12, 34, "Values should not be equal."
End Sub
Public Sub TestAreSame()
 Assert.AreSame New Collection, New Collection, "Objects should be same reference."
End Sub
Public Sub TestAreNotSame()
 Assert.AreNotSame New Collection, New Collection, "Objects should not be the same reference."
End Sub
Public Sub TestFail()
 Assert.Fail "This wasn't meant to be."
End Sub
Public Sub TestInconclusive()
 Assert.IsTrue True
 Assert.Inconclusive "No idea."
 Assert.Fail "shouldn't output this result."
End Sub
Public Sub TestIsFalse()
 Assert.IsFalse False, "True should be False."
End Sub
Public Sub TestIsNothing()
 Dim foo As Object
 Assert.IsNothing foo, "Foo should be nothing."
End Sub
Public Sub TestIsNotNothing()
 Dim foo As New Collection
 Assert.IsNotNothing foo, "Foo shouldn't be nothing."
End Sub
Public Sub TestIsTrue()
 Assert.IsTrue True, "False should be True."
End Sub
Public Sub TestBlowUp()
 Assert.IsTrue True
 Assert.AreEqual False, True, "This failed assertion prevents reporting the division by zero that follows."
 Debug.Print 1 / 0
 Assert.Fail "Test should have failed by now."
End Sub
Public Sub TestNoAssert()
End Sub

---

TestEngine class module (with default instance)

The TestEngine class' goal has been reduced to the following:

• Use the Reflection library to get all methods from the specified class instance.
• Find test methods among all found methods - a test method is a Public Sub whose name begins with Test, or that is decorated with a "magic comment" that marks the method as a test method.
• Execute all found test methods, delegate outputting the test results.

Option Explicit
Private Type TTestEngine
 Output As ITestOutput
 RegisteredTests As Dictionary
 CurrentTest As TestMethod
 CurrentTestAllResults As List
 CurrentTestFailedOrInconclusiveResults As List
End Type
Private Const TestMethodNamePrefix As String = "Test"
Private Const TestMethodAttribute As String = "TestMethod"
Private this As TTestEngine
Public Property Get Output() As ITestOutput
 Set Output = this.Output
End Property
Public Property Set Output(ByVal value As ITestOutput)
 Set this.Output = value
End Property
Public Sub RunAllTests(ByVal projectName As String, ByRef classInstance As Object)
 Set this.RegisteredTests = ReflectTestMethods(projectName, classInstance)
 Dim test As TestMethod
 Dim result As TestResult
 Dim item As Variant
 For Each item In this.RegisteredTests.Items 'requires Variant
 Set test = item 'cast Variant to TestMethod
 Set result = test.Run
 this.Output.WriteResult test.MethodName, result
 Next
End Sub
Private Function ReflectTestMethods(ByVal projectName As String, ByRef classInstance As Object) As Dictionary
 Dim classMethods As List
 Set classMethods = ClassModule.GetMethods(projectName, TypeName(classInstance))
 Dim result As New Dictionary
 Dim prospect As Method
 Dim tMethod As TestMethod
 For Each prospect In classMethods
 If CanAddTestMethod(prospect, result) Then
 Set tMethod = New TestMethod
 Set tMethod.OwnerInstance = classInstance
 tMethod.MethodName = prospect.name
 result.Add tMethod.MethodName, tMethod
 End If
 Next
 Set ReflectTestMethods = result
End Function
Private Function CanAddTestMethod(ByVal prospect As Method, ByRef testMethods As Dictionary) As Boolean
 Dim result As Boolean
 If Not IsTestMethod(prospect) Then Exit Function
 If testMethods.Exists(prospect.name) Then Exit Function
 CanAddTestMethod = True
End Function
Private Function IsTestMethod(ByVal TestMethod As Method) As Boolean
 Dim result As Boolean
 result = _
 Framework.Strings.StartsWith(TestMethodNamePrefix, TestMethod.name, False) Or _
 TestMethod.HasAttribute(TestMethodAttribute)
 If Not result Then Exit Function
 result = result And Framework.Strings.StartsWith("public sub", TestMethod.Signature, False)
 IsTestMethod = result
End Function
Private Sub Class_Initialize()
 Dim testOutput As New DebugTestOutput
 Set this.Output = testOutput 'default value. Client code may override.
End Sub

---

TestMethod class module

The whole CurrentTestXxxxxx hack from the previous iteration, was blatantly begging to be extracted into this class. I have no regrets; calling the Run function cleanly returns the test's result - the TestEngine isn't even aware that the test code might make 20 assertions and fail 15 times; there is no way a failed Assert call can stop a test method from running to completion, so I'm gathering all results in this.AssertResults, and when the method completes, I evaluate the test's result based on the contents of that result set:

Option Explicit
Private Type TTestMethod
 OwnerInstance As Object
 MethodName As String
 AssertResults As List
 ListenAssertEvents As Boolean
End Type
Private this As TTestMethod
Private WithEvents assertion As Assert
Public Property Get OwnerInstance() As Object
 Set OwnerInstance = this.OwnerInstance
End Property
Friend Property Set OwnerInstance(ByRef value As Object)
 Set this.OwnerInstance = value
End Property
Public Property Get MethodName() As String
 MethodName = this.MethodName
End Property
Friend Property Let MethodName(ByVal value As String)
 this.MethodName = value
End Property
Public Function Run() As TestResult
 Dim result As TestResult
 On Error GoTo CleanFail
 this.ListenAssertEvents = True
 CallByName this.OwnerInstance, this.MethodName, VbMethod
 this.ListenAssertEvents = False
CleanExit:
 Set Run = EvaluateResults
 Exit Function
CleanFail:
 this.AssertResults.Add TestResult.Create(Inconclusive, "Test raised an error: " & Err.Description)
 Resume CleanExit
End Function
Private Function EvaluateResults() As TestResult
 Dim result As TestResult
 Set result = TestResult.Create(Succeeded)
 If this.AssertResults.Count = 0 Then
 Set result = TestResult.Create(Inconclusive, "No assertions made.")
 ElseIf this.AssertResults.Count = 1 Then
 Set result = this.AssertResults.First
 Else
 Dim assertResult As TestResult
 For Each assertResult In this.AssertResults
 If assertResult.TestOutcome = Failed _
 Or assertResult.TestOutcome = Inconclusive _
 Then
 Set result = assertResult
 Exit For
 End If
 Next
 End If
 Set EvaluateResults = result
End Function
Private Sub assertion_Completed(ByVal result As TestResult)
 If this.ListenAssertEvents Then this.AssertResults.Add result
End Sub
Private Sub Class_Initialize()
 Set this.AssertResults = List.Create
 Set assertion = Assert.DefaultInstance
End Sub
Private Sub Class_Terminate()
 Set assertion = Nothing
End Sub

The EvaluateResults method seemingly defaults to success, and creates too many objects. I don't like it.

Answer 1

TestEngine

• I don't see these being used anywhere.

  CurrentTest As TestMethod
  CurrentTestAllResults As List
  CurrentTestFailedOrInconclusiveResults As List
  

• These are scoped to the class when they could be scoped to the IsTestMethod function.

  Private Const TestMethodNamePrefix As String = "Test"
  Private Const TestMethodAttribute As String = "TestMethod"
  

• I'm in the air about passing the project name to RunAllTests. You could just as easily call Application.VBE.ActiveVBProject. I don' think it's unreasonable to think that the end user is in the test project that they want to run. Of course, your way is more explicit and you may have other reasons for doing what you've done. So, take it for what it's worth (not much).

• I'm also no sure about passing the Test class in as an Object. I suppose it could implement an empty interface, just for the typing, but I don't know if that would really be any better. Just some food for thought.

• Here you have a bad comment followed by a good one.

  Dim test As TestMethod
  Dim result As TestResult
  Dim item As Variant
  For Each item In this.RegisteredTests.Items 'requires Variant
   Set test = item 'cast Variant to TestMethod
   Set result = test.Run
   this.Output.WriteResult test.MethodName, result
  Next
  

  Why does this.RegisteredTest.Items require a variant?

  • CanAddTestMethod feels weird, but it's fine. The alternatives are worse. If there was another way to return early from the function I would recommend it, but there isn't. It's actually a lot slicker than it looks.

• A readability nitpick here. I would rewrite this:

  Dim result As Boolean
  result = _
   Framework.Strings.StartsWith(TestMethodNamePrefix, TestMethod.name, False) Or _
   TestMethod.HasAttribute(TestMethodAttribute)
  

  Like this:

  Dim result As Boolean
  result = _
   Framework.Strings.StartsWith(TestMethodNamePrefix, TestMethod.name, False) _
   Or TestMethod.HasAttribute(TestMethodAttribute)
  

• Last, but not least. Be careful with circular references. Each TestMethod here has a reference to the Test1Class, but it has no way to dispose of the TestMethods. It doesn't even know about them (which shows a good separation of concerns actually).

  If CanAddTestMethod(prospect, result) Then
   Set tMethod = New TestMethod
   Set tMethod.OwnerInstance = classInstance
   tMethod.MethodName = prospect.name
   result.Add tMethod.MethodName, tMethod
  End If 
  

  The problem is the way COM handles garbage collection. Objects don't get destroyed until the reference count for them reaches zero. Since In this case, all of the methods will remain in memory until TestEngine exits it's scope. It might not cause an issue in this instance, but it would certainly be more memory efficient to get rid of them as you're finished with them. Set a breakpoint in TestMethod's Class_Terminate event and pay close attention to when (if ever) it's actually being raised.

  I'm having a hard time explaining this, so here is Microsoft's VB6 documentation on circular references. I find the most enlightening part of this document is this diagram.

  Circular Reference

TestMethod

• The Run sub..... wow man. That's error handling done right. It's a very clean routine. I like it. I know you don't like the flag, and truthfully, neither do I, but I don't see how it could be done differently.

• I see what you're talking about with the EvaluateResults function. Unfortunately, there's not much you can do. I don't suppose your List class has a Contains function? I'm sure it would be less efficient, but it would at least abstract the complexity of contained in the Else. The only other thing I can think to do is restrict each TestMethod to one and only one assertion and I wouldn't really recommend that.

• Mathieu Guindon: Wow, the currentXxxx stuff was meant to be deleted.. and I forgot! The test class is passed as object to make it as flexible and easy as possible for the client side - notice how simple TestClass1 is. The "requires variant" comment is there to remind me not to try a for each loop with a TestMethod, because no idea why, but it doesn't work - probably something about Dictionary.Items that I haven't figured out yet.. to me the cast as TestMethod comment would be the least useful ;)
• Mathieu Guindon: As for EvaluateResults, I need to iterate list items in the order they were added; yes my List has a Contains method, but no way of pulling the first item that matches a condition... and I don't want to assume a test method will only assert once! I'll take a close look at the Class_Terminate handler in TestMethod, you've got me seriously worried here - excellent review!
• RubberDuck: I should mention that I'm not 100% certain that you have a circular reference on your hands, but I think it's possible that you do. I also hadn't considered that you wanted the first instance. Add a comment there so you don't forget!

Answer 2

If this.AssertResults.Count = 0 Then
 Set result = TestResult.Create(Inconclusive, "No assertions made.")

No. Assertions are not a way to make a test pass, they're a way to make it fail. Anyone can write a passing test, that's why TDD asks you to write a failing test first.

This:

2014年09月21日 20:19:44 MagicCommentWorks: [INCONCLUSIVE] - No assertions made.

Should have been this instead:

2014年09月21日 20:19:44 MagicCommentWorks: [PASS]

No assertions made is not an inconclusive test - it's a passing test, in this case for a test method that's essentially an empty code block, but a passing test nonetheless.

Consider a test method written like this:

'arrange
Dim sut As New MyClassUnderTest
'act
Dim items As Collection
Set items = sut.GetItems
'assert
Dim item As Variant
For Each item In items
 If Not IsObject(item) Then
 Assert.Fail "All items are expected to be objects."
 Exit Sub
 End If
Next

If all item items in the items collection are objects, this test has no reason to fail, and should succeed. With the EvaluateResult as currently written, this test is Inconclusive because no assertions were made. Nonsense.