3

I have a method:

public static void AddPersonToPeopleList(List<PersonModel> people, PersonModel person)
        {
            CheckWhiteSpace(person.FirstName, "FirstName");
            CheckWhiteSpace(person.LastName, "LastName");

            people.Add(person);
        }

That calls the helper methdod:

public static void CheckWhiteSpace(string name, string paramName) {
            if (string.IsNullOrWhiteSpace(name)) {
                throw new ArgumentException("You passed in an invalid parameter", paramName);
            }
        }

Which simply checks for null or white space in my string with the parameter name, and throws an exception if it does.

        [Theory]
        [InlineData("Mick", "", "LastName")]
        [InlineData("", "Adams", "FirstName")]
        public void AddPersonToPeopleList_ShouldFail(string firstName, string lastName, string param)
        {
            PersonModel newPerson = new PersonModel { FirstName = firstName, LastName = lastName };
            List<PersonModel> people = new List<PersonModel>();

            Assert.Throws<ArgumentException>(param, () => DataAccess.AddPersonToPeopleList(people, newPerson));
        }

My test is designed to simply make sure an exception is thrown if either the first name or second name is empty.

My question is whether I need to also make a test for the helper method I used (CheckWhiteSpace) or whether the test I've made takes care of testing the functionality of that helper method and my original method?

7

It depends on your intentions with the CheckWhiteSpace method:

  • If CheckWhiteSpace is intended to become a reusable library function, placed in a class different from the one where AddPersonToPeopleList belongs to, then a unit test for it makes sense. The fact CheckWhiteSpace is public may be an indicator for this.

  • If CheckWhiteSpace is currently not reused anywhere else, and you are not sure if it will ever be, then it should (currently) stay in the class where AddPersonToPeopleList "lives", and it should be made private. In this case, no additional unit test will be required.

Note it is an implementation detail of AddPersonToPeopleList that it uses CheckWhiteSpace internally, and implementation details may be subject to change. Furthermore, if you start with CheckWhiteSpace as a private method, but later decide to make it reusable in public, then it is early enough to write a separate unit test for this method.

It is neither a good idea to

  • write any tests for implementation details,

  • nor to leave out any tests because of making assumptions about implementation details.

So think which of the mentioned categories fits best to your case, then you have your answer.

| improve this answer | |
3

Yes, you wrote the code so it needs a test.

If you are arguing that it's too trivial to test then its equally too trivial to require the extra function, bundle it in with where ever its called.

You are bound to have Prince or Madonna sign up to your service and then you will thank me for making your write this test!

Here's How I would code it: Notice I have removed the static and added an interface to reduce coupling and so that we can test multiple implementations and mock the DataAccess in other tests.

Also, the validation check is so trivial that we move it to a private method. (no need to test private methods) But if it was more complex you would make a IPersonValidator and inject it into the DataAccess component. In this case you would want to test IPersonValidator in the same way we are testing IDataAccess here

public class DataAccess : IDataAccess
{
    public List<Person> GetPeople() 
    {
        //return all the people
    }

    public void AddPerson(Person p)
    {
        if(!validatePerson(p))
        {
            throw new Exception("invalid person");
        }
        //add to database
    }

    private bool validatePerson(Person p)
    {
        //validation code
    }
}

///tests
public void TestAddPerson()
{
    IDataAccess t = new DataAccess();
    var p = new Person() { Name="Madonna"});
    t.AddPerson(p);
    var actual = t.GetPeople();

    Assert.IsTrue(actual.Contains(p));
}
| improve this answer | |
  • 2
    @Fabio, no you don't need to write new tests if that code is already properly covered by existing tests. Good automated testing practice is opposite to what Ewan says: you aren't testing methods (unless they are public APIs); you are testing functionality. – David Arno Jun 4 '19 at 7:42
  • 1
    @davidarno note, this is a public static method with no interface – Ewan Jun 4 '19 at 7:47
  • 1
    Removing the static would be a ridiculous thing to do. I often wonder whether you just troll us with these silly comments. – David Arno Jun 4 '19 at 8:21
  • 2
    "...but we know that static is amost always bad..." No! Static is only bad if the function has side effects. If it has no side effects, then make it static. For some reason, you appear to have missed the memo on that one. – David Arno Jun 4 '19 at 8:55
  • 1
    @DavidArno its also bad for a variety of other reasons. you should never make a function static simply to indicate lack of side effects. I mean just look at the example here which calls another static function! – Ewan Jun 4 '19 at 8:59
2

If you've been doing TDD properly, and this is method is something that you've created during refactoring, then no - as this is already covered by tests at the appropriate level of abstraction. Refactoring is rearranging the internals without changing the functionality - and without changing tests, that act as a safety net for you. You have to make a design decision though - decide (as a team) what constitutes internals vs what is the "surface area" that should be exposed to client code and to tests. Note that both clients and tests should be unaware of those internals. Otherwise you cannot refactor. Think of the test suite as being another client for the code being tested. If at some point you find that your high level design is not quite right or that functionality needs to change, then you have to change the tests as well, or add new ones - this is expected, but it is not refactoring.

Now, if you were making something like a library that other code will use, and this function was part of its API, then you would write a test for it directly, because it's at the "surface area" in this context, but you still wouldn't have your tests coupled to the internals of that library. This consideration is (roughly) along the same lines as Doc Brown's point about reuse.

That said, sometimes, when you are dealing with a difficult-to-understand piece of logic, or a complicated, finicky algorithm, but a lot of the intricacies of how it works constitutes internal detail in that particular level of abstraction, you may want to write tests that go into that detail, to help you get a handle on the problem. You have to somehow differentiate these tests from the other ones, though, as these aren't really a part of the test suite for the application/library - these are for you. Also, since this is internal detail, the code may be volatile, and so it may change relatively frequently in ways that will require these tests to change or make them outright obsolete. So you may not even check in these tests. You may decide to delete them once you're done (yes, this is allowed). Or if this peace of code is deemed to be of particular interest (at least for the time being), you may want to decide on a way to somehow differentiate these as white-box tests and check them in anyway, with the team having a mutual understanding that these are coupled to the internals (and that they may eventually need to be changed/replaced/deleted); it may be difficult to get everyone on the same page though, and your tools & pipeline may be setup in such a way as to make this awkward to implement. Again, if this code is of particular interest, and it turns out to be long-lived, another option to consider is to place it into a separate library/package, and then write these tests in that context - giving you proper isolation and separation of concerns.

| improve this answer | |
1

YAGNI principle:

It's up to looking at your own codebase to see whether you would need to use this validation elsewhere. If you do then it's reasonable to refactor out the method and then write a specific set of tests for it.

It looks like you are defining the helper method in the same class as your AddPersonToPeopleList class. If you want to explicitly test that functionality you can move it to a separate class or as an extension method of string.

If you decide to refactor, your code would could be something like:

public static class StringExtensions{
     public static void CheckWhiteSpace(this string name, string paramName){
        if(string.IsNullOrWhiteSpace(name)
            throw new ArgumentException("You passed in an invalid parameter", paramName); 
     }
}

Called then as

person.FirstName.CheckWhiteSpace("FirstName")

Nice explanation here: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/extension-methods

| improve this answer | |
1

In your particular example:

First:
You need to add test cases for "FirstName" and "LastName" being null or white space " ".

Second:
No, don't test logic which already tested through other tests.
If you are using CheckWhiteSpace somewhere else in your application, you probably are having tests which covers logic of CheckWhiteSpace as well.

Test separately logic which not fully tested through the consumers or not tested because of being mocked.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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