9
\$\begingroup\$

I am just starting to implement unit testing into my project and familiarizing myself with NUnit. I have added unit tests for encryption/decryption methods that I have implemented, and am looking for more things to start testing for in my existing code before moving forward.

The most basic classes that I have are my IValueConverters, so I thought it would be a good place to start.

Here is my first test:

[TestFixture]
public class ConverterUnitTest
{
    [TestCase]
    public void BooleanToVisibilityConverterTest()
    {
        var btvConverter = new BooleanToVisibilityConverter();

        Assert.That(btvConverter.Convert(true, typeof(Visibility), null, "en-us").Equals(Visibility.Visible));
        Assert.That(btvConverter.Convert(false, typeof(Visibility), null, "en-us").Equals(Visibility.Collapsed));
    }
}

As you can see, this test is just ensuring that true returns Visibility.Visible and false returns Visibility.Collapsed. What I am wondering is, does it make sense to have multiple Asserts in one test, or should I split them up into multiple tests even though they are both testing the same converter? Also, is it a good practice to test converters?

\$\endgroup\$
  • 1
    \$\begingroup\$ Would it be relevant to include the code for BooleanToVisibilityConverter to show what it is that I am trying to test? \$\endgroup\$ – dub stylee Apr 22 '15 at 2:25
  • \$\begingroup\$ BooleanToVisiblityConverter is already part of WPF, which means, you should not write your own one: msdn.microsoft.com/en-us/library/… Use the existing one & do not write unit tests against Microsoft one. \$\endgroup\$ – Erti-Chris Eelmaa Apr 22 '15 at 14:56
  • \$\begingroup\$ Thanks for the info @Erti-ChrisEelmaa, I didn't mention that this is a WinRT application. Does the built-in BooleanToVisibilityConverter work with WinRT as well? \$\endgroup\$ – dub stylee Apr 22 '15 at 16:15
6
\$\begingroup\$

This is a good unit test. It may seem like a trivial case to test a boolean response, but wrapping this in a test ensures that you always get the correct response. What if you decide to refactor the internals of BooleanToVisibilityConverter down the road? With this test, you can always be confident that it's behaving correctly.

To me, the two assertions look like they belong together. As a matter of style, you could split the test up so that you have two tests with a straight AAA (Arrange, Act, Assert) pattern - one for true, one for false. In my opinion, that would be a little excessive. I like a test that looks like this because it is clear and quick.

If you were writing a test of a converter that had many possible outputs and multiple input conditions (for example), then you definitely should consider splitting it up and testing each condition separately. For now, you're on the right track.

\$\endgroup\$
  • \$\begingroup\$ I also added a null test, which is equivalent to false in that it should (does) return Visibility.Collapsed. Looking at some of my other converters, it does look like some of them will be better split up into multiple tests depending on how many possible inputs/outputs I expect. Thanks for the tips. \$\endgroup\$ – dub stylee Apr 22 '15 at 2:42
  • \$\begingroup\$ There's definitely a tipping point/code smell where I split off a test into multiple tests. Unit tests should be lean and mean! No worries, glad to help. \$\endgroup\$ – Nate Barbettini Apr 22 '15 at 2:45
3
\$\begingroup\$

Since you're using nUnit, you could actually break them out into separate tests by utilizing the TestCase attribute more effectively:

[TestFixture]
public class ConverterUnitTest
{
    [TestCase(true, Visibility.Visible)]
    [TestCase(false, Visibility.Collapsed)]
    public void BooleanToVisibilityConverterTest(bool value, Visibility expected)
    {
        var btvConverter = new BooleanToVisibilityConverter();

        Assert.That(btvConverter.Convert(value, typeof(Visibility), null, "en-us").Equals(expected));
    }
}

Otherwise, like Nate mentions, I think the test is pretty good.

I noticed you mentioned in comments that you need to test the null value, but this can be done by tweaking the first parameter to a nullable boolean:

[TestFixture]
public class ConverterUnitTest
{
    [TestCase(true, Visibility.Visible)]
    [TestCase(false, Visibility.Collapsed)]
    [TestCase(null, Visibility.Collapsed)]
    public void BooleanToVisibilityConverterTest(bool? value, Visibility expected)
    {
        var btvConverter = new BooleanToVisibilityConverter();

        Assert.That(btvConverter.Convert(value, typeof(Visibility), null, "en-us").Equals(expected));
    }
}
\$\endgroup\$
  • \$\begingroup\$ I like this use of the TestCase attribute. It makes everything much more readable, and you know the actual and expected values for each test, without repeating the same code over and over. I ended up removing the null test, since that value shouldn't really be passed to the converter in the first place. Thanks for the additional information. \$\endgroup\$ – dub stylee Apr 23 '15 at 17:33
2
\$\begingroup\$

A word of caution when learning about Unit Tests. Don't over-test! IValueConverter is a good place to start with tests, and I believe you did a good job. In your comment to Nate you mentioned that you added a null test. This to me is a small form of over-testing. I'm going to assume you are using/learning about MVVM with WPF since it is most common to use that class in WPF. That particular class that you are writing about is going to bind to a boolean (not a nullable boolean) booleans initialize to false by default. So it would be a compilation error that would show the case where null can happen. That being said I would possibly remove the null check.

Now for your test.

    Assert.That(btvConverter.Convert(true, typeof(Visibility), null, "en-us").Equals(Visibility.Visible));
    Assert.That(btvConverter.Convert(false, typeof(Visibility), null, "en-us").Equals(Visibility.Collapsed));

when you pass in a parameter to a function, a 3rd party looking at your code will assume that need said parameter. I'm speaking directly to "en-us". Does the culture string affect your visibility? If it was set to some other culture would I get different results? Your tests don't answer that. If that parameter is not used then pass in null.

Another thing is about your Assert. You are using the the boolean overload for Assert.That(...). When that test fails it will tell you "Expected true but what false". Not very helpful IMO. It's not helpful because without clicking on the Stacktrace in the TestExplorer (which isn't always available) you don't know which of those 2 asserts failed. There are a few ways to make your tests give you more explicit error messages that tell you what you want to know. One is that you can use a string after your test that gets shown when your test fails.

    Assert.That(btvConverter.Convert(true, typeof(Visibility), null, "en-us").Equals(Visibility.Visible), "The converter did not return Visible when it should have");
    Assert.That(btvConverter.Convert(false, typeof(Visibility), null, "en-us").Equals(Visibility.Collapsed), "The converter did not return Collapsed");

Another option is to use the IResolveConstraints overload with NUnit. They help a little bit as well.

    Assert.That(btvConverter.Convert(true, typeof(Visibility), null, "en-us"), Is.EqualTo(Visibility.Visible));
    Assert.That(btvConverter.Convert(false, typeof(Visibility), null, "en-us"), Is.EqualTo(Visibility.Collapsed));

That will at show "Expected <Visibility.Visible>, but found <Visibility.Collapsed"

that helps some too, and you don't have to add a string always.

And the last option (the one I prefer) is to use FluentAssertions. It's a personal preference, but what i like is that sometimes the TestFrame work of choice doesn't work and you end up having to switch. Using FluentAssertions you would only have to change the attributes for your test methods and test classes. Plus I like the way it reads:

    [Test]
    public void Test()
    {
        IValueConverter converter = new BooleanToVisibilityConverter();
        converter.Convert(true, null, null, null).Should().Be(Visibility.Visible);
        converter.Convert(false, null, null, null).Should().Be(Visibility.Hidden);
    }

In the fail for the second Assertion it says Expected object to be Hidden, but found Collapsed.

Now granted this last part is personal preference. Find your niche and stay consistent.

EDIT

So based on your mention of using WinRT and the mention of compilation errors when passing in a null for a string parameter leads me to believe that your best bet is to either use string.Empty or my preference is to see a field with a meaning behind its name: unusedLanguage or something to that effect. Then when you use it in your test it reads clearly btvConverter.Convert(true, null, null, unusedLanguage). Granted it is a little bit of a workaround and looks weird, but at least it states its intentions clearly.

and now that I've typed out btvConverter a few times it is annoying me. using the first letters of the converters name is silly. your test is specifically for a Boolean to visibility converter. converter is is clear enough of a name in the context.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your feedback Robert. I initially tried passing in null to the last parameter of Convert(), but got a compilation error. I noticed the different in parameters compared to the WPF version which expects a CultureInfo parameter. I like your use of the IResolveConstraints because those messages are far more useful than "pass" or "fail". \$\endgroup\$ – dub stylee Apr 22 '15 at 16:24
  • \$\begingroup\$ As far as the "en-us", since the language parameter isn't really relevant to these tests, should I just pass in string.Empty or something like that, since it doesn't allow me to use null? \$\endgroup\$ – dub stylee Apr 22 '15 at 16:25
  • \$\begingroup\$ I'm not sure how you had a compilation error. The above test was ran and worked as intended. And CultureInfo is a class which automatically is also an object, and objects can be null. I wonder what is different between your code and mine that makes mine compile. (FYI I used the WPF BooleanToVisibilityConverter built in) \$\endgroup\$ – Robert Snyder Apr 22 '15 at 17:12
  • \$\begingroup\$ I believe that the difference is WinRT. The IValueConverter.Convert method takes looks like this: object Convert(object value, Type targetType, object parameter, string language) rather than the last parameter being CultureInfo as in the WPF version. \$\endgroup\$ – dub stylee Apr 22 '15 at 17:14
  • 1
    \$\begingroup\$ Excellent. I will try again to use null when I get home from work tonight. I am pretty sure that it didn't let me though, hence the en-us. If it doesn't allow null I will make a const string that will be more informative that having an unused language parameter. Thanks again for the extra information. \$\endgroup\$ – dub stylee Apr 22 '15 at 17:27
1
\$\begingroup\$

I find it's useful to define a few variables when I write my tests. These particular variables always have the same names, expected and actual. Doing this makes it really very easy to see what's being tested in our tests. Let's see what happens when we apply this to your code.

var actual = btvConverter.Convert(true, typeof(Visibility), null, "en-us");

Assert.That(actual.Equals(Visibility.Visible));
Assert.That(btvConverter.Convert(false, typeof(Visibility), null, "en-us").Equals(Visibility.Collapsed));

That's starting to look better, but wait. That second call is different than the first. Can we comfortably call it's result actual as well? I don't believe so. It's being called with different arguments. You have two different tests cases here and should test them separately.

I showed you this method because it is okay to have a couple of closely related asserts in a tests, but you should be able to get the results into a single result variable like actual.

\$\endgroup\$

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.