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

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.

StackExchange Code Review Q#63506, answer score: 5