In unit tests, should I check all null and invalid child properties in the input arguments that cause error and force developers too?

Question

In a large application, I have a lot of methods with input arguments that each argument is a model with nested models (as you see in the image).

You see that I have written some conditional codes and TestCases in this unit test to check each case that one of the child properties is invalid.

---

In another example, assume that we have the following method in MyClass:

public void SetItem(Task task)
{
    foreach (var subTasks in task.SubTasks)
    {
        ..........
    }
}

So should I force the developer to check task.SubTask == null before the above loop and throw an ArgumentException by a unit test like bellow? I mean is it a "MUST" that the developer checks SubTasks value (and each property of input task before working with it) to check whether it is null or not and throw ArgumentException when the related property is null?

[Fact]
public void SetItem_WhenSubTasksIsNull_SetItemFromTask(Task task)
{
    // Act and Assert
    Assert.Throws<ArgumentException>(
          ()=> new MyCalss().SetItem(new Task(){
                   SubTasks=null
      }));
}

Is it a correct approach? Should I force the developers to write the same conditional codes in his/her related method to pass my unit test or I should not consider these cases as TestCases?

Maybe developers say:

• Normally our codes returns these exceptions if these fields have invalid value (for example when we wrote arg1.childA.childB and childA is null, it returns the appropriate exception itself), so it is not necessary to we check their values and raise the related same exception when their values are invalid.
• Checking all fields at first forces us to refactor hundreds of methods to add a lot of conditional codes in them and it take many time and
• In many cases we know that child properties filled with correct values because we ourselves provide their values in other methods and we include required fields in them
• Also this makes the codes of each method more crowded and reduces readability

On the other hand, we see in the image that in large projects usually there are a lot of fields in an input model that checking them is possible and it cause writing many lines of codes for most of the unit test such as what you see in the above image.

So, What is the correct approach and answer to these issues?

Answer 1

Firstly, in the social aspect of the question, I would avoid using expressions like "force the developer" or "It is a MUST". It may be received as an external interference with the developers' work to add more bureaucracy and micro-management. If you are not in this role, I hardly will work out.

I would suggest coming to a shared agreement on how work should be carried out. E.g., if the team agrees to follow the Programmer's Oath, you should discuss how to achieve this:

I will produce, with each release, a quick, sure, and repeatable proof that every element of the code works as it should.

If they can provide this proof, you are good.

Now for the coding aspect.

public void SetItem(Task task)
{
    foreach (var subTasks in task.SubTasks)
    {
        ..........
    }
}

What happens if you don't have a check for task.SubTasks as null? Your clients will be forced to deal with a NullPointerException. So far, so good. But then you decide to refactor your code and return a NoSubtasksException. Now your clients will have to change their code - meaning that you coupled your clients to the internal implementation of your code. Probably you don't want this.

Now for the questions

• Normally our codes returns these exceptions if these fields have invalid value (for example when we wrote arg1.childA.childB and
  childA is null, it returns the appropriate exception itself), so it
  is not necessary to we check their values and raise the related same
  exception when their values are invalid.

It's a strategy, but it still means that you need to create checks for these exceptions. If you following TDD e.g., you wouldn't even be able to write the code that throws the exceptions without checks for them being written first.

• Checking all fields at first forces us to refactor hundreds of methods to add a lot of conditional codes in them and it takes many
  time and In many cases we know those child properties filled with
  correct values because we ourselves provide their values in other
  methods and we include required fields in them

Firstly, one doesn't need to tackle everything at once. You can follow a water leak approach and improve things as you need to change them. If a piece of code is really important, the tendency is for it to suffer changes often, meaning it will be improved sooner.

• Also this makes the codes of each method more crowded and reduces readability

Then it's part of creating better abstractions.

Three things that you may consider are:

• The Null Object pattern;
• The Decorator pattern;
• Optional.

So you don't need to null, which is an anti-pattern.