Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I do not consider myself a DDD expert but, as a solution architect, do try to apply best practices whenever possible. I know there is a lot of discussion around the pro's and con's of the no (public) setter "style" in DDD and I can see both sides of the argument. My problem is that I work on a team with a wide diversity in skills, knowledge and experience meaning that I cannot trust that every developer will do things the "right" way. For instance, if our domain objects are designed so that changes to the object's internal state is performed by a method but provide public property setters, someone will inevitable set the property instead of calling the method. Use this example:

public class MyClass
{
    public Boolean IsPublished
    {
        get { return PublishDate != null; }
    }

    public DateTime? PublishDate { get; set; }

    public void Publish()
    {
        if (IsPublished)
            throw new InvalidOperationException("Already published.");

        PublishDate = DateTime.Today;

        Raise(new PublishedEvent());
    }
}

My solution has been to make property setters private which is possible because the ORM we are using to hydrate the objects uses reflection so it is able to access private setters. However, this presents a problem when trying to write unit tests. For example, when I want to write a unit test that verifies the requirement that we can't re-publish, I need to indicate that the object has already been published. I can certainly do this by calling Publish twice, but then my test is assuming that Publish is implemented correctly for the first call. That seems a little smelly.

Let's make the scenario a little more real-world with the following code:

public class Document
{
    public Document(String title)
    {
        if (String.IsNullOrWhiteSpace(title))
            throw new ArgumentException("title");

        Title = title;
    }

    public String ApprovedBy { get; private set; }
    public DateTime? ApprovedOn { get; private set; }
    public Boolean IsApproved { get; private set; }
    public Boolean IsPublished { get; private set; }
    public String PublishedBy { get; private set; }
    public DateTime? PublishedOn { get; private set; }
    public String Title { get; private set; }

    public void Approve(String by)
    {
        if (IsApproved)
            throw new InvalidOperationException("Already approved.");

        ApprovedBy = by;
        ApprovedOn = DateTime.Today;
        IsApproved = true;

        Raise(new ApprovedEvent(Title));
    }

    public void Publish(String by)
    {
        if (IsPublished)
            throw new InvalidOperationException("Already published.");

        if (!IsApproved)
            throw new InvalidOperationException("Cannot publish until approved.");

        PublishedBy = by;
        PublishedOn = DateTime.Today;
        IsPublished = true;

        Raise(new PublishedEvent(Title));
    }
}

I want to write unit tests that verify:

  • I cannot publish unless the Document has been approved
  • I cannot re-publish a Document
  • When published, the PublishedBy and PublishedOn values are properly set
  • When publised, the PublishedEvent is raised

Without access to the setters, I cannot put the object into the state needed to perform the tests. Opening access to the setters defeats the purpose of preventing access.

How do(have) you solve(d) this problem?

share|improve this question
    
The more I think about this, the more I think that your entire problem is having methods with side-effects. Or rather, a mutable immutable object. In a DDD-world, should you not return a new Document object from both Approve and Publish, rather than updating the internal state of this object? –  pdr Mar 18 '13 at 18:02
1  
Quick question, which O/RM are you using. I'm a big fan of EF but declaring setters as protected does rub me the wrong way a bit. –  Mike Brown Mar 18 '13 at 21:23
    
We have a mix right now because of the free-range development I have been charged with wrangling in. Some ADO.NET using AutoMapper to hydrate from a DataReader, a couple of Linq-to-SQL models (that will be the next to replace) and some new EF models. –  SonOfPirate Mar 19 '13 at 14:49
    
Calling Publish twice is not smelly at all and is the way to do it. –  Peri Mar 21 '13 at 20:21
add comment

5 Answers

up vote 22 down vote accepted

I cannot put the object into the state needed to perform the tests.

If you cannot put the object into the state needed to perform a test, then you cannot put the object into the state in production code, so there's no need to test that state. Obviously, this isn't true in your case, you can put your object into the needed state, just call Approve.

  • I cannot publish unless the Document has been approved: write a test that calling publish before calling approve causes the right error without changing the object state.

    void testPublishBeforeApprove() {
        doc = new Document("Doc");
        AssertRaises(doc.publish, ..., NotApprovedException);
    }
    
  • I cannot re-publish a Document: write a test that approves an object, then calling publish once succeed, but second time causes the right error without changing the object state.

    void testRePublish() {
        doc = new Document("Doc");
        doc.approve();
        doc.publish();
        AssertRaises(doc.publish, ..., RepublishException);
    }
    
  • When published, the PublishedBy and PublishedOn values are properly set: write a test that calls approve then call publish, assert that the object state changes correctly

    void testPublish() {
        doc = new Document("Doc");
        doc.approve();
        doc.publish();
        Assert(doc.PublishedBy, ...);
        ...
    }
    
  • When publised, the PublishedEvent is raised: hook to the event system and set a flag to make sure it's called

You also need to write test for approve.

In other word, don't test the relation between internal fields and IsPublished and IsApproved, your test would be quite fragile if you do that since changing your field would mean changing your tests code, so the test would be quite pointless. Instead you should test the relationship between calls of public methods, this way, even if you modify the fields you wouldn't need to modify the test.

share|improve this answer
    
When Approve breaks, several tests break. You're no longer testing a unit of code, you're testing the full implementation. –  pdr Mar 18 '13 at 16:32
    
I share pdr's concern which is why I hesitated going this direction. Yes, it seems cleanest, but I don't like having multiple reasons an individual test can fail. –  SonOfPirate Mar 18 '13 at 16:40
4  
I've yet to see a unit test which could only fail for a single possible reason. Also, you can put the "state manipulation" parts of the test into a setup() method --- not the test itself. –  Peter K. Mar 18 '13 at 18:49
10  
Why is depending on approve() somehow brittle, yet depending on setApproved(true) somehow not? approve() is a legitimate dependency in the tests because it is a dependency in the requirements. If the dependency only existed in the tests, that would be another issue. –  Karl Bielefeldt Mar 18 '13 at 19:13
1  
@pdr, how would you test a stack class? Would you try to test the push() and pop() methods independently? –  Winston Ewert Mar 18 '13 at 23:28
show 22 more comments

Yet another approach is to create a constructor of the class that allows the internal properties to be set on instantiation:

 public Document(
  String approvedBy,
  DateTime? approvedOn,
  Boolean isApproved,
  Boolean isPublished,
  String publishedBy,
  DateTime? publishedOn,
  String title)
{
  ApprovedBy = approvedBy;
  ApprovedOn = approvedOn;
  IsApproved = isApproved;
  IsApproved = isApproved;
  PublishedBy = publishedBy;
  PublishedOn = publishedOn;
}
share|improve this answer
2  
This does not scale well at all. My object could have many more properties with any number of them having or not having values at any given point in the object's lifecycle. I follow the principal that constructors contain parameters for properties that are required for the object to be in a valid initial state or dependencies an object requires to function. The purpose of the properties in the example are to capture the current state as the object is manipulated. Having a constructor with every property or overloads with different combinations is a huge smell and, as I said, doesn't scale. –  SonOfPirate Mar 18 '13 at 16:46
    
Understood. Your example did not mention many more properties, and the number in the example is "on the cusp" of having this as a valid approach. It seems that this is telling you something about your design: you can't put your object into any valid state on instantiation. That means you need to put it into a valid initial state, and them manipulate it into the right state for test. That implies Lie Ryan's answer is the way to go. –  Peter K. Mar 18 '13 at 18:47
    
Even if object has one property and will never change this solution is bad. What stops anyone from using this constructor in production? How will you mark this constructor [TestOnly]? –  Peri Mar 21 '13 at 20:17
    
Why is it bad in production? (Really, I'd like to know). Sometimes it's necessary to recreate the precise state of an object at creation... not just a single valid initial object. –  Peter K. Mar 22 '13 at 0:07
1  
So while that helps put the object into a valid initial state, testing the behavior of the object at it progresses through its lifecycle requires that the object be changed from its initial state. My OP has to do with testing these additional states when you can't simply set properties to change the object's state. –  SonOfPirate Mar 27 '13 at 0:20
show 5 more comments

One strategy is that you inherit the class(in this case Document) and write tests against the inherited class. The inherited class allows some way to set the object state in tests.

In C# one strategy could be to make setters internal, then exposing internals to test project.

You could also use the class API like you described("I can certainly do this by calling Publish twice"). This would be setting object state using the public services of the object, it doesn't seem too smelly to me. In the case of your example, this would probably be the way I'd do it.

share|improve this answer
    
I thought about this as a possible solution but hesitated to make my properties overridable or expose the setters as protected because it felt like I was opening the object up and breaking encapsulation. I think making the properties protected is certainly better than public or even internal/friend. I will definitely give this approach more thought. It's simple and effective. Sometimes that's the best approach. If anyone disagrees, please add comments with specifics. –  SonOfPirate Mar 18 '13 at 16:49
add comment

To test in absolute isolation the commands and queries that the domain objects receive, I'm used to supply each test with a serialization of the object in the expected state. In the arrange section of the test, it loads the object to test from a file that I have previously prepared. At first I started with binary serializations, but json has proved to be a lot easier to mantain. This proved to work well, whenever absolute isolation in tests provides actual value.

edit just a note, some times JSON serialization fails (as in case of cyclic object's graphs, that are a smell, btw). In such situations, I rescue to binary serialization. It's a bit pragmatic, but works. :-)

share|improve this answer
    
And how do you prepare object in expected state if there is no setter and you don't want to call it's public methods to set it up? –  Peri Mar 21 '13 at 20:19
    
I have written a small tool for that. It load a class by reflection that create a new entity using it's public constructor (typically taking just the identifier) and invoke an array of Action<TEntity> on it, saving a snapshot after each operation (with a conventional name based on the action's index and the entity name). The tool is executed manually at each refactoring of the entity code and the snapshots are tracked by the DCVS. Obviously each Action calls a public command of the entity, but this is done out of the tests runs that this way are truly Unit test. –  Giacomo Tesio Mar 21 '13 at 22:25
    
I don't understand how that changes anything. If it still calls public methods on sut (system under test) then its no different then just calling those methods in the test. –  Peri Mar 21 '13 at 23:01
    
After the snapshots are produced, they are stored in files. Each tests does not depend on the sequence of the operations required to obtain the starting state of the entity, but from the state itself (loaded from the snapshot). The method under test itself is then isolated from changes to the other methods. –  Giacomo Tesio Mar 21 '13 at 23:31
    
What when someone changes the public method that was used to prepare serialized state for your tests but forgets to run the tool to regenerate the serialized object? Tests still are green even if there is an error in code. Still I say this doesn't change anything. You still run public methods so setup the objects you test. But you run them long before tests are run. –  Peri Mar 21 '13 at 23:38
show 6 more comments

You say

do try to apply best practices whenever possible

and

the ORM we are using to hydrate the objects uses reflection so it is able to access private setters

and I have to think that using reflection to bypass access controls on your classes is not what I'd describe as "best practice". Its going to be horribly slow too.


Personally, I would scrap your unit test framework and go with something in the class - it seems that you're writing tests from the viewpoint of testing the entire class anyway, which is good. In the past, for some tricky components that needed testing, I ave embedded the asserts and setup code into the class itself (it used to be a common design pattern to have a test() method in every class), so you create a client that simply instantiates an object and calls the test method which can set itself up as you like without nastiness like reflection hacks.

If you're concerned about code bloat, just wrap the testing methods in #ifdefs to make them only available in debug code (probably a best practice itself)

share|improve this answer
3  
-1: Scrapping your testing framework and going back to test methods inside the class would be going back to the dark ages of unit testing. –  Robert Johnson Mar 18 '13 at 13:43
9  
No -1 from me, but including test code in production is generally a Bad Thing(TM). –  Peter K. Mar 18 '13 at 13:44
    
what else does the OP do? Stick to screwing with private setters?! Its like choosing which poison you want to drink. My suggestion to the OP was to put the unit test into debug code, not production. In my experience, putting unit tests into a different project just means that project gets closely tied to the original anyway, so from a dev PoV, there's little distinction. –  gbjbaanb Mar 18 '13 at 23:31
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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