# Unit Testing - A Better Solution

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

Set tMethod = New TestMethod
Set tMethod.OwnerInstance = classInstance
tMethod.MethodName = prospect.name

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

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)
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.

# 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

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.

# 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.

• 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 Sep 23 '14 at 1:25
• 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! – Mathieu Guindon Sep 23 '14 at 1:30
• 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! – RubberDuck Sep 23 '14 at 1:57
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.


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.