1

The Question:

I'm developing an app in C# and am currently designing a menu which contains submenus and needs to be different depending on the category of the user.

I've written the code but am struggling to structure it so that it is testable.

I've read a lot about unit testing and testable code but still find it tough to write it well - I was hoping people could give me some pointers on re-structuring so that it's testable and so I'm not setting myself up for trouble down the road.

The Code:

A menu item:

public struct Item
{
    public string HRef { get; set; }
    public string Title { get; set; }

    public Item(string hRef, string title)
    {
        HRef = hRef;
        Title = title;
    }
}

A menu:

It has a list of items and a list of submenus (I took inspiration from how a directory tree is structured with files and folders)

public class Menu
{
    public string Name { get; set; }
    public List<Item> Items { get; set; }
    public List<Menu> SubMenus { get; set; }

    public Menu(string name, List<Item> items, List<Menu> subMenus)
    {
        Name = name;
        Items = items;
        SubMenus = subMenus;
    }
}

The Menu Manager: - The thing under test

The menu manager contains a repository where I get the category of a user from

It has a GetMenu() method which accepts the user name and returns the correct menu depending on the user's category.

It has two private methods which build the menu for each category (currently just Admin and Default but will eventually be more).

public class Manager
    {
        WorkflowPlusApp.Database.IInfoStaffRepository repository;

        public Manager(IInfoStaffRepository repository)
        {
            this.repository = repository;
        }

        public Menu GetMenu(string userName)
        {
            var userCategory = repository.GetStaffCategory(userName);

            switch(userCategory)
            {
                case "Admin": return GetAdminMenu();
                
                default: return GetDefaultMenu();
            }
        }




        private Menu GetAdminMenu()
        {
            return new Menu(
                name: "/",
                items: new List<Item>()
                {
                    new Item(hRef: "", title: "Request Form"),
                    new Item(hRef: "", title: "Tasks"),
                    new Item(hRef: "", title: "Project"),
                },
                subMenus: new List<Menu>()
                {
                    new Menu(
                        name: "/Config",
                        items: new List<Item>()
                        {
                            new Item(hRef: "", title: "Area")
                        },
                        subMenus: new List<Menu>())
                });
        }




        private Menu GetDefaultMenu()
        {
            return new Menu(
                name: "/",
                items: new List<Item>()
                {
                    new Item(hRef: "", title: "Request Form"),                    
                },
                subMenus: new List<Menu>());
        }
    }

The Unit Test

I've written a unit test using MSTest and Moq

[TestMethod()]
        public void GetMenu_Staff_Admin_Returns_Admin_Menu()
        {
            var userName = "TestUser";

            //Setup Mock
            var mockRepository = new Mock<WorkflowPlusApp.Database.IInfoStaffRepository>();
            mockRepository.Setup(x => x.GetStaffCategory(userName)).Returns("Admin");
            
            //Setup
            var manager = new Menus.Manager(mockRepository.Object);
            var menu = manager.GetMenu(userName);

            //Test - Does it return the Admin menu?
            Assert.IsTrue(menu.Items.Count == 3);
            Assert.IsTrue(menu.Items[0].Title == "Request Form");
            Assert.IsTrue(menu.Items[0].Title == "Tasks");
            Assert.IsTrue(menu.Items[0].Title == "Project");
            Assert.IsTrue(menu.SubMenus.Where(x => x.Name == "Config").ToList()[0].Items.Count == 1);
            Assert.IsTrue(menu.SubMenus.Where(x => x.Name == "Config").ToList()[0].Items[0].Title == "Area");
        }

The Admin menu should be returned in a particular order and have particular entries so I thought by using the Titles I'm testing a behaviour - if the menu changes it should fail and alert me that the menu isn't right for Admin staff.

I can't help feeling like this test is tied to much to the implementation and is bad code though.

In setting up the Mock I have to know two things:

That the Manager's GetMenu() function calls the repository's GetStaffCategory() function

That the category the repository returns is Admin

So I'm tying myself to two of the Manager's implementation details - if they change I'll have to change the test code

I would love to be able to write code well and while the books and articles I've read are helpful I still find it difficult to do in practice, any help or suggestions - even if it's scrap the whole thing and use a simple Dictionary - would be helpful towards me learning to write testable decent code.

Many Thanks

Nick

1
  • No this looks pretty valid. You are testing what the Manager does when it is asked Y and its collaborator provides X. Add another case to cover when the collaborator does Z, etc... to get good coverage
    – Kain0_0
    Jan 20, 2021 at 23:15

1 Answer 1

1

The omniscient mocker

In setting up the Mock I have to know two things:

During the arrange stage of your test, you need to know the things you need to mock. That usually entails needing to mock specific return values of your test object's dependency's methods.

There's differences between white box and black box testing, but I'm not focusing on specifically picking one or the other here. For basic unit testing, knowing what parts of the interface of your test object's dependencies you're relying on is a necessary requirement to be able to isolate your test object and unit test it.

Testing the wrong thing

The Admin menu should be returned in a particular order and have particular entries so I thought by using the Titles I'm testing a behaviour - if the menu changes it should fail and alert me that the menu isn't right for Admin staff.

I can't help feeling like this test is tied to much to the implementation and is bad code though.

It feels like a good pattern being underused when you design a recursive tree structure, but then limit yourself to methods that return the entire tree structure in a single go. Not saying it can't be done, but before I saw your MenuManager code I was expecting it to return each level individually.

The problem here is that there are parts here where you have no logic. getAdminMenu and getDefaultMenu are a hardcoded mapping. And that's okay. We can argue about different ways to write it but what doesn't change is that sometimes you just need some hardcoded mappings (in whatever form) to define specific content for your application.

But mappings are not behavior, and tests are all about testing behavior. You're struggling to write a test here because you're focusing on the part that shouldn't be tested (i.e. the specific menu content). All you're doing is going through a pointless exercise to see if you can write the same menu structure twice (once in the mapping, once in the assert logic).

Tests that assert mapping tend to never fail unless you change the mapping, and in most if not all cases, changing the mapping was intentional, and you just forgot to also change the test's assertion logic. That's not a useful test, it's an arbitrary self-imposed additional hurdle to cross.

This mapping just doesn't need to be tested.

And before the "everything should be tested blah blah test coverage" comments are posted, let me just say that I agree that all behavior should be tested to maximize test coverage, but I still insist that hardcoded mappings are not behavior.


Telegraphing your intent

We can argue about different ways to write it

Did I just quote myself? Yes I did, other Flater. Yes I did. Let's discuss how your coding style can impact your (and your readers') perception and expectations.

At a very basic level, variables/fields/properties are something, and methods do something. Logic, i.e. the thing we test, is the "doing" part of your code. Mappings, i.e. the thing we shouldn't be testing, is the "being" part of your code, commonly referred to as "state" (as opposed to "behavior").

The reason you've been stuck trying to test your mapping is because you made it into a method, and then tried to test it because it's a method. I would wager that you wouldn't have tried to write a test for it if you had used properties instead of methods.

By storing this mapping in a property, you clearly signal that this is not behavior that needs to be tested. And properties can be made part of an interface just as easily.

I can already hear someone writing a comment to the effect of "but you can write methods to return state (like OP has) or write properties with complex logic in their getters and setters". Yes, you can do that on a technical level, but it wouldn't pass a code review due to being bad coding practice. This is no differet from how the compiler lets me choose the names of my variables completely freely, but good practice and its review will flag some variable names and approve others.


More abstractions

What you need here is to separate the mapping (data) from the mapper (behavior/logic), and when you test the mapper, either rely on the existing mapping (by considering it as a static constant) or mocking a mapping for the purposes of the test.

I'll show a basic example of both. In either case, the test for behavior will rely on the mapping, rather than trying to assert the mapping.

Using static constant mapping

Note that this is less flexible than the other option, but there are justified cases for it. A never-changing menu structure (in the scope of a given build) is a valid justification.

public static class MenuMapping
{
    public static Menu Default => new Menu ( ... );
    public static Menu Admin   => new Menu( ... );
}

public class MenuManager
{
    public Menu GetMenu(string userName)
    {
        var userCategory = repository.GetStaffCategory(userName);

        switch(userCategory)
        {
            case "Admin": 
                return MenuMapping.Admin;                
            default: 
                return MenuMapping.Default;
        }
    }
}

[TestMethod()]
public void GetMenu_Staff_Admin_Returns_Admin_Menu()
{
    var userName = "TestUser";

    //Setup Mock
    var mockRepository = new Mock<WorkflowPlusApp.Database.IInfoStaffRepository>();
    mockRepository.Setup(x => x.GetStaffCategory(userName)).Returns("Admin");
        
    //Setup
    var manager = new Menus.Manager(mockRepository.Object);
    var menu = manager.GetMenu(userName);

    //Test - Does it return the Admin menu?

    Assert.AreEqual(menu, MenuMapping.Admin);
}

Mocked mapping

In this approach, the mapping itself becomes an injected dependency of the mapper. This opens the door to mocking its content. You then no longer have to write asserts for the real admin menu content, you just have to confirm that the MenuManager returns the correct value based on its mocked injected mapping.

public interface IMenuMapping
{
    Menu Default { get; }
    Menu Admin { get; }
}

public class MenuMapping : IMenuMapping
{
    public Menu Default => new Menu ( ... );
    public Menu Admin   => new Menu( ... );
}

public class MenuManager
{
    private readonly IInfoStaffRepository repository;
    private readonly IMenuMapping menuMapping;

    public Manager(IInfoStaffRepository repository, IMenuMapping menuMapping)
    {
        this.repository = repository;
        this.menuMapping = menuMapping;
    }

    public Menu GetMenu(string userName)
    {
        var userCategory = repository.GetStaffCategory(userName);

        switch(userCategory)
        {
            case "Admin": 
                return menuMapping.Admin;                
            default: 
                return menuMapping.Default;
        }
    }
}

[TestMethod()]
public void GetMenu_Staff_Admin_Returns_Admin_Menu()
{
    var userName = "TestUser";

    //Setup Mock repository
    var mockRepository = new Mock<WorkflowPlusApp.Database.IInfoStaffRepository>();
    mockRepository.Setup(x => x.GetStaffCategory(userName)).Returns("Admin");

    //Setup Mock mapping
    Menu mockMenu = new Menu( ... );
    var mockMapping = new Mock<IMenuMapping>();
    mockRepository.Setup(x => x.Admin).Returns(mockMenu);
        
    //Setup
    var manager = new Menus.Manager(mockRepository.Object);
    var menu = manager.GetMenu(userName);

    //Test - Does it return the Admin menu?

    Assert.AreEqual(menu, mockMenu);
}

Important note:
In both cases, I wrote an oversimplified equality check because the complexity of checking equality is not the focus here. You could override the default equality logic for Menu objects, or you could write more specific assert logic.

Personally, I tend to write some generic assertion helper methods, e.g. to confirm if a given list contains a given set of expected values, whether it contains only those expected values, none of the expected values, whether it contains them in order, ... And when you have that kind of logic, the assertion logic in your tests is kept to a minimum.

Writing these assertion methods is well beyond the scope of this answer, but I wanted to point it out because clean code is often built on the backs of many small improvements that lower the complexity density in your code.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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