17
\$\begingroup\$

I am relatively new to TDD and am trying to adopt better names for my tests.

I wrote the following tests abruptly and have since refactored them to the best of my ability, but despite my best efforts, the test names are still (mostly) abysmal.

I am here today to ask that you review my test names.

[Test]
public void Edit_ReturnsCorrectView()
{
    var actual = controller.Edit("dynamic-contagion-part-one") as ViewResult;

    Assert.NotNull(actual);
    Assert.That(actual.ViewName, Is.EqualTo("Edit"));
}

[Test]
public void Edit_ReturnsCorrectModel()
{
    var viewResult = (ViewResult) controller.Edit("dynamic-contagion-part-one");
    var actual = viewResult.Model as PostInputModel;

    Assert.NotNull(actual);
    Assert.That(actual.Title, Is.EqualTo("Dynamic contagion, part one"));
}

[Test]
public void Edit_NonExistentPost_ReturnsNotFound()
{
    var actual = controller.Edit("non-existent-slug") as HttpNotFoundResult;

    Assert.NotNull(actual);
    Assert.That(actual.StatusDescription, 
        Is.EqualTo("You cannot edit a post that does not exist."));
}

[Test]
public void Edit_Post_ReturnsCorrectView()
{
    var actual = controller.Edit(post) as ViewResult;

    Assert.NotNull(actual);
    Assert.That(actual.ViewName, Is.EqualTo("Edit"));
}

[Test]
public void Edit_EditsPostContent()
{
    post.Content = "arbitrary content.";
    controller.Edit(post);

    var amendedPost = 
        repository.Object.Posts.First(p => p.Slug == "dynamic-contagion-part-one");
    Assert.That(amendedPost.Content, Is.EqualTo("arbitrary content."));
}

[TestCase("foo", ExpectedResult = "foo")]
[TestCase("foo\r\nbar", ExpectedResult = "foo")]
public string Edit_EditsPostSummary(string content)
{
    post.Content = content;
    controller.Edit(post);

    var amendedPost = 
        repository.Object.Posts.First(p => p.Slug == "dynamic-contagion-part-one");
    return amendedPost.Summary;
}

[Test]
public void Edit_EditsPostSlug()
{
    post.Title = "Hello, World";
    controller.Edit(post);

    repository.Object.Posts.First(p => p.Slug == "hello-world");
}

[Test]
public void Edit_Post_NonExistentPost_ReturnsNotFound()
{
    var model = 
        new PostInputModel {Title="Non Existent Slug",Slug="non-existent-slug"};

    var actual = controller.Edit(model) as HttpNotFoundResult;

    Assert.NotNull(actual);
    Assert.That(actual.StatusDescription,
        Is.EqualTo("You cannot edit a post that does not exist."));
}

[Test]
public void Edit_InvalidModel_ReturnsCorrectModel()
{
    controller.ModelState.AddModelError("", "");
    var model = new PostInputModel();

    var actual = controller.Edit(model) as ViewResult;

    Assert.NotNull(actual);
    Assert.That(actual.ViewName, Is.EqualTo("Edit"));
    Assert.AreEqual(model, actual.Model);
}


[Test]
public void Edit_OccupiedSlug_ReturnsCorrectView()
{
    var model = new PostInputModel
    {
        Title = "Lowering in language design, part two",
        Content = "arbitrary content."
    };

    var actual = controller.Edit(model) as ViewResult;

    Assert.NotNull(actual);
    Assert.That(
        controller.ModelState[""].Errors[0].ErrorMessage,
        Is.EqualTo("You have previously published a post with this title. " +
                   "Please choose another one."));
}

I welcome all feedback regarding this segment of code but in an attempt to be more specific, something that makes me particularly nauseous are these test names:

  • Edit_ReturnsCorrectView
  • Edit_Post_ReturnsCorrectView

Here I am trying to differentiate which action method I am testing according to the HTTP verb which is clearly ambiguous and generally yucky.

I am also concerned about the Edit_EditsX convention that I have developed and wonder if there is a better approach to testing that data in my (fake) repository is updated accordingly.

\$\endgroup\$
10
\$\begingroup\$

Let me start out by saying that your test names are actually decent. You have already come to the conclusion that a good test name is one that can convey what is being tested without requiring someone to actually read the test.

As with many things, there are three important pieces of information about what a test is doing:

  1. What is the thing being tested?
  2. What is the input to the thing being tested? / In what context is the thing being tested?
  3. What is the expected result?

If I see a test is failing and the name provides all this information, I can start formulating an idea of what the issue might be without doing more than seeing the test report.

There are a few different theories about how these three things should be ordered based on what the person feels is the focal point. But I prefer the order introduced by Roy Osherove, which matches the order I listed above. This provides us with:

[Test]
public void UnitOfWork_StateUnderTest_ExpectedBehavior()
{
  // ...
}

You don't have to be concerned with the name becoming very wordy. These are test cases and don't need to follow the same naming conventions you would use for methods in your production code. Artificially restricting yourself to HTTP verbs is preventing you from accurately describing the context that is being tested.

Additionally, try to stay away for using "correct value" as the description of the expected result. If I didn't write the code or tests, I likely don't know what the expected result is. More importantly, I won't know why that is the expected value.

Bellow are a few examples of your test case names converted into this style:

  • Edit_ReturnsCorrectView -> Edit_SomeSlugName_ViewNameEdit
  • Edit_Post_ReturnsCorrectView -> Edit_SomePost_ViewNameEdit
  • Edit_EditsPostContent -> Edit_PostWithAlteredContent_PostsContentUpdated

The use of Some is also taken from Roy. It is meant to indicate that there is nothing special about this value and a different arbitrary value should have the same result.


Other notes:

var actual = controller.Edit(model) as HttpNotFoundResult;
Assert.NotNull(actual);

and

var actual = controller.Edit(model);
Assert.NotNull(actual);

Are two very different things. The second case uniquely states that the result must not be null. However, the first case can fail the null check for two different reasons (the result is null or the result is of the wrong type). I suspect you are using this just ensure that the correct type is returned. If that is the case, you should be able to use the testing framework to verify the result is the correct type instead of potentially introducing a null.


Most of your tests are checking that some text field contains an expected sentence. This type of test can be very fragile. If the message displayed to the user is reworded or translated to a different language, the test would start failing. Do you actually care about the wording of the error message or is knowing that a HttpNotFoundResult instance was returned sufficient? Is there an IsError property you could test instead?


Your tests are using a mock repository that is filled with some sample data. There is nothing wrong with this. The one thing I would change would be to move all of the hard coded strings from the tests into the class that produces the mock as constants. This way, all of the data, is defined in one places and you don't have to edit this test class if a different test class requires the repository to contain something slightly different.

\$\endgroup\$
  • 1
    \$\begingroup\$ Let me start by saying that your "other notes" are very useful, thank you. Your actual answer is useful too but I must say, I am struggling to convey Roy's naming convention in my own code. Could you please supply me with a handful of examples for my code listing? +1 \$\endgroup\$ – User 12345678 Jul 22 '14 at 14:34
  • \$\begingroup\$ @ByteBlast: I added a few examples and one more other note. \$\endgroup\$ – unholysampler Jul 22 '14 at 14:50

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.