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?
4
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.
<code>void testPublishBeforeApprove() {doc = new Document("Doc");AssertRaises(doc.publish, ..., NotApprovedException);}</code><code>void testPublishBeforeApprove() { doc = new Document("Doc"); AssertRaises(doc.publish, ..., NotApprovedException); } </code>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.
<code>void testRePublish() {doc = new Document("Doc");doc.approve();doc.publish();AssertRaises(doc.publish, ..., RepublishException);}</code><code>void testRePublish() { doc = new Document("Doc"); doc.approve(); doc.publish(); AssertRaises(doc.publish, ..., RepublishException); } </code>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
<code>void testPublish() {doc = new Document("Doc");doc.approve();doc.publish();Assert(doc.PublishedBy, ...);...}</code><code>void testPublish() { doc = new Document("Doc"); doc.approve(); doc.publish(); Assert(doc.PublishedBy, ...); ... } </code>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.
27
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;
}
10
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.
1
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. 🙂
11
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)
3