Testing an action returns a View with a ViewModel using xUnit and Moq This is my first test, I think it's testing what I need it to test but wanted to get some feedback.

I wanted to test to make sure the controller action returns a view with a certain ViewModel.

The code that it will be testing:

Controller:

public class UserController : Controller
{
    private readonly ILogger&lt;UserController&gt; _logger;
    private readonly IViewModelService _vmService;

    public UserController(ILogger&lt;UserController&gt; logger, IViewModelService vmService)
    {
        _logger = logger;
        _vmService = vmService;
    }

    public async Task&lt;IActionResult&gt; Index()
    {
        return View(await _vmService.GetIndexVM());
    }
}


ViewModel

public class UserListVM
{
    public IQueryable&lt;DimUser&gt; UserList { get; set; }
}


ViewModelService

public async Task&lt;UserListVM&gt; GetIndexVM()
{
    return new UserListVM()
    {
        UserList = await _userRepo.GetUserList()
    };
}


The Test:

public class UserControllerTests
{ 
    [Fact]
    public async Task Index_ReturnsAViewResult_WithUserListVM()
    {
        // Arrange
        var logger = new Mock&lt;ILogger&lt;UserController&gt;&gt;();
        var vmService = new Mock&lt;IViewModelService&gt;();
        var myList = new List&lt;DimUser&gt;
        {
            new DimUser()
            {
                UserId = 1,
                FirstName = "Test",
                Surname = "TestSur",
                RefNumber = "ABC1111111",
                DateOfBirth = DateTime.Now
            }
        };

        vmService.Setup(x =&gt; x.GetIndexVM()).ReturnsAsync(new UserListVM()               
        {
            UserList = myList.AsQueryable()
        });

        var userController = new UserController(logger.Object, vmService.Object);

        // Act
        var result = await userController.Index();
        var viewResult = Assert.IsType&lt;ViewResult&gt;(result);
        var model = Assert.IsType&lt;UserListVM&gt;(viewResult.ViewData.Model);

        // Assert
        Assert.IsType&lt;UserListVM&gt;(model);
    }
}
Let's review each code segment one by one:

public class UserController : Controller
{
    private readonly ILogger&lt;UserController&gt; _logger;
    private readonly IViewModelService _vmService;

    public UserController(ILogger&lt;UserController&gt; logger, IViewModelService vmService)
    {
        _logger = logger;
        _vmService = vmService;
    }

    public async Task&lt;IActionResult&gt; Index()
    {
        return View(await _vmService.GetIndexVM());
    }
}


I would generally recommend to avoid code like this:

return View(await _vmService.GetIndexVM());


It is hard to add proper error handling, add transformation logic, add conditional branching, etc. A better approach would be to separate these two operations:

var indexViewModel = await _vmService.GetIndexVM();
return View(indexViewModel);




public async Task&lt;UserListVM&gt; GetIndexVM()
{
    return new UserListVM()
    {
        UserList = await _userRepo.GetUserList()
    };
}


First of all the same applies here as above, do not mix object creation logic and async calls.

Secondly, in this simple code you have repeated four times the underlying collection type, which is an implementation detail. If you need to change that implementation detail in the lower layer that would propagate through several layers. Remember that hiding implementation details will help you minimize the scope of a change. A better approach would be:

public async Task&lt;UsersVM&gt; GetIndexVM()
{
    return new UsersVM()
    {
        User = await _userRepo.GetUsers()
    };
}


There is another thing, this service now has two responsibilities:
1) Retrieve data via a lower layer
2) Transform data to the presentation layer

In other words, this layer is an adapter between your presentation layer and repository layer. Generally speaking the service layer is the place where your business logic should reside. Because there is no business logic here, that's why it acts as an adapter.

I have seen the following two approaches regarding object mapping:


Each layer transforms its objects to the lower layer's object
model
Each layer accepts upper layer's object model and it
transforms it into its own model   


The first one fits nicely into the n-layer architecture model where each layer only knows about that layer, which is directly beneath it. So, presentation layer knows about service layer. Service layer knows about repository layer.

The second approach violates this rule. Service layer knows about repository layer and knows about presentation layer's domain model. It is not bad, but the first approach (in my opinion) separates the concerns better. 



public class UserListVM
{
    public IQueryable&lt;DimUser&gt; UserList { get; set; }
}


Here your naming and data type is not matching. With this name you are stating that it should contain a List, which implies that you could use such operators like Add, Remove, etc. IQueryable does not provide such API. 

IQueryable is a type, which is used for deferred execution. In other words it indicates that this is just a query not the materialised form of the query. The problem with this is that it will execute the query when you somehow iterate through it (via foreach or calling .Count, etc.) If you do this in your view then your repository's datacontext might already be disposed.

A better approach would be to expose it like this:

public class UsersVM
{
    public IList&lt;DimUser&gt; Users{ get; set; }
}




Your test's Arrange part looks good, so I will spend some thought on the rest:

// Act
var result = await userController.Index();
var viewResult = Assert.IsType&lt;ViewResult&gt;(result);
var model = Assert.IsType&lt;UserListVM&gt;(viewResult.ViewData.Model);

// Assert
Assert.IsType&lt;UserListVM&gt;(model);


Your Act section should consist only of the call of the Index function of the userController. The assertions should go the under the Assert section.

I would also consider to use IsAssignableForm&lt;T&gt; instead of IsType&lt;T&gt;, because the former supports inheritance as well.