Unit Testing the Duck

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();
mockHost.SetupAllProperties();
var parser = new RubberduckPars

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.

StackExchange Code Review Q#118473, answer score: 6