I have been following this tutorial and trying to understand this Xunit test for an update service.

The service is as follows:

public async Task<Category> Update(Category category)
{
  if (_categoryRepository.Search(c => c.Name == category.Name && c.Id != category.Id).Result.Any())
    return null;

  await _categoryRepository.Update(category);
  return category;
}

And the Xunit test:

[Fact]
public async void Update_ShouldNotUpdateCategory_WhenCategoryDoesNotExist()
{
  var category = new Category()
  {
    Id = 1,
    Name = "Test Category 1"
  };

  var categoryList = new List<Category>()
  {
    new Category()
    {
        Id = 2,
        Name = "Test Category 2"
    }
  };

  _categoryRepositoryMock.Setup(c => c.Search(c => c.Name == category.Name && c.Id != category.Id))
      .ReturnsAsync(categoryList);

  var result = await _categoryService.Update(category);

  Assert.Null(result);
}

I know that from this authors notes about what the test is supposed to do is contradicting what the method name's when-condition (It's about 3/4 the way the linked tutorial page):

• The method does not update a category when the updated category name already exists.

The test is returning null, but I'm not sure if it's doing it for the right reason.

Is it that it can't find the category in the mock repo? As I tried adding a category in hopes that it would find the category and fail the test, but it still came back as null:

new Category()
{
  Id = 1,
  Name = "Test Category One"
}

Or is it that the mocking of the validation which returns categoryListing and even though it is not the correct record (as it's name is not a match) enough to cause the null? (I'm guessing it is this one)

I am new to unit testing, but it doesn't feel like a good test shouldn't the mock repository be preloaded with records and then the updated record have it's validation tested against it? I'm guessing this would just be another way of doing this, however, how would I change the code to accomplish that?

CodePudding user response：

The test is returning null, but I'm not sure if it's doing it for the right reason.

You are exactly right - service does not check for existence of the category and test passes only because the mock is setup to mimic behaviour "another category with the same name exists".

I am new to unit testing, but it doesn't feel like a good test shouldn't the mock repository be preloaded with records and then the updated record have it's validation tested against it

It is one of the approaches - it is matter of preference/best practices what and how to split up and test. I would say that provided code is bad in many parts:

1. The call to Task.Result (in CategoryService.Update) over async method is considered a bad practice (read more #1, read more #2):

if (_categoryRepository.Search(c => c.Name == category.Name && c.Id != category.Id).Result.Any())
                return null;

2. In general repository pattern over EF can be considered as antipattern (but there are a lot of people who will not agree with this), but even if you go with it the implementation of Update method still is very questionable

3. Contract relying on returning null also seems very questionable.

I'm guessing this would just be another way of doing this, however, how would I change the code to accomplish that?

First of all you will need to change the Update method to check for existence of the record. For example by adding next line:

if (!(await _categoryRepository.Search(c => c.Id == category.Id)).Any())
    return null;

(though I would combine both Search calls to _categoryRepository.Search(c => c.Name == category.Name || c.Id != category.Id) and checked the search results for count and existence of record with corresponding Id)

And then update the test by adding extra Setup call (other test can also require Setup update):

_categoryRepositoryMock.Setup(c => c.Search(c => c.Id == category.Id))
    .ReturnsAsync(new List<Category> {  });