Closed salfab closed 2 years ago
Can you provide sample code of where this would be useful? I'm struggling to understand this use case.
The scenario where this would be useful is when using inheritance of contexts.
Whenever the Act
part of the test is common, but the contexct and the outcomes differs, I use class inheritance to re-use the parts of the contexts that are common.
I can build you a multi-class sample if you like, but perhaps the following pseudo-code explanation will be sufficient.
Let's imagine I want to test a NamingConventionsService
that can turn a specified string into a camelCase string. The service will contain a method called ToCamelCase
and will accept a parameter called symbolName
The Because of
part of code would be located in a class named When_ToCamelCase_is_called
ToCamelCase
can either result in
NullArgumentException
if the specified string is nullInvalidOperationException
if the specified argument is in all-caps.to implement that, we will inherit three times from When_ToCamelCase_is_called
With_null_specified_symbolName
With_all_caps_specified_symbolName
With_pascal_cased_specified_symbolName
Now : let's imagine that in the class With_pascal_cased_specified_symbolName
, the arrange is not written correctly : one of the mocked objects is not correctly setup.
In the case of Moq with strict behavior, when there is a missing setup, an exception of type MockException
is thrown.
Of course, we really need this exception to show up in the test runner.
However, because of the way the exception handling works in MSpec, we will not see the exception show up in the results of the tests. Instead, it will be swallowed by Catch.Exception
, and the test will be red when asserting the returned value, which will most likely will be null because the exception bubbled up before ToCamelCase
could return a value.
Right now, MSpec allows us to swallow all exception (or all exceptions of a certain - single - type) and assign them in a property that can later be asserted. It is the responsibility of the developer to nmake sure the exception is empty, however, it is a tedious and repetitive task that many developers will forget to do :
Developers don't want to assert that no exceptions was thrown : they want the thrown exceptions to show up in the test runner and make the test red in the process, with a useful stack trace.
This issue tries to do just that, by allowing the developer to mark the tests ( It Xxx
) that are expected to throw an exception. All other tests are expected to have an empty ThrownException
property.
Are these explanations sufficient, or do you need me to take the time to build a working example. (probably not before friday, though)
Thanks, I'll go over this.
Just to set expectations here, I have a lot of work to do to stabilise what we already have, fix bugs, and push out a 1.0 release before we can consider adding new features. I'd say this was a v1.1 feature as it changes the API.
Sure, let me know if I can help in any way
Ok, I think I understand what you're facing, unfortunately I don't agree with your solution, either by using a Catch.Strict
or by introducing new delegates.
Consider the following class under test:
public class NamingConventionService
{
public string ToCamelCase(string symbolName)
{
if (symbolName == null)
throw new ArgumentNullException();
throw new InvalidOperationException();
}
}
The test you are proposing is (correct me if I'm wrong) something like below, where the Because
is shared by both the test that checks for exceptions, and for the return result.
class when_to_camel_case_is_called
{
static NamingConventionService service;
static string symbol_name;
static string result;
static Exception exception;
Establish context = () =>
service = new NamingConventionService();
Because of = () =>
exception = Catch.Exception(() => result = service.ToCamelCase(symbol_name));
class With_null_specified_symbolName
{
Establish context = () =>
symbol_name = null;
// This passes
It should_throw_arg_null = () =>
exception.ShouldBeOfExactType<ArgumentNullException>();
}
class With_pascal_cased_specified_symbolName
{
Establish context = () =>
symbol_name = "mySymbol";
// This fails with result != "MySymbol", but the InvalidOperationException is swallowed by the Catch
It should_return_pascal_case = () =>
result.ShouldEqual("MySymbol");
}
}
I don't think this is the way to approach this particular test though, because you are mixing up the Act
portion of the test with parts of the Assert
(ie exception testing and result testing). FWIW I don't agree with the current Catch.Only<>
method in Mspec either, as this should be part of the assert, not the action. Xunit for instance has deliberately not included such a feature either.
Instead, it would be better to structure the test like the below, where the actions are clearly defined and exception testing is separate and distinct
class when_to_camel_case_is_called_better
{
static NamingConventionService service;
static string symbol_name;
Establish context = () =>
service = new NamingConventionService();
class when_invalid_arguments_are_passed
{
static Exception exception;
Because of = () =>
exception = Catch.Exception(() => service.ToCamelCase(symbol_name));
class With_null_specified_symbolName
{
Establish context = () =>
symbol_name = null;
// This passes
It should_throw_arg_null = () =>
exception.ShouldBeOfExactType<ArgumentNullException>();
}
}
class when_testing_return_value
{
static string result;
Because of = () =>
result = service.ToCamelCase(symbol_name);
class when_passed_lower_camel_case
{
Establish context = () =>
symbol_name = "mySymbol";
// Throws InvalidOperationException as expected
It should_return_pascal_case = () =>
result.ShouldEqual("MySymbol");
}
}
}
I believe the above is clearer in intent than providing more scaffolding in the framework to deal with exceptions.
Hi Robert, thank you for your elaborate response.
I'm going to jump straight to your proposed structure.
Unfortunately, I see a couple of problems with the structure in your example.
One basic problem with the check of the type of the exception is that the stack trace is pointing to the assert, and not to the code that threw the exception.
Another issue is when using inheritance to re-use contexts.
In my case, I would like to have the when
class described only once, and used by both scenarios that throw exceptions and scenarios that don't throw exceptions.
The reason is simply because in more advanced scenarios, the cases where exception would be thrown would be more subtle than just passing a null argument. It could be a combination of arranges that trigger this, and if you have a 3-levels deep hierarchy of contexts, you don't want to have to copy-paste the whole hierarchy of contexts just because one single line of the Because
is different.
You mentioned
I don't think this is the way to approach this particular test though, because you are mixing up the Act portion of the test with parts of the Assert (ie exception testing and result testing).
I'm not sure I understand what you mean by "mixing up the Act portion of the test with parts of the Assert". Ideally, what would be great is if the Act
were the same, whether an exception is thrown or not. (because some inherited contexts might lead to one or the other)
Ideally still. it would be done with no mention of exceptions handling, so whether you are doing result-assertions or exception-assertions, the content of the Because
would not have to be different, and therefore, no mixup between act and arrange.
(I suppose that could be done, within an abstract base class, and that could be totally transparent for the user.)
did I manage to convey that need correctly, or do you need me to build a concrete example ?
Of course, I am open to any other implementation that would cover that requirement. My proposition of an implementation was just the least invasive I could think of at the time.
Cheers
Problem 1: the stack trace is pointing to the assert, and not to the code that threw the exception
This isn't a problem IMO. You expected something to be of a certain type, it wasn't, so the test fails. You'll notice that Record.Exception
in xUnit works exactly the same way.
Problem 2: complex nested contexts
Again, I feel there are better ways to solve the problem of shared code using base classes, static methods or test helpers. If your contexts are complicated to construct, it might be because the code under test is doing too much and should be refactored in some way so that testing is more focused. Another hacky way of doing this would be to design your own try/catch
mechanism that expected certain exceptions and let others bubble through and use that in your Act
, but I don't see that as being the optimal route as it violates AAA
testing.
Happy to review code if you want to post your tests.
Regarding Problem 1, you mentionned:
You expected something to be of a certain type, it wasn't, so the test fails.
I don't disagree with you.
however, in a single context, you will have many tests. for instance 20.
With that approach, 1 out of the 20 tests will have a message that will actually help you ("the exception is not of the right type) the 19 others will throw a NullReferenceException
on the line
It should_return_a_camelCase_string = () => result.Should().Be(...);
I don't want to see a NullReferenceException
here, just because result
was left null. I want to see the original exception that caused result
to be null. That is, a MockException
.
What are your thoughts on this take on problem 1?
Regarding problem 2, I will try to build a minimal example to illustrate my point.
hello again, Robert. I built a little showcase that illustrates the problem.
I posted it on github right here :
https://github.com/salfab/showcase-of-mspec-problems-with-exceptions
Please read the readme file to walk you through the issues I am facing.
Cheers
I should note that I am open to changing the implementation.
One way we could tackle this would be to have an attribute [ExpectsException]
. This attribute could be applied on a context.
It could only be applied to contexts that inherit from a ContextBase
abstract class
When applied, the tests in the current class (not the inherited classes, though) would use a Catch.Exception
around the Because
defined in the Act
context.
By doing so, I believe it would be an elegant fix to my problem, and also, provide a better experience for Exceptions testing altogether.
What are your thoughts ?
Currently, Exceptions that are caught in a catch block are swallowed and returned to the caller of
Catch.Exception
. the return type ofCatch.Exception
isException
Given a test that doesn't assert the exception is not null. it will succeed, even if an unexpected exception is thrown - for instance, if a Mock is not setup in struct mode with a moq mock.
In order to provide an additional behavior that prevents it, we need a
Catch.Strict()
and aCatch.OnlyStrict<T>()
method. These two methods will not return anException
, but instead will wrap it in an object, for instance,StrictExceptionObserver
. ThisStrictExceptionObserver
will expose anException
property. At the end of the test, theStrictExceptionObserver
will check that theException
property's getter has been accessed, and if not, the exception will be rethrown, and will fail the test.