Is this enough for unit testing a basic controller?

Question

My pet project, is a community driven lyrics archive, it is still a work in progress, and all code is open sourced on GitHub.

I have a local git branch tests/add-controller-tests where I wish to add some unit tests on my controllers. I have purposefully kept my controllers basic, for example here is my HTTP GET Index action on the HomeController:

namespace Bejebeje.Mvc.Controllers
{
 using System.Diagnostics;
 using System.Threading.Tasks;
 using Bejebeje.Models.Lyric;
 using Bejebeje.Services.Services.Interfaces;
 using Microsoft.AspNetCore.Mvc;
 using Models;
 public class HomeController : Controller
 {
 private readonly IArtistsService _artistsService;
 private readonly ILyricsService _lyricsService;
 public HomeController(
 IArtistsService artistsService,
 ILyricsService lyricsService)
 {
 _artistsService = artistsService;
 _lyricsService = lyricsService;
 }
 public async Task<IActionResult> Index()
 {
 IndexViewModel viewModel = new IndexViewModel();
 viewModel.Lyrics = await _lyricsService
 .GetRecentLyricsAsync();
 viewModel.FemaleArtists = await _artistsService
 .GetTopTenFemaleArtistsByLyricsCountAsync();
 return View(viewModel);
 }
 [ResponseCache(Duration = 0, Location = ResponseCacheLocation.None, NoStore = true)]
 public IActionResult Error()
 {
 ErrorViewModel viewModel = new ErrorViewModel
 {
 RequestId = Activity.Current?.Id ?? HttpContext.TraceIdentifier,
 };
 return View(viewModel);
 }
 }
}

And here is my unit test (just one):

namespace Bejebeje.Mvc.Tests.Controllers
{
 using System.Collections.Generic;
 using System.Threading.Tasks;
 using Bejebeje.Models.Artist;
 using Bejebeje.Models.Lyric;
 using FluentAssertions;
 using Microsoft.AspNetCore.Mvc;
 using Moq;
 using Mvc.Controllers;
 using NUnit.Framework;
 using Services.Services.Interfaces;
 [TestFixture]
 public class HomeControllerTests
 {
 [Test]
 public async Task Index_ReturnsAViewResult_WithAnIndexViewModel()
 {
 // arrange
 IEnumerable<ArtistItemViewModel> tenFemaleArtists = new List<ArtistItemViewModel>
 {
 new ArtistItemViewModel
 {
 FirstName = "A1",
 LastName = "A1",
 ImageAlternateText = "A1",
 ImageUrl = "A1",
 PrimarySlug = "A1",
 },
 new ArtistItemViewModel
 {
 FirstName = "A2",
 LastName = "A2",
 ImageAlternateText = "A2",
 ImageUrl = "A2",
 PrimarySlug = "A2",
 },
 new ArtistItemViewModel
 {
 FirstName = "A3",
 LastName = "A3",
 ImageAlternateText = "A3",
 ImageUrl = "A3",
 PrimarySlug = "A3",
 },
 new ArtistItemViewModel
 {
 FirstName = "A4",
 LastName = "A4",
 ImageAlternateText = "A4",
 ImageUrl = "A4",
 PrimarySlug = "A4",
 },
 new ArtistItemViewModel
 {
 FirstName = "A5",
 LastName = "A5",
 ImageAlternateText = "A5",
 ImageUrl = "A5",
 PrimarySlug = "A5",
 },
 new ArtistItemViewModel
 {
 FirstName = "A6",
 LastName = "A6",
 ImageAlternateText = "A6",
 ImageUrl = "A6",
 PrimarySlug = "A6",
 },
 new ArtistItemViewModel
 {
 FirstName = "A7",
 LastName = "A7",
 ImageAlternateText = "A7",
 ImageUrl = "A7",
 PrimarySlug = "A7",
 },
 new ArtistItemViewModel
 {
 FirstName = "A8",
 LastName = "A8",
 ImageAlternateText = "A8",
 ImageUrl = "A8",
 PrimarySlug = "A8",
 },
 new ArtistItemViewModel
 {
 FirstName = "A9",
 LastName = "A9",
 ImageAlternateText = "A9",
 ImageUrl = "A9",
 PrimarySlug = "A9",
 },
 new ArtistItemViewModel
 {
 FirstName = "A10",
 LastName = "A10",
 ImageAlternateText = "A10",
 ImageUrl = "A10",
 PrimarySlug = "A10",
 }
 };
 Mock<IArtistsService> mockArtistsService = new Mock<IArtistsService>();
 mockArtistsService
 .Setup(x => x.GetTopTenFemaleArtistsByLyricsCountAsync())
 .ReturnsAsync(tenFemaleArtists);
 IEnumerable<LyricItemViewModel> tenRecentLyrics = new List<LyricItemViewModel>
 {
 new LyricItemViewModel
 {
 Title = "L1",
 LyricPrimarySlug = "L1",
 ArtistId = 1,
 ArtistName = "L1",
 ArtistPrimarySlug = "L1",
 ArtistImageUrl = "L1",
 ArtistImageAlternateText = "L1",
 },
 new LyricItemViewModel
 {
 Title = "L2",
 LyricPrimarySlug = "L2",
 ArtistId = 2,
 ArtistName = "L2",
 ArtistPrimarySlug = "L2",
 ArtistImageUrl = "L2",
 ArtistImageAlternateText = "L2",
 },
 new LyricItemViewModel
 {
 Title = "L3",
 LyricPrimarySlug = "L3",
 ArtistId = 3,
 ArtistName = "L3",
 ArtistPrimarySlug = "L3",
 ArtistImageUrl = "L3",
 ArtistImageAlternateText = "L3",
 },
 new LyricItemViewModel
 {
 Title = "L4",
 LyricPrimarySlug = "L4",
 ArtistId = 4,
 ArtistName = "L4",
 ArtistPrimarySlug = "L4",
 ArtistImageUrl = "L4",
 ArtistImageAlternateText = "L4",
 },
 new LyricItemViewModel
 {
 Title = "L5",
 LyricPrimarySlug = "L5",
 ArtistId = 5,
 ArtistName = "L5",
 ArtistPrimarySlug = "L5",
 ArtistImageUrl = "L5",
 ArtistImageAlternateText = "L5",
 },
 new LyricItemViewModel
 {
 Title = "L6",
 LyricPrimarySlug = "L6",
 ArtistId = 6,
 ArtistName = "L6",
 ArtistPrimarySlug = "L6",
 ArtistImageUrl = "L6",
 ArtistImageAlternateText = "L6",
 },
 new LyricItemViewModel
 {
 Title = "L7",
 LyricPrimarySlug = "L7",
 ArtistId = 7,
 ArtistName = "L7",
 ArtistPrimarySlug = "L7",
 ArtistImageUrl = "L7",
 ArtistImageAlternateText = "L7",
 },
 new LyricItemViewModel
 {
 Title = "L8",
 LyricPrimarySlug = "L8",
 ArtistId = 8,
 ArtistName = "L8",
 ArtistPrimarySlug = "L8",
 ArtistImageUrl = "L8",
 ArtistImageAlternateText = "L8",
 },
 new LyricItemViewModel
 {
 Title = "L9",
 LyricPrimarySlug = "L9",
 ArtistId = 9,
 ArtistName = "L9",
 ArtistPrimarySlug = "L9",
 ArtistImageUrl = "L9",
 ArtistImageAlternateText = "L9",
 },
 new LyricItemViewModel
 {
 Title = "L10",
 LyricPrimarySlug = "L10",
 ArtistId = 10,
 ArtistName = "L10",
 ArtistPrimarySlug = "L10",
 ArtistImageUrl = "L10",
 ArtistImageAlternateText = "L10",
 },
 };
 Mock<ILyricsService> mockLyricsService = new Mock<ILyricsService>();
 mockLyricsService
 .Setup(x => x.GetRecentLyricsAsync())
 .ReturnsAsync(tenRecentLyrics);
 HomeController homeController = new HomeController(mockArtistsService.Object, mockLyricsService.Object);
 // act
 IActionResult actionResult = await homeController.Index();
 // assert
 ViewResult view = actionResult.Should().BeOfType<ViewResult>().Subject;
 IndexViewModel viewModel = view.Model.Should().BeOfType<IndexViewModel>().Subject;
 viewModel.FemaleArtists.Should().HaveCount(10);
 viewModel.Lyrics.Should().HaveCount(10);
 }
 }
}

As far as tests concenrning the controller, is there anything else that I should test? Also, any other suggestions on naming or making things more readable ...etc

In my test, I can extract the code that builds the lists to a method, is there anything else?

Answer 1

First, here are some quick observations about your implementation:

1. It does not handle faulty or malfunctioning cases:
   • What if one of the services fails?
   • What if one of the services responds quite slowly?
2. It does not take advantage of concurrent async calls
   • As I can see the artistsService call does not depend on the previous service call
   • You can run them concurrently like this or you can further enhance it:

var recentLyrics =_lyricsService.GetRecentLyricsAsync();
var topTenFemailArtists = _artistsService.GetTopTenFemaleArtistsByLyricsCountAsync();
await Task.WhenAll(recentLyrics, topTenFemailArtists);
viewModel.Lyrics = await recentLyrics;
viewModel.FemaleArtists = await topTenFemailArtists;

3. It relies on implicit routing
   • Make it explicit via the HttpGetAttribute (1) then you can & should test this aspect as well:

[HttpGet, Route("Home/Index")]
public async Task<IActionResult> Index()

4. viewModel is not really good name for your variable.
   • It uses Hungarian notation which should be avoided if possible.
5. Try to separate data retrieval and response creation.
   • It could help you a lot during debugging.

Now, let's review your test

1. First of all: naming. Please try to use the Given-When-Then structure in order to describe under what circumstances which method how should behave
   • In your case, for example: GivenAFlawlessArtists_AndAFlawlessLyricsServices_WhenTheIndexActionIsCalled_ThenItFetchesDataFromTheServices_AndPopulatesTheResponseWithTheResults
   • Given a flawless Artists And a flawless Lyrics Services
   • When the Index action is called
   • Then it fetches data from the Services And populates the response with the Results
   • It describe what do you except under certain conditions
2. As you have already mentioned the sample data generation could and should be extracted away.
3. The homeController is not a really good name. In general you can name it to SUT. This abbreviates the following: System Under Test.
   • It helps the reader of your code to remain focused.
4. It might make sense to perform deep comparison on your viewmodel as well
   • To make sure that the data is not changed / masked / tampered by the controller's action.

• Lukas Körfer: I agree with a lot of your points, however I don't see the problem with viewModel. Hungarian Notation means using the type of a variable in its name, but viewModel is just what the variable represents in the context of MVVM. I also think it is perfectly fine to keep the name homeController for the tests. SUT refers to a system, but unit tests usually test units, hence the term Unit Under Test (UUT) exists.
• J86: In terms of handling things going wrong, do I just wrap in a try / catch? and if error, redirect to an error controller/action?
• Peter Csala: @J86 As always it depends. Is there anything what you can do in case of failure? For example perform a retry or fallback to another read replica, etc. If your ASP.NET application has been configured with a global error handler then you should not need to try/catch and manually redirect. ASP.NET will do that on your behalf.
• J86: Thanks, I really can't think of anything that would go wrong :/ I mean unless the database connection string is wrong, but there's not much we can do about that