6
\$\begingroup\$

I recently reinstated the unit tests in Rubberduck. Previously, our parser was a synchronous parser, with everything running in sequence, and we could just request a parse result. Now, however, it runs asynchronously and we can only request parses. As a result, I have to somehow perform a blocking call to the parser, which I did with a semaphore. First, the semaphore blocks the code from continuing to execute, then the event handlers that gets called when the parser state changes releases it (or, if the code parses remarkably fast, the semaphore has a slot available and waiting for the method to take).

Below are a subset of the tests for my Introduce Parameter refactoring. While I am especially looking for feedback on how I handle the blocking parser and the way I set up the tests in general, all feedback is welcome.

[TestClass]
public class IntroduceParameterTests
{
    private readonly SemaphoreSlim _semaphore = new SemaphoreSlim(0, 1);

    void State_StateChanged(object sender, ParserStateEventArgs e)
    {
        if (e.State == ParserState.Ready)
        {
            _semaphore.Release();
        }
    }

    [TestMethod]
    public void IntroduceParameterRefactoring_NoParamsInList_Sub()
    {
        //Input
        const string inputCode =
@"Private Sub Foo()
    Dim bar As Boolean
End Sub";
        var selection = new Selection(2, 10, 2, 13); //startLine, startCol, endLine, endCol

        //Expectation
        const string expectedCode =
@"Private Sub Foo(ByVal bar As Boolean)

End Sub";

        //Arrange
        var builder = new MockVbeBuilder();
        VBComponent component;
        var vbe = builder.BuildFromSingleStandardModule(inputCode, out component);
        var project = vbe.Object.VBProjects.Item(0);
        var module = project.VBComponents.Item(0).CodeModule;
        var codePaneFactory = new CodePaneWrapperFactory();
        var mockHost = new Mock<IHostApplication>();
        mockHost.SetupAllProperties();
        var parser = new RubberduckParser(vbe.Object, new RubberduckParserState());

        parser.State.StateChanged += State_StateChanged;
        parser.State.OnParseRequested();
        _semaphore.Wait();
        parser.State.StateChanged -= State_StateChanged;

        var qualifiedSelection = new QualifiedSelection(new QualifiedModuleName(component), selection);

        //Act
        var refactoring = new IntroduceParameter(parser.State, new ActiveCodePaneEditor(vbe.Object, codePaneFactory), null);
        refactoring.Refactor(qualifiedSelection);

        //Assert
        Assert.AreEqual(expectedCode, module.Lines());
    }

    [TestMethod]
    public void IntroduceParameterRefactoring_ImplementsInterface_MultipleInterfaceImplementations()
    {
        //Input
        const string inputCode1 =
@"Sub fizz(ByVal boo As Boolean)
End Sub";

        const string inputCode2 =
@"Implements IClass1

Sub IClass1_fizz(ByVal boo As Boolean)
    Dim fizz As Date
End Sub";

        const string inputCode3 =
@"Implements IClass1

Sub IClass1_fizz(ByVal boo As Boolean)
End Sub";
        var selection = new Selection(4, 10, 4, 14); //startLine, startCol, endLine, endCol

        //Expectation
        const string expectedCode1 =
@"Sub fizz(ByVal boo As Boolean, ByVal fizz As Date)
End Sub";

        const string expectedCode2 =
@"Implements IClass1

Sub IClass1_fizz(ByVal boo As Boolean, ByVal fizz As Date)

End Sub";

        const string expectedCode3 =
@"Implements IClass1

Sub IClass1_fizz(ByVal boo As Boolean, ByVal fizz As Date)
End Sub";

        //Arrange
        var builder = new MockVbeBuilder();
        var project = builder.ProjectBuilder("TestProject1", vbext_ProjectProtection.vbext_pp_none)
            .AddComponent("IClass1", vbext_ComponentType.vbext_ct_ClassModule, inputCode1)
            .AddComponent("Class1", vbext_ComponentType.vbext_ct_ClassModule, inputCode2)
            .AddComponent("Class2", vbext_ComponentType.vbext_ct_ClassModule, inputCode3)
            .Build();
        var vbe = builder.AddProject(project).Build();
        var component = project.Object.VBComponents.Item(1);
        vbe.Setup(v => v.ActiveCodePane).Returns(component.CodeModule.CodePane);

        var codePaneFactory = new CodePaneWrapperFactory();
        var mockHost = new Mock<IHostApplication>();
        mockHost.SetupAllProperties();
        var parser = new RubberduckParser(vbe.Object, new RubberduckParserState());

        parser.State.StateChanged += State_StateChanged;
        parser.State.OnParseRequested();
        _semaphore.Wait();
        parser.State.StateChanged -= State_StateChanged;

        var qualifiedSelection = new QualifiedSelection(new QualifiedModuleName(component), selection);

        var module1 = project.Object.VBComponents.Item(0).CodeModule;
        var module2 = project.Object.VBComponents.Item(1).CodeModule;
        var module3 = project.Object.VBComponents.Item(2).CodeModule;

        var messageBox = new Mock<IMessageBox>();
        messageBox.Setup(m => m.Show(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MessageBoxButtons>(), It.IsAny<MessageBoxIcon>()))
                  .Returns(DialogResult.OK);

        //Act
        var refactoring = new IntroduceParameter(parser.State, new ActiveCodePaneEditor(vbe.Object, codePaneFactory), messageBox.Object);
        refactoring.Refactor(qualifiedSelection);

        //Assert
        Assert.AreEqual(expectedCode1, module1.Lines());
        Assert.AreEqual(expectedCode2, module2.Lines());
        Assert.AreEqual(expectedCode3, module3.Lines());
    }

    [TestMethod]
    public void IntroduceParameterRefactoring_PassInTarget_Nonvariable()
    {
        //Input
        const string inputCode =
@"Private Sub Foo()
    Dim bar As Boolean
End Sub";

        //Arrange
        var builder = new MockVbeBuilder();
        VBComponent component;
        var vbe = builder.BuildFromSingleStandardModule(inputCode, out component);
        var project = vbe.Object.VBProjects.Item(0);
        var module = project.VBComponents.Item(0).CodeModule;
        var codePaneFactory = new CodePaneWrapperFactory();
        var mockHost = new Mock<IHostApplication>();
        mockHost.SetupAllProperties();
        var parser = new RubberduckParser(vbe.Object, new RubberduckParserState());

        parser.State.StateChanged += State_StateChanged;
        parser.State.OnParseRequested();
        _semaphore.Wait();
        parser.State.StateChanged -= State_StateChanged;

        var messageBox = new Mock<IMessageBox>();
        messageBox.Setup(m => m.Show(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MessageBoxButtons>(), It.IsAny<MessageBoxIcon>()))
                  .Returns(DialogResult.OK);

        //Act
        var refactoring = new IntroduceParameter(parser.State, new ActiveCodePaneEditor(vbe.Object, codePaneFactory), messageBox.Object);

        //Assert
        try
        {
            refactoring.Refactor(parser.State.AllUserDeclarations.First(d => d.DeclarationType != DeclarationType.Variable));
            messageBox.Verify(m =>
                    m.Show(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MessageBoxButtons>(),
                        It.IsAny<MessageBoxIcon>()), Times.Once);
        }
        catch (ArgumentException e)
        {
            Assert.AreEqual("Invalid declaration type", e.Message);
            Assert.AreEqual(inputCode, module.Lines());
            return;
        }

        Assert.Fail();
    }
}
\$\endgroup\$
6
\$\begingroup\$

I don't believe a Semaphore is needed in the first place. Remove it, register an event handler and continue your "act" and "assert" phase inside it.

Something like this:

parser.State.StateChanged += (o, e) => {
    var qualifiedSelection = new QualifiedSelection(new QualifiedModuleName(component), selection);
    var refactoring = new IntroduceParameter(parser.State, new ActiveCodePaneEditor(vbe.Object, codePaneFactory), null);
    refactoring.Refactor(qualifiedSelection);

    Assert.AreEqual(expectedCode, module.Lines());
};

After creating a quick scenario that I believe mimics your use case, it seems to work just fine: https://gist.github.com/Vannevelj/5d0e348fd1424492ff8f


parser.State.OnParseRequested();

I'm not feeling comfortable with this name for a public member since it's not of the [verb][action] form -- RequestParse() might be more appropriate.


var selection = new Selection(4, 10, 4, 14); //startLine, startCol, endLine, endCol

Either use named arguments or don't use them -- don't put them in comments


Why do you group 3 scenarios in one test? Either use a parameterized test or extract common logic and keep it separated. Nobody wants to sift through multiple test cases when one of them fails.


Avoid try-catch in a unit test -- that means you're doing it the other way around. Using [ExpectedException(typeof(ArgumentException), "Invalid declaration type"] you've got most of it covered already though I could see why you also want to compare whether the code has changed.


Don't use an empty Assert.Fail(), pass in a message.


try
{
    refactoring.Refactor();
    messageBox.Verify();
} catch (ArgumentException)
}
Assert.Fail();

Given this setup, does it make sense to do the mock.Verify() call? If refactoring.Refactor() throws the exception, the mock.Verify() call will never be evaluated.

\$\endgroup\$
  • \$\begingroup\$ You are very right. I moved that .Verify() call to the catch. As for grouping three scenarios, that is testing interfaces, which must be in a different class module in VBA. That is really only once scenario that makes sure the entire output works. I guess I could split it into three scenarios identical but for the Assert, but that seems a bit redundant. \$\endgroup\$ – Hosch250 Feb 1 '16 at 16:35
  • \$\begingroup\$ OnParseRequested raises an event, ParseRequested - its name is in line with naming guidelines IMO. \$\endgroup\$ – Mathieu Guindon Feb 1 '16 at 21:16
  • 4
    \$\begingroup\$ @Mat'sMug I would say it's somewhat ambiguous since two conventions collide here. You have to choose which one to prioritize of course -- I personally would probably go with the one that defines the public API. Often, OnEventTriggered methods are private rather than public because they're not meant to be called directly. You could also just do both and have a public method RequestParse() call private void OnParseRequested() \$\endgroup\$ – Jeroen Vannevel Feb 1 '16 at 21:20
5
\$\begingroup\$

IMO this eventhandler

void State_StateChanged(object sender, ParserStateEventArgs e)
{
    if (e.State == ParserState.Ready)
    {
        _semaphore.Release();
    }
}  

should contain an if condition for the case that an error occured in a way that it will throw an exception.

If in the private method RubberDuckParse.Parse() occurs either a COMException or a SyntaxErrorException the ParserState will be set to Error. This won't ever (if I read the code correctly) raise the state Ready hence your test is running forever because the SemaphoreSlim is waiting forever.

\$\endgroup\$
5
\$\begingroup\$

There's a fair amount of setup and teardown code. I'd change many of the local variables into fields, and then add a [TestInitialize] and [TestCleanUp] method. Extract as much of the test rig as you can into these methods.

Always remember that test code is production code too. It's just code that we don't ship to users. It deserves to be written to a high level of Single Responsibility and DRYness as well.

By moving all but what's different out of the tests, it's easier to highlight what it is that we're actually testing, rather than getting lost in a bunch of boiler plate setup code.

\$\endgroup\$
  • 2
    \$\begingroup\$ That said, boy that builder class is handy. I know exactly how much boiler plate would be here if it didn't exist. \$\endgroup\$ – RubberDuck Feb 1 '16 at 10:30
4
\$\begingroup\$
parser.OnParseRequested();

That's the asynchronous hook that the application uses to trigger a complete parsing of everything in the VBE: using it for your test code makes your life much harder than it needs to be, leading you to do funky stuff like that SemaphoreSlim thing.

If the RubberduckParser class doesn't expose a method to trigger a synchronous parse, make one. It doesn't even need to be on the IRubberduckParser interface that the rest of Rubberduck sees, since the tests instantiate the concrete implementation directly.

I would then expect the tests to look something like this:

// arrange
//(mock code setup)

var state = new RubberduckParserState();
var parser = new RubberduckParser(mockVbe.Object, state);

parser.Parse();

if (state.State != ParserState.Ready)
{
    Assert.Inconclusive("Test code could not be parsed.");
}

// instantiate SUT

// act
//sut.DoSomething();

// assert
Assert.AreEqual(expected, actual);

Now, the whole setup code for each and every single test is going to be all about getting a mock VBE instance that contains your test code: there has to be a way of getting the input/expected code out of there.

Actually, the fact that these tests intake a whole mock IDE and compare the side effects of each feature, tells me that all these tests are in fact quite sophisticated, automated integration tests.

If more granular tests can't be written against the involved objects, it means something else: the IntroduceParameter class (which should really be IntroduceParameterRefactoring, to avoid confusing it with a method's name) has too many responsibilities, as do every feature that can only be tested via its side-effects.

The consequence cause of this (it could only be a consequence if we had TDD'd this!) is presumably that there's a lot of duplicated code everywhere, code that's responsible for doing things that possibly need to be done in pretty much every one of theser functionalities, but that are implemented - unit tests would be much more granular, and much more numerous. "A unit is a class" is only true when a class has only one responsibility.

There should be a bunch of tests that use the same input code just because they need input code or a mock CodePane object - hence the setup code that creates the mock VBE instance could be moved to a private method in the test class, that creates the mock VBE with the setup code, once - for all tests in the class... and then,

Similarly, I think I'd move the MessageBox mock setup to its own function, that sets up the return value according to a given DialogResult parameter.


TL;DR:

Your tests are good integration tests, that correctly verify whether a feature works as intended as a unit - but the "unit" iself is most likely too widely scoped. Note, I'm not blaming you - I'm completely guilty of this, too.

\$\endgroup\$
  • \$\begingroup\$ At one point I had exposed a parse method that took in a string for testing. Not sure if it still exists, but it felt much better. Totally forgot about it until just now. \$\endgroup\$ – RubberDuck Feb 2 '16 at 0:42
  • 1
    \$\begingroup\$ @RubberDuck that would be the low-level method that returns the ANTLR IParseTree... but the SUT works off the "symbol table" derived from it. \$\endgroup\$ – Mathieu Guindon Feb 2 '16 at 0:46
  • \$\begingroup\$ Ahh. I guess I've missed a lot of development. \$\endgroup\$ – RubberDuck Feb 2 '16 at 0:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.