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.

  

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.

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.