Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have never written a unit test and I am really new to C#. I am attempting to test a method, am I testing for the right things here?

The Method

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Reflection;
using Orlean.Properties;

namespace Orlean.Helpers
{

    public enum SemverFormat
    {
        Full,
        Version,
        Major,
        Minor,
        Patch,
        Micro,
        Pre,
        Meta
    }

    public class AppInfo
    {
        // Grab the SemVer from assembly
        // You may pass any value 0-6 for id
        // SemverFormat.Full or nothing will return the entire SemVer string (X.Y.Z-pre+meta)
        // SemverFormat.Version will return the SemVer Major.Minor.Patch/Micro (X.Y.Z)
        // SemverFormat.Major will return the SemVer Major (X)
        // SemverFormat.Minor will return the SemVer Minor (Y)
        // SemverFormat.Patch or SemverFormat.Micro will return the SemVer Patch/Micro (Z)
        // SemverFormat.Pre will return the SemVer Pre data
        // SemverFormat.Meta will return the SemVer Meta data
        public static string SemverPart(SemverFormat? Option = SemverFormat.Full)
        {
            var attribute = Assembly.GetExecutingAssembly().GetCustomAttributes(false).OfType<SemverAttribute>().FirstOrDefault();
            string semver = (attribute == null) ? string.Empty : attribute.GetVersion;

            string[] delimiter = { "-", "+" };
            var semverArray = semver.Split(delimiter, StringSplitOptions.RemoveEmptyEntries);
            var versionArray = semverArray[0].Split('.');

            switch (Option)
            {
                case SemverFormat.Version:
                    return semverArray[0] ?? string.Empty;
                case SemverFormat.Major:
                    return versionArray[0] ?? string.Empty;
                case SemverFormat.Minor:
                    return versionArray[1] ?? string.Empty;
                case SemverFormat.Patch:
                case SemverFormat.Micro:
                    return versionArray[2] ?? string.Empty;
                // Determine if SemVer contains pre data and return it if it does
                case SemverFormat.Pre:
                    if (semver.Contains("-"))
                    {
                        return semverArray[1] ?? string.Empty;
                    }
                    else
                    {
                        return string.Empty;
                    }
                case SemverFormat.Meta:
                    if (semver.Contains("-"))
                    {
                        return semverArray[2] ?? string.Empty;
                    }
                    else
                    {
                        return semverArray[1] ?? string.Empty;
                    }
                case SemverFormat.Full:
                default:
                    return semver;
            }
        }
    }
}

The Unit Test

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Orlean.Helpers;
using System.Text.RegularExpressions;

namespace Orlean.Tests.Helpers
{
            [TestClass]
            public class AppInfoTests
            {

                public string stripCharacters(string word)
                {
                    return Regex.Replace(word, @"[^\w]", "");
                }

                [TestMethod]
                public void SemverPart()
                {
                    string fullParameter = AppInfo.SemverPart(SemverFormat.Full);
                    string fullNoParameter = AppInfo.SemverPart();
                    string version = AppInfo.SemverPart(SemverFormat.Version);
                    string major = AppInfo.SemverPart(SemverFormat.Major);
                    string minor = AppInfo.SemverPart(SemverFormat.Minor);
                    string patch = AppInfo.SemverPart(SemverFormat.Patch);
                    string micro = AppInfo.SemverPart(SemverFormat.Micro);
                    string pre = AppInfo.SemverPart(SemverFormat.Pre);
                    string meta = AppInfo.SemverPart(SemverFormat.Meta);

                    Assert.IsNotNull(fullParameter);
                    Assert.IsNotNull(fullNoParameter);
                    Assert.IsNotNull(version);
                    Assert.IsNotNull(major);
                    Assert.IsNotNull(minor);
                    Assert.IsNotNull(patch);
                    Assert.IsNotNull(micro);
                    Assert.IsNotNull(meta);

                    if (fullParameter.Contains("-")) 
                    {
                        Assert.IsTrue(pre.Length > 0);
                    }
                    else if (fullNoParameter.Contains("-")) 
                    {
                        Assert.IsTrue(pre.Length > 0);
                    }
                    else
                    {
                        Assert.IsTrue(pre.Length == 0);
                    }

                    Assert.IsTrue(fullParameter == fullNoParameter);
                    Assert.IsTrue(patch == micro);
                    Assert.IsTrue(stripCharacters(version) == major + minor + patch);
                    Assert.IsTrue(stripCharacters(version) == major + minor + micro);
                    Assert.IsTrue(stripCharacters(fullParameter) == major + minor + patch + stripCharacters(pre) + stripCharacters(meta));
                    Assert.IsTrue(stripCharacters(fullNoParameter) == major + minor + patch + stripCharacters(pre) + stripCharacters(meta));
                    Assert.IsTrue(stripCharacters(fullParameter) == major + minor + micro + stripCharacters(pre) + stripCharacters(meta));
                    Assert.IsTrue(stripCharacters(fullNoParameter) == major + minor + micro + stripCharacters(pre) + stripCharacters(meta));
                }
            }
        }
share|improve this question

2 Answers 2

up vote 5 down vote accepted

I will give a general overview of things you should change but definitely take a look at some of my answers related to unit testing I will put at the bottom.

Naming

I very much advise to use a naming scheme like [UnitOfWorkName]_[ScenarioUnderTest]_[ExpectedBehaviour]. I would give an example in your situation but I can't because your test is very unclear (red flag).

In a different situation it could be AddUser_WithoutFirstname_ThrowsArgumentException. When this test fails I know exactly what scenario doesn't work and I don't have to look at the actual code to figure out what it does; consider it a summary.

Small - Smaller - Smallest - A Good Unit test

Unit tests test one unit and they assert to one set of related post-conditions. They are also void of logic since all they should be able to do is perform an action and confirm the results.

You are testing AppInfo.SemverPart() with the different options. An sich this is fine, but they are multiple different actions so every single one of them should be separated into its own unit test. Yes, you will now have 10 very small unit tests; your manager will be impressed. You will also have a very clear overview in your test results window about what inputs worked and what ones didn't.

There are many benefits to splitting tests up in the smallest units possible but this should already be enough (this doesn't mean aggregated units aren't important; you should test all units: even those that are composed of other units).

Logic

No logic in your test! Instead use multiple tests to test the different scenarios. For reasons: see above.

Misc

I have no idea what SemverPart does or means; refactor this into a name in the form of [action][context] (for example: GetVersion()).


I would make stripCharacters private since the outside world doesn't need it (even if nobody calls it, it's good practice).

Reading

share|improve this answer
    
Ok I have read all but the last three so far and I have one question here... This method shouldn't ever throw an exception. So am I just testing to ensure that it's returning the expected results, or should I be testing something that does something as trivial as return a string at all? Are the Assert.IsNotNull statements even called for here? –  aaronmallen Jun 7 at 4:53
    
Also My pre data may not always have a value (hence the if statement) so how can I test for that? –  aaronmallen Jun 7 at 5:05
1  
The "expected result" should be the result of the code's actions, not of the assert statement. So GetSemver_NoParameter_ShouldReturn_EntireSemverVersion is the way to go. You are the one that determines with what data the tests are executed; you can give it the values you want it to have. Make tests that have each type of value. No, it is entirely okay to use the Assert.IsNotNull statements but I would also add Assert.AreEqual statements to compare the actual value of the response. –  Jeroen Vannevel Jun 7 at 12:28
1  
You have to abstract the datasource (in your case an assembly) by defining an interface which you can stub in your tests. In that stubbed implementation you can then hardcode the "assembly" values. More information here. –  Jeroen Vannevel Jun 7 at 17:23
1  
You'll need to create an interface so you can inject it (through setter or constructor) with an implementation for testing purposes. –  Jeroen Vannevel Jun 9 at 3:30

You should split each of the AppInfo.SemverPart() calls into different tests. The key point here is that when an Assert method fails, it throws an exception. This means that the test stops executing at this point without performing any other checks. This means if fullParameter is null, that is the only piece of information you get about what is wrong with your code. However, if you have lots of tests, it is possible that many of them will fail. This will provide you with a fuller picture of what is wrong with the code.


Assert.IsTrue(AA == BB);

is the same as

Assert.AreEqual(AA, BB);

There are many more methods in Assert that you can take advantage of. There is also CollectionAssert and StringAssert. The second will likely be useful for what you are working with here.


Your test should never use an if statement to decide what assertion to make. You want each test to have well defined inputs and well defined expected results.


Since you will end up with many test methods, how you name them is important. A good convention is

(Function being tested)_(Description of context or inputs)_(Expected result)

Don't use results from the function being tested to define what the expected result is. If there is one bug in you code, reusing values produced by the tested code can hide bugs. It is perfectly fine to use constant string as the expected value when comparing the actual result.


To make your code easier to test, it is best to pass in dependencies. The tests you have right now are just on the basic structure because you don't the actual version string you function is working on. It is using whatever is set for the current assembly.

It would be better to adjust the function api to take in the version string and the ServerFormat enum. This will make it much easier to write test like this:

[TestMethod]
public void SemverPart_Major_FirstPart()
{
  Assert.AreEqual("1", AppInfo.SemverPart("SOME INPUT WHERE MAJOR IS 1", SemverFormat.Major)
}

This leave you actual code looking like

public static string AppInfo.SemverPart(SemverFormat format)
{
  string version = //get version from assembly;
  return AppInfo.SemverPart(version, format);
}

public static string AppInfo.SemverPart(string version, SemverFormat format)
{
  //actual splitting and logic to get requested format.
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.