6

I'm trying to follow the Arrange-Act-Assert pattern when writing unit test and I got to a point where I'm confused on which approach will be better. I'm using xUnit and my first approach to the problem was:

//Arrange
int key = 1;
string value = "X";

//Act
board.Add(key, value);
var result = Assert.Throws<ArgumentException>(() => board.Add(key, value));

//Assert
Assert.IsType<ArgumentException>(result);

and my second approach is:

int key = 1;
string value = "X";

board.Add(key, value);

Assert.Throws<ArgumentException>(() => board.Add(key, value));

which is a better approach?

Edit: Blogged about this on wp.me/p4f69l-3z

pmbanugo
  • 303
  • 3
  • 12
  • In your first example, you have redundant assertions. You're already asserting that you've thrown an `ArgumentException`. If you're examining the exception object, it should be to make sure that a message is set as expected. As an aside, it looks an awful lot like you're testing the underlying implementation of a `Dictionary`. – 48klocs Jun 05 '14 at 21:06
  • I'd recommend jimmy_keen's answer and considering [FluentAssertions](http://fluentassertions.codeplex.com/documentation) for asserting eceptions (or more if you love their fluent syntax, as I do). – EagleBeak Jun 05 '14 at 21:59
  • 1
    going with the AAA pattern @jimmy_keen answer will be my preferred choice but, if I just want it Simple Sweet and Short I'll go with my second approach without worrying about AAA. – pmbanugo Jun 05 '14 at 22:05
  • 1
    AAA, like everything else, is a guideline. Its your engineering superpowers that decide when guidelines should be ignored. This might be one of those cases. – Ritch Melton Jun 05 '14 at 22:41
  • OT: I prefer the NUnit Assert.That/Assert.Throws to Fluent stuff. Fluent is great if you have a lesser framework that doesn't have the NUnit syntax, such as xUnit and MSTest. – Ritch Melton Jun 05 '14 at 22:43

5 Answers5

7

Your first .Add call should be really part of arrange. Treat it as precondition/setup for act. Other than that you can wrap act in Action to make it read better:

//Arrange
int key = 1;
string value = "X";
board.Add(key, value);

//Act
Action addingSameKeySecondTime = () => board.Add(key, value);

//Assert
Assert.Throws<ArgumentException>(addingSameKeySecondTime)

FluentAssertions library mentioned in comments makes such asserts even more sentence-alike:

int key = 1;
string value = "X";
board.Add(key, value);

Action addingSameKeySecondTime = () => board.Add(key, value);

addingSameKeySecondTime.ShouldThrow<ArgumentException>();
Theo Lenndorff
  • 4,556
  • 3
  • 28
  • 43
k.m
  • 30,794
  • 10
  • 62
  • 86
  • You don't need `FluentAssertions` it's easily readable/understandable without it. – pmbanugo Jun 05 '14 at 22:23
  • 1
    with the approach of using a delegate for the `Act` section, it doesn't actually call the code under test. The invocation of the method is done on the `Assert.Throw` which makes me stick with the approach of calling the code like this `var result = Assert.Throws(() => board.Add(key, value));` and asserting afterwards `Assert.IsType(result);`, just like I did in the example in my question – pmbanugo Jun 05 '14 at 22:46
  • @pmbanugo What's your problem with FluentAssertions? It's build for this. – Dennis Doomen Jun 19 '14 at 18:18
  • @Dennis Doomen I have no problem with FluentAssertions. It's nice and makes your test read like a sentence, but you don't necessarily need it to make your test readable. I don't like adding too many libraries to my projects just to beautify my code. I like to Keep It Simple and Sweet. Using `Assert` tells me what the code does. – pmbanugo Jun 20 '14 at 10:41
2

I usually take the following approach

// Arrange
int key = 1;
string value = "X";
board.Add(key, value);

// Act & Assert
Assert.Throws<ArgumentException>(() => board.Add(key, value));

this is the approach taken in ASP.NET MVC (e.g. https://aspnetwebstack.codeplex.com/SourceControl/latest#test/Common/PrefixContainerTest.cs)

Patrick McDonald
  • 64,141
  • 14
  • 108
  • 120
1

For me, source code should be self-descriptive, so AAA comments are more like workaround and might not provide enough flexibility. Check this library: Heleonix.Testing You can write tests as in JavaScript Jasmine/Jest styles in both Given/When/Then and AAA forms.

ErTR
  • 863
  • 1
  • 14
  • 37
Hennadii
  • 149
  • 1
  • 6
0

I'd say your second example is better. Assert.Throws will pass/fail the test so there's no reason to get it's result and assert on that. When I'm writing 'will throw' tests I keep it on one or two lines:

[Test]
public void SomeMethod_NullSomething_ShouldThrow() {
    var something = MakeTarget();

    Assert.Throws<ArgumentNullException>(() => something.SomeMethod(null));
}
Neil Smith
  • 2,565
  • 1
  • 15
  • 18
0

Actually there is no good answer on asserting exception... It's like testing events, except that they interrupt the code flow. Let's say you would test the add event (I'm using NUnit):

// Arrange
int key = 1;
var eventFired = false;
board.Added += (boardItem) => {
    eventFired = boardItem.key == key;
};

// Act
board.Add(key, "X");

// Assert
Assert.That(eventFired, Is.True);

It's the same thing when you test an exception:

// Arrange
int key = 1;    
var exceptionRaised = false;
board.Add(key, "X");

// Act
try {
    board.Add(key, "X");
}
catch(InvalidOperationException ex) {
    exceptionRaised = true;
}

// Assert
Assert.That(exceptionRaised, Is.True);

So the Assert.Throws can be used for convenience but it doesn't fit with AAA style. But remember AAA is not mandatory to write good tests, the most important is to have your tests easy to understand.

Elias Platek
  • 1,074
  • 1
  • 9
  • 16
  • 2
    I disagree. Using the Assert.Throws in NUnit, or the Fluent assertions with xUnit, MSTest is a much better approach than flagging a bool. – Ritch Melton Jun 05 '14 at 22:40
  • 1
    I am not saying that it's better to use a boolean, just that Assert.Throws mixes act and assert step, which was the actual question ;) – Elias Platek Jun 06 '14 at 17:50
  • "Actually there is no good answer on asserting exception.." - I disagree with that statement. Assert.Throws or Fluent is a good answer. – Ritch Melton Jun 06 '14 at 19:31
  • Well, my opinion is that using exceptions is bad, [and I'm not the only one](http://ericlippert.com/2014/03/03/living-with-unchecked-exceptions/). Again, Assert.Throws does not fit with AAA testing style – Elias Platek Jun 06 '14 at 22:17
  • It looks like the most upvoted answer is in AAA style and I have no idea what checked exceptions have to do with this post. – Ritch Melton Jun 07 '14 at 01:53
  • 1
    Because when I'm declaring a lambda expression I don't consider to be acting anything – Elias Platek Jun 07 '14 at 08:07
  • Calling a function isn't acting? – Ritch Melton Jun 07 '14 at 17:34
  • 1
    That's right, calling a function is acting. And the call is done inside Assert.Throws ;) Again, I don't say it's bad to use this function, I just say that testing exception cannot be compliant with AAA testing model – Elias Platek Jun 07 '14 at 18:22
  • 1
    Try to debug the code of jimmy_keen, you will see that the code of the lambda is not executed before you reach the assert step. My code is just a simplified version of this function, but it separates those two logical steps. Of course I do not recommend to use the code of my answer in production, it was just here to explain that (guess what:) Assert.Throws mixes act and assert step. – Elias Platek Jun 07 '14 at 18:58
  • I agree that if you intend to follow AAA strictly, then `Assert.Throws` is not the way. **Declaring** an anonymous function with the code under test is definitely not the same thing as **running** the code under test. The act is deferred and as such you shouldn't comment the lambda line as the act, you're not testing if the closure is constructed and assigned. – acelent Nov 11 '14 at 22:36
  • On the other hand, I'm not a fan of boolean flags either, so the provided example should capture the exception and check for not null, and that way, you can easily add further assertions if you ever need to. But that's beyond the point. – acelent Nov 11 '14 at 22:38