11

I have a Unit Test, which looks like this:

[Test]
public void Should_create_person()
{
     Assert.DoesNotThrow(() => new Person(Guid.NewGuid(), new DateTime(1972, 01, 01));
}

I am asserting that a Person object is created here i.e. that validation does not fail. For example, if the Guid is null or the date of birth is earlier than 01/01/1900, then the validation will fail and an exception will be thrown (meaning the test fails).

The constructor looks like this:

public Person(Id id, DateTime dateOfBirth) :
        base(id)
    {
        if (dateOfBirth == null)
            throw new ArgumentNullException("Date of Birth");
        elseif (dateOfBith < new DateTime(1900,01,01)
            throw new ArgumentException("Date of Birth");
        DateOfBirth = dateOfBirth;
    }

Is this a good idea for a test?

Note: I am following a Classicist approach to Unit Testing the Domain Model if that holds any bearing.

  • Does the constructor have any logic that worth to be asserted after-during the initialization? – Laiv Jan 29 '18 at 10:42
  • 2
    Never bother testing constructors!!! Construction should straight forward. Are you expecting fails in Guid.NewGuid(), or constructor of DateTime? – ivenxu Jan 29 '18 at 10:48
  • @Laiv, please see the update to the question. – w0051977 Jan 29 '18 at 10:49
  • 1
    It worth nothing to implement a test as the one you shared. However, I would test also the opposite. I would test the case where birthDate causes an error. That's the invariant of the class you want to be it under control and test. – Laiv Jan 29 '18 at 10:53
  • 3
    The test looks fine, save for one thing: the name. Should_create_person? What should create a person? Give it a meaningful name, like Creating_person_with_valid_data_succeeds. – David Arno Jan 29 '18 at 11:09
18

This is a valid test (although rather overzealous) and I sometimes do it to test constructor logic, however as Laiv mentioned in the comments you should ask yourself why.

If your constructor looks like this:

public Person(Guid guid, DateTime dob)
{
  this.Guid = guid;
  this.Dob = dob;
}

Is there a lot of point in testing whether it throws? Whether the parameters are assigned correctly I can understand but your test is rather overkill.

However, if your test does something like this:

public Person(Guid guid, DateTime dob)
{
  if(guid == default(Guid)) throw new ArgumentException("Guid is invalid");
  if(dob == default(DateTime)) throw new ArgumentException("Dob is invalid");

  this.Guid = guid;
  this.Dob = dob;
}

Then your test becomes more relevant (as you're actually throwing exceptions somewhere in code).

One thing I would say, generally it's bad practice to have a lot of logic in your constructor. Basic validation (like the null/default checks I'm doing above) are ok. But if you're connecting to databases and loading someone's data then that's where code starts to really smell...

Because of this, if your constructor is worth testing (because there's lots of logic going) then maybe something else is wrong.

You're almost certainly going to have other tests covering this class in business logic layers, constructors and variable assignments are almost certainly going to get complete coverage from these tests. Therefore it maybe pointless to add specific tests specifically for the constructor. However, nothing is black and white and I'd have nothing against these tests if I was code reviewing them - but I'd question whether they add much value above and beyond tests elsewhere in your solution.

In your example:

public Person(Id id, DateTime dateOfBirth) :
        base(id)
    {
        if (dateOfBirth == null)
            throw new ArgumentNullException("Date of Birth");
        elseif (dateOfBith < new DateTime(1900,01,01)
            throw new ArgumentException("Date of Birth");
        DateOfBirth = dateOfBirth;
    }

You're not only doing validation, but you're also calling a base constructor. For me this provides more reason to have these tests as they have the constructor/validation logic now split over two classes which decreases visibility and increases the risk of unexpected change.

TLDR

There is some value to these tests, however validation/assignment logic is likely to be covered by other tests in your solution. If there's lots of logic in these constructors which does require significant testing then it suggests to me there's a nasty code smell lurking in there.

  • @Laith, Please see the update to my question – w0051977 Jan 29 '18 at 10:56
  • I notice that you're calling a base constructor in your example. IMHO this adds more value of your test, constructor logic is now split over two classes and is therefore slightly higher risk of change therefore giving more reason to test it. – Liath Jan 29 '18 at 10:57
  • "However, if your test does something like this:" < Don't you mean "if your constructor does something like this"? – Kodos Johnson Jan 29 '18 at 18:21
  • "There is some value to these tests" - interestingly for me anyway, the value is showing that we could make this test redundant by using a new class to represent the person's dob (e.g. PersonBirthdate) which performs date of birth validation. Similarly the Guid check could be implemented on the Id class. This means you really don't have to have that validation logic in the Person constructor any more since it's not possible to construct one with invalid data - except for null refs. Of course, you have to write tests for the other two classes :) – Stephen Byrne Jan 29 '18 at 22:47
12

Already a good answer here, but I think one additional thing is worth mentioning.

When doing TDD "by the book", one needs to write a test first which calls the constructor, even before the constructor gets implemented. That test could actually look like the one you presented, even if there would be zero validation logic inside the constructor's implementation.

Note also that for TDD, one should write another test first like

  Assert.Throws<ArgumentException>(() => new Person(Guid.NewGuid(), 
        new DateTime(1572, 01, 01));

before adding the check for DateTime(1900,01,01) to the constructor.

In TDD context, the shown test makes perfectly sense.

  • Nice angle I'd not considered! – Liath Jan 29 '18 at 13:31
  • 1
    This demonstrates to me why such a rigid form of TDD is a waste of time: the test should have value after the code is written, or you're just writing every line of code twice, once as an assertion, and once as code. I would argue that the constructor itself is not a piece of logic that needs testing; the business rule "people born before 1900 must not be representable" is testable, and the constructor is where that rule happens to be implemented, but when would the test of an empty constructor ever add value to the project? – IMSoP Jan 30 '18 at 11:32
  • Is it really tdd by the book? I would create instance and call its method right away in a code. Then I would write test for that method, and by doing that I would also have to create instance for that method, so both constructor and method will be covered in that test. Unless in constructor there is some logic, but that part is covered by Liath. – Rafał Łużyński Jan 30 '18 at 16:44
  • @RafałŁużyński: TDD "by the book" is about writing tests first. It actually means to always write a failing test first (not compiling counts as failure as well). So you first write a test calling the constructor even when there is no constructor. Then you try to compile (which fails), then you implement an empty constructor, compile, run the test, result=green. Then you write the first failing test and run it - result=red, then you add the functionality to make the test "green" again, and so on. – Doc Brown Jan 30 '18 at 17:18
  • Of course. I didn't mean I write implementation first, then test. I just write "usage" of that code in a level above, then tests for that code, then I implement it. I am doing "Outside TDD" usually. – Rafał Łużyński Jan 30 '18 at 19:35

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.