Exceptions are swallowed/obscured

[erizzo]

In a simple usage like this:

var mock = new Mock<Something>();
using (Sequence.Create())
{
    mock.Setup(m => m.Method1()).InSequence();
    mock.Setup(m => m.Method2()).InSequence();
    mock.DoStuff();
}

If an exception is thrown from the DoStuff() method, the test will fail but with a sequence exception, hiding the actual exception from DoStuff(). It makes troubleshooting such tests very difficult. Basically I've had to comment-out all the Moq.Sequences stuff in order to get to the actual error.

[erizzo]

I'm fairly new to C# but what I think is happening is that the using block essentially gets compiled to the equivalent of

var s = new Sequence();
try
{
}
finally
{
    s.Dispose();
}

So in the case of an exception from anything inside the using block, the Sequence.Dispose() method is seeing that the sequence is incomplete, thus creating and throwing its own exception before the original exception has a chance to bubble up (due to Dispose() being in a finally block, or something very similar).

[stakx]

You are right that a using statement is roughly equivalent to a try…finally with a call to Dispose in the finally block, and it's this Dispose method that throws SequenceException. According to common .NET design guidelines, Dispose methods should never throw exceptions. It is unfortunate that Moq.Sequences does exactly that, thereby swallowing any exceptions that might already have been thrown inside the using block.

Unfortunately, this cannot really be fixed without fundamentally changing the syntactical usage pattern for Moq.Sequences.

There are ways to work around this problem, though. Which one applies depends on what you need:

1. Instead of letting the exception "bubble up" out of the using block (which might mean it gets swallowed), you can catch and process it early, while you're still inside the using (and before it gets swallowed):

   using (Sequence.Create())
   {
       …
       try
       {
           mock.DoStuff(); // this will throw
       }
       catch (Exception ex)
       {
           … // process the exception thrown by the above statement here
       }
   }
   

   or if you're unit testing, perhaps (in xUnit):

   Exception ex = null;
   using (Sequence.Create())
   {
       …
       ex = Record.Exception(() => mock.DoStuff());
   }
   Assert.IsType<DoStuffException>(ex);
   

2. If you are just interested in the exception thrown by mock.DoStuff(), and you want to ignore the exception thrown by sequence.Dispose, then perhaps you don't need Sequence.Create at all? In that case, uncommenting / removing code related to Moq.Sequences might be the right choice.

   After all, ideally, a unit test asserts one thing, and one thing only.

3. If you want both exceptions, that is, the one thrown by mock.DoStuff() and the one thrown by sequence.Dispose(), it gets trickier. Off the top of my head:

   Exception ex = null;
   var sequence = Sequence.Create();
   try
   {
       try
       {
           …
           mock.DoStuff(); // this will throw
       }
       catch (Exception innerEx)
       {
           ex = innerEx;
           throw;
       }
   }
   finally
   {
       try
       {
           sequence?.Dispose();
       }
       catch (SequenceException outerEx)
       {
           if (ex == null) throw; else throw new AggregateException(ex, outerEx);
       }
   }
   

I think it goes without saying that you want to avoid (3). It's not exactly legible code. If possible, try to do something like in (1) or (2), i.e. handle any exceptions inside the using block, or don't have the using block at all.

[erizzo]

In a unit test I do need to bubble up the underlying DoStuf() exception, since that is indicative of the real test failure. Sadly, we're not using xUnit, so I can't do the (relatively clean) option above. What I ended up finding to work was this:

Exception ex = null;
try {
    using (Sequence.Create()) {
        mock.Setup(m => m.Method1()).InSequence();
        mock.Setup(m => m.Method2()).InSequence();
        try {
            mock.DoStuff();
        }
        catch (Exception processingEx) {
            ex = processingEx;
        }
    }
}
catch (SequenceException) {
    if (ex != null) throw ex;
    throw;
}

This seems to work, my test failure includes the actual exception from DoStuff(). But it sure is ugly and destroys the clear intent of "normal" Sequence usage. Maybe there's a way to incorporate this into the library so users can leverage it without the ugly double-try-catch...

[stakx]

It suspect that solving this problem properly would mean that the using syntax to set up a sequence context would have to be given up.

One possible alternative would be to use Action lambdas instead:

Sequence.Create(() =>
{
    ...
    mock.DoStuff();
});

If the lambda throws, then the exception would bubble up (be rethrown by Sequence.Create) and the sequence setup wouldn't be verified. If the action succeeds, then the sequence setup is verified, which may lead to a SequenceException being thrown.

What do you think of that?

[erizzo]

Yes, that seems like a reasonable alternative for a situation like mine where a test can produce exceptions on its own. I would humbly suggest a better name for Create() in that case; something like Evaluate or Verify or similar.

[stakx]

Yes, I was thinking the same, there would probably be a name that fits better than Create. Verify sounds quite sensible to me. OTOH, it would be nice if the library's API were still recognizable if something like the above gets implemented. :)

[erizzo]

I'm thinking you could leave both usages intact, the new one would be useful for certain circumstances, while the other is familiar and (slightly) simpler).

[stakx]

Did some quick prototyping over there, but not yet completely satisfied with the result due to the additional line noise from the lambdas. The using syntax sure looks prettier.

I have also started wondering if it wouldn't be the better solution to separate the sequence setup and verify stages (with the mock exercising happening in-between).

var sequence = Sequence.Setup(() =>
{
    mock.Setup(…).InSequence();
});
… // exercise mock
sequence.Verify();

Which would obviously be quite a change, but solve the exception swallowing problem and probably a few others. I'll stop for now but will think about this some more later.

[dmch-dev]

@stakx separate sequence setup and verify stages is a good solution. Sequence checks are not working when I using the following block inside method which I'm testing.

try
{
     // some services called here which sequence need to verify
}
catch (Exception exception)
{
   // no throw; here. SequenceException just swallowed and test will not fail if methods call sequence is incorrect.
}

Please, redesign library concept as you mentioned.