I live over in .NET world, but read the blog for more test knowledge.
I've read a great deal about mocks on this blog - surely the last example would not be as bad if you used mocks instead of instantiating everything?
Furthermore, over in .NET world there's a new term called an automocker - I guess you could view it as a hybrid between an IoC container and a mocking framework.
It would let you do exactly as you described your ideal world, but without the null values.
Check out this link for more info or search the web for automocker...
I don't really have an informed opinion one way or another on the use of asserts.
But, if you really wanted to keep your asserts in the constructor another way to approach the problem would be to use builders to construct your test objects. Something like:
Door door = new Door(); Window window = new Window(); House house = HouseBuilder.new().with(door).with(window);
I think that a pretty clear way to tackle the problem. Where the HouseBuilder provides some sensible defaults for the other variables. This also solves the problem of not creating objects in an invalid state
I disagree because in your case one needs to know about the fact that the lock() method you are testing only uses the door and window, and does not use anything else.
This results in your testers having to know about the implementation, which should not be the case... maybe my house has a folding roof just like convertibles have, and I have to lock that too, but it is null.
The only reason you know the secure methods only needs those two objects is because you know the code.
What if Secure() activated motion sensors in the Kitchen, LivingRoom and BedRoom?
Maybe it is just a bad example, but from what I see, this test code needs to know wayyyy too much about what is going on already..
I would probably have some sort of ISecurable interface and would test each object independantly.. You then don't give a crap what the house is doing since you know that all it is doing is calling ISecurable.Secure(), and each implementer you have tested, right?
If the ability to add in an interface is not an option, then it really becomes a moot point, you cant change the code anyway.. So just get an IoC container in there and make the hurt go away :)
Misko, I completely agree with you. Those asserts (other null asserts and guard clauses) are redundant and get in the way (not only in the way of the test but of the reader of the production code). Not to mention in the case in question, it'd be the responsibility of your DI framework to not pass in a null (in production), not the responsibility of this class.
This is exactly why people should use an AMC (Auto mocking container) when testing. Your tests become much less brittle, much smaller (less setup), and more intention revealing.
Using a builder is another solution but also comes with cost of just pushing the setup code to a common a place that still needs to be maintained. Typically you would only keep those builders in test assemblies which provides little value when you consider an AMC can do all that for free.
When you add a StairWell parameter to the constructor none of your existing tests will break with the AMC. With the builder approach or non-builder approach it will need to be refactored.
Removing the asserts all together is the most dangerous as now you are allowing for objects to be created with an inconsistent state. Very shortly you will start seeing those null tests scattered throughout the system and SRP and LOD start to scream when that happens.
In the code I've written and seen, we assert not null before an object is used, not when they are stored. Is this reasonable? That would put Assert.notNull(door) and Assert.notNull(window) in your secure() method. Verifying the presence of your objects before using them makes much more sense to me than verifying the presence of all objects you might be concerned with maybe someday.
To Howie, the null-checking assertion are made to ensure that the object's state is right before using it. The example above is quite simple, but assume that you have a code where there are several places in which the "kitchen" or "bedroom" fields are set independently one to the other.
If you don't check the consistance of the state in those places, you won't be able (whitout a complete suite of unit test) to easily identify "where" your kitchen has been set to null.
I agree to earlNameless... in that way to test object's methods, you need to know the method's source code (behavior) to determine the entities involved in the test. Mysko teached us about the importance of the explicit declaration of the dependancies: so, if our House uses, in a part of the code, just a small part of the House's dependencies, it's a sympthom that there is something wrong with House design: maybe, you don't need an House but just an HouseLockingSystem, which needs to know only about House's doors and window (not about kitchen, or bedroom, or everything else...)
My thoughts : You need to differentiate between a valid null and an invalid null
make sure you set up a constructor for a valid house only with a door and a window parameter, fill the rest with "Null objects" http://en.wikipedia.org/wiki/Null_Object_pattern
than you don't need the clumsy constructor, also you don't need a lot of nulls, you keepm the logic internal to your house and you know the difference between a valid null and invalid null.
There may be another alternative to explore: tag parameters with nullness annotations such as in JSR-305 (http://jcp.org/en/jsr/detail?id=305).
It would have some distinct advantages: 1. People using the constructor know that null is not supported just by looking for @Nonnull parameters in the constructor's signature rather than having to look up the javadoc. 2. You don't have to bother with writing and maintaining statements like "null is not permitted" in the javadoc, which often seem to be lacking or out of synch with the actual code. 3. All the Assert.notNull statements can be removed from the constructor, making it much cleaner again. 4. Users with up-to-date static analysis tools (such as FindBugs) get a warning right away when passing null instead of waiting for an assertion error at runtime. 5. You don't have to write a bunch of tests that check that null really is prohibited as stated in the javadoc. (This is what led me to search for this functionality in the first place - these tests were getting to be really numerous and time-consuming).
For the test, you could still pass in null where you want to and suppress the generated warnings. You of course run the risk of using the constructor in a manner that is clearly stated as not being supported, and are relying upon knowledge of the internal implementation as others have mentioned. I guess in that case, at least the test code has been warned of its risky usage and can be updated if the internals of the class under test change.
That said, I'd feel a lot more comfortable if these warnings were generated by the compiler itself instead of relying upon third party tools (which other users may lack, thereby allowing them to miss the warnings).
I'm also not sure how to balance this against using a DI framework since I don't have much experience with any of them. I think Misko has a good point that it clutters the test though, even if the grunt-work of creating the objects is handled elsewhere.
So I guess the fundamental question is, do you want to: 1. Take a bit of a gamble by ignoring some of the contract for the class under test so that the test can be simplified -or- 2. Follow the contract exactly as stated up-front by providing all the necessary dependencies and making it a bit more difficult to sift through the internals of a failing test (due to the existence of unrelated objects)?
Knowing the implementation in order to write the test is expected. This is TDD not TAD right? If the roof needs locking, change the specification, the test fails, then change the implementation so everything turns green.
The test is the verification of how the system works. I personally find it easier and quicker to scan the specifications to find out how things work rather than reading implementation code.
As for removing assertions from the constructor I have to again disagree. When a client comes around trying to consume the api there needs to be a fast fail if the object is in an inconsistent state.
@Kyle K... I would like to see a full blown DBC specification for java, something like spec# in .net land. http://research.microsoft.com/en-us/projects/specsharp/
I would venture that a solution here would be to pass mocks (auto-generated by some library), which are by definition non null.
Plus when the code change and your method suddenly needs to do something on another one of its dependencies, you get a message like "unexpected call to mock("kitchen").clean" instead of "NullPointerException" ...
I do disagree Miško .. The problem is this case is the logic of assertion behind the program as a programmer they have to check all the possible input for argument . I am a tester but i would recommend assertion is to be more in the case of web programming and interactive modules.
Several comments have taken issue with the fact that the test code Misko discusses would have to know details about the implementation. While it is true that there is a certain brittleness where test code knows internal details - this is unit testing, which is, by nature, whitebox, not black box. If you're using mocks (instead of fakes) then you definitely should know about the internals... specifically, you're testing that your code does what you expect, given certain conditions. You do have to know what those conditions may be, and you can exclude conditions that you know won't be.
If someone comes along and changes the implementation such that those assumptions aren't valid, that will break the tests. But, really, that's the point. You have a suite of unit tests which form a regression. If I change the behaviour, I change the meaning of the test. I therefore have to also change the test to be valid for the new assumptions.
A lot of this depends on whether you're testing behaviour or input-output, and that's a test-design philosophy. TDD doesn't assert behaviour driven, but use of behavioural mocks does tend to indicate this approach.
Null is not an object. Null is a way of making obects optional. You are really asking that *all constructor arguments be optional*, which seems wrong to me.
If the object is documented as doing this, then it must put all kinds of crazy null checks that verbosify the code and can't possibly do anything sensible anyway. I know this is fairly standard in Java, but that doesn't make it a good thing.
That Java even *lets* you pass in a non-object for all reference types is a flaw in the langauge's type system. ML and C++ for instance have methods for specifying required reference parameters. I hear that future version of Java will getting @NonNull as standard as well?
Maybe it makes testing a little easier, but making all constructor arguments optional strikes me as a good way to make the implementation of the class more verbose than it needs to be.
Also, making constructor arguments optional will in some cases make you *return* null maybe even unintentionally, which is *always the wrong thing to do*.
"Now let's say that i want to test the secure() method. The secure method needs door and window."
You are testing the contract. If the secure() method needs to go to the LivingRoom and get a torch, that's not your problem. In order to operate correctly, the House needs this stuff. Mock it! If you wrote to interfaces, you'd be able to mock at top level and not worry about "creating so many objects". Then a house with a BrightlyLitDoor (implements Door) would not need a torch from the LivingRoom, and a House with a MagicWindow would not even need the Window to be secure.
In Spring, I usually do this in InitializingBean.afterPropertiesSet(). That way the framework can validate that my bean is wired properly, but I don't necessarily need to call this method if I don't want to in my tests.
I think there are two separate issues here: 1. do you allow the user to pass NULL values instead of objects. 2. To assert or not to assert.
First you need to decide if it is valid for the method to accept NULL values. Then you need to assert all inputs for valid values (be it NULL, or what ever other values).
I would agree with neronotte, maybe there is a flaw in the design of the house. Maybe the fact that I want to ignore some parameters of the constructor, because I know my test won't need, them is a 'smell'? There could be a hidden object creeping around...Though not always, as it seems reasonable that not every method of an object uses every instance field.
As with the 'to assert or not' question, Effective Java, Item 38 suggests that methods that are not to be exposed should use Java's assert construct, which can be turned off. Publicly exposed methods should explicitly check and throw exceptions (as in Miško's example). That seems sensible to me. Therefore, I would check that I'm not passed null objects when I explicitely forbid them (i.e. in the javadoc) in my API, and add unit tests for those corner cases. And I'd rather use Dummy Objects instead of null (based on the test double taxonomy at http://xunitpatterns.com/Mocks,%20Fakes,%20Stubs%20and%20Dummies.html), to make the tests work in both situations. (Or maybe we have a point in favour of the setter injection here ;-)
It's interesting, however, to see how unit testing has an impact on design and coding practices. We're trying to exercise very specific parts and/or behaviours of a software that are meant to exist in the midst of other pieces. Unit testing entails extra thinking so as to be able to isolate each and every part of a software and to use them easily outside of their expected environment.
One sentence in your post really got me there, and i think worth a comment by itself: "null is a great way to tell the reader, "these are not the objects you are looking for"".
I have to say I disagree. the behaviour of null object is to cause a null pointer exception when accessed, and a possibly cryptic stack dump. there has to be a very good reason to choose this path. why not create a mock object instead of null, using constructor injection, that exports more information and possibly even communicates meaningful results back to the testing code when accessed?
(for bonus points, a framework could possibly generate these 'tripwire' items for you automagically using reflection, and even restart the test with a real item instead of a tripwire, just in case)
Compare these two outputs:
output 1: java.lang.NullPointerException at line 44 (then, 32 more lines down the road...) at getRoom() (line 63) (aha! we crashed on the room being null... - too bad this text isn't part of the stackdump!)
output 2: TripWire: getRoomName() got unexpectedly accessed in testSecureHouse() - Consult business logic to see if this makes sense and fix your test or your bug. stackdump logged. exception caught - Restarting test with real LivingRoom object. test passed.
and the calling constructor difference?
case 1: House house = new House(door, window, null, null, null, null);
case 2: House House(door, window, TripWire(roof), TripWire(kitchen), TripWire(livingRoom), TripWire(bedRoom)).
Please proof read your blog. :)
ReplyDeleteHey,
ReplyDeleteI live over in .NET world, but read the blog for more test knowledge.
I've read a great deal about mocks on this blog - surely the last example would not be as bad if you used mocks instead of instantiating everything?
Furthermore, over in .NET world there's a new term called an automocker - I guess you could view it as a hybrid between an IoC container and a mocking framework.
It would let you do exactly as you described your ideal world, but without the null values.
Check out this link for more info or search the web for automocker...
http://blog.eleutian.com/CommentView,guid,762249da-e25a-4503-8f20-c6d59b1a69bc.aspx
:-)
I don't really have an informed opinion one way or another on the use of asserts.
ReplyDeleteBut, if you really wanted to keep your asserts in the constructor another way to approach the problem would be to use builders to construct your test objects. Something like:
Door door = new Door();
Window window = new Window();
House house = HouseBuilder.new().with(door).with(window);
house.secure();
assertTrue(door.isLocked());
assertTrue(window.isClosed());
I think that a pretty clear way to tackle the problem. Where the HouseBuilder provides some sensible defaults for the other variables. This also solves the problem of not creating objects in an invalid state
I disagree because in your case one needs to know about the fact that the lock() method you are testing only uses the door and window, and does not use anything else.
ReplyDeleteThis results in your testers having to know about the implementation, which should not be the case... maybe my house has a folding roof just like convertibles have, and I have to lock that too, but it is null.
Were you going somewhere with the external API? It seems to trail off mid-sentence although I was interested in it.
ReplyDeleteI totally agree with earlNameless..
ReplyDeleteThe only reason you know the secure methods only needs those two objects is because you know the code.
What if Secure() activated motion sensors in the Kitchen, LivingRoom and BedRoom?
Maybe it is just a bad example, but from what I see, this test code needs to know wayyyy too much about what is going on already..
I would probably have some sort of ISecurable interface and would test each object independantly.. You then don't give a crap what the house is doing since you know that all it is doing is calling ISecurable.Secure(), and each implementer you have tested, right?
If the ability to add in an interface is not an option, then it really becomes a moot point, you cant change the code anyway.. So just get an IoC container in there and make the hurt go away :)
Misko, I completely agree with you. Those asserts (other null asserts and guard clauses) are redundant and get in the way (not only in the way of the test but of the reader of the production code). Not to mention in the case in question, it'd be the responsibility of your DI framework to not pass in a null (in production), not the responsibility of this class.
ReplyDeleteThis is exactly why people should use an AMC (Auto mocking container) when testing. Your tests become much less brittle, much smaller (less setup), and more intention revealing.
ReplyDeleteUsing a builder is another solution but also comes with cost of just pushing the setup code to a common a place that still needs to be maintained. Typically you would only keep those builders in test assemblies which provides little value when you consider an AMC can do all that for free.
When you add a StairWell parameter to the constructor none of your existing tests will break with the AMC. With the builder approach or non-builder approach it will need to be refactored.
Removing the asserts all together is the most dangerous as now you are allowing for objects to be created with an inconsistent state. Very shortly you will start seeing those null tests scattered throughout the system and SRP and LOD start to scream when that happens.
In the code I've written and seen, we assert not null before an object is used, not when they are stored. Is this reasonable? That would put Assert.notNull(door) and Assert.notNull(window) in your secure() method. Verifying the presence of your objects before using them makes much more sense to me than verifying the presence of all objects you might be concerned with maybe someday.
ReplyDeleteThis comment has been removed by the author.
ReplyDeleteTo Howie,
ReplyDeletethe null-checking assertion are made to ensure that the object's state is right before using it.
The example above is quite simple, but assume that you have a code where there are several places
in which the "kitchen" or "bedroom" fields are set independently one to the other.
If you don't check the consistance of the state in those places, you won't be able (whitout a
complete suite of unit test) to easily identify "where" your kitchen has been set to null.
I agree to earlNameless... in that way to test object's methods, you need to know the method's
source code (behavior) to determine the entities involved in the test. Mysko teached us about the
importance of the explicit declaration of the dependancies: so, if our House uses, in a part of the
code, just a small part of the House's dependencies, it's a sympthom that there is something wrong
with House design: maybe, you don't need an House but just an HouseLockingSystem, which
needs to know only about House's doors and window (not about kitchen, or bedroom, or everything else...)
Hi,
ReplyDeleteNice discussion.
My thoughts :
You need to differentiate between a valid null and an invalid null
make sure you set up a constructor for a valid house only with a door and a window parameter, fill the rest with "Null objects" http://en.wikipedia.org/wiki/Null_Object_pattern
than you don't need the clumsy constructor, also you don't need a lot of nulls, you keepm the logic internal to your house and you know the difference between a valid null and invalid null.
What do you think ?
Sometimes ago I blogged about ways of indicating the absence of an object:
ReplyDeletehttp://rnaufal.livejournal.com/10017.html
One interesting comment I received about using Assert instead of null checking:
"There's a document in the JDK docs titled Programming With Assertions that says (http://java.sun.com/j2se/1.4.2/docs/guide/lang/assert.html#usage):
Do not use assertions for argument checking in public methods"
Great discussion, Misko.
ReplyDeleteThere may be another alternative to explore: tag parameters with nullness annotations such as in JSR-305 (http://jcp.org/en/jsr/detail?id=305).
It would have some distinct advantages:
1. People using the constructor know that null is not supported just by looking for @Nonnull parameters in the constructor's signature rather than having to look up the javadoc.
2. You don't have to bother with writing and maintaining statements like "null is not permitted" in the javadoc, which often seem to be lacking or out of synch with the actual code.
3. All the Assert.notNull statements can be removed from the constructor, making it much cleaner again.
4. Users with up-to-date static analysis tools (such as FindBugs) get a warning right away when passing null instead of waiting for an assertion error at runtime.
5. You don't have to write a bunch of tests that check that null really is prohibited as stated in the javadoc. (This is what led me to search for this functionality in the first place - these tests were getting to be really numerous and time-consuming).
For the test, you could still pass in null where you want to and suppress the generated warnings. You of course run the risk of using the constructor in a manner that is clearly stated as not being supported, and are relying upon knowledge of the internal implementation as others have mentioned. I guess in that case, at least the test code has been warned of its risky usage and can be updated if the internals of the class under test change.
That said, I'd feel a lot more comfortable if these warnings were generated by the compiler itself instead of relying upon third party tools (which other users may lack, thereby allowing them to miss the warnings).
I'm also not sure how to balance this against using a DI framework since I don't have much experience with any of them. I think Misko has a good point that it clutters the test though, even if the grunt-work of creating the objects is handled elsewhere.
So I guess the fundamental question is, do you want to:
1. Take a bit of a gamble by ignoring some of the contract for the class under test so that the test can be simplified -or-
2. Follow the contract exactly as stated up-front by providing all the necessary dependencies and making it a bit more difficult to sift through the internals of a failing test (due to the existence of unrelated objects)?
Knowing the implementation in order to write the test is expected. This is TDD not TAD right? If the roof needs locking, change the specification, the test fails, then change the implementation so everything turns green.
ReplyDeleteThe test is the verification of how the system works. I personally find it easier and quicker to scan the specifications to find out how things work rather than reading implementation code.
As for removing assertions from the constructor I have to again disagree. When a client comes around trying to consume the api there needs to be a fast fail if the object is in an inconsistent state.
@Kyle K... I would like to see a full blown DBC specification for java, something like spec# in .net land. http://research.microsoft.com/en-us/projects/specsharp/
I would venture that a solution here would be to pass mocks (auto-generated by some library), which are by definition non null.
ReplyDeletePlus when the code change and your method suddenly needs to do something on another one of its dependencies, you get a message like "unexpected call to mock("kitchen").clean" instead of "NullPointerException" ...
Cheers
PH
I do disagree Miško .. The problem is this case is the logic of assertion behind the program as a programmer they have to check all the possible input for argument . I am a tester but i would recommend assertion is to be more in the case of web programming and interactive modules.
ReplyDeleteSeveral comments have taken issue with the fact that the test code Misko discusses would have to know details about the implementation. While it is true that there is a certain brittleness where test code knows internal details - this is unit testing, which is, by nature, whitebox, not black box. If you're using mocks (instead of fakes) then you definitely should know about the internals... specifically, you're testing that your code does what you expect, given certain conditions. You do have to know what those conditions may be, and you can exclude conditions that you know won't be.
ReplyDeleteIf someone comes along and changes the implementation such that those assumptions aren't valid, that will break the tests. But, really, that's the point. You have a suite of unit tests which form a regression. If I change the behaviour, I change the meaning of the test. I therefore have to also change the test to be valid for the new assumptions.
A lot of this depends on whether you're testing behaviour or input-output, and that's a test-design philosophy. TDD doesn't assert behaviour driven, but use of behavioural mocks does tend to indicate this approach.
I'm very skeptical of this.
ReplyDeleteNull is not an object. Null is a way of making obects optional. You are really asking that *all constructor arguments be optional*, which seems wrong to me.
If the object is documented as doing this, then it must put all kinds of crazy null checks that verbosify the code and can't possibly do anything sensible anyway. I know this is fairly standard in Java, but that doesn't make it a good thing.
That Java even *lets* you pass in a non-object for all reference types is a flaw in the langauge's type system. ML and C++ for instance have methods for specifying required reference parameters. I hear that future version of Java will getting @NonNull as standard as well?
Maybe it makes testing a little easier, but making all constructor arguments optional strikes me as a good way to make the implementation of the class more verbose than it needs to be.
Also, making constructor arguments optional will in some cases make you *return* null maybe even unintentionally, which is *always the wrong thing to do*.
Can't you just switch to the assert keyword and disable assertions when running unit tests?
ReplyDelete"Now let's say that i want to test the secure() method. The secure method needs door and window."
ReplyDeleteYou are testing the contract. If the secure() method needs to go to the LivingRoom and get a torch, that's not your problem. In order to operate correctly, the House needs this stuff. Mock it! If you wrote to interfaces, you'd be able to mock at top level and not worry about "creating so many objects". Then a house with a BrightlyLitDoor (implements Door) would not need a torch from the LivingRoom, and a House with a MagicWindow would not even need the Window to be secure.
In Spring, I usually do this in InitializingBean.afterPropertiesSet(). That way the framework can validate that my bean is wired properly, but I don't necessarily need to call this method if I don't want to in my tests.
ReplyDeleteI think there are two separate issues here:
ReplyDelete1. do you allow the user to pass NULL values instead of objects.
2. To assert or not to assert.
First you need to decide if it is valid for the method to accept NULL values.
Then you need to assert all inputs for valid values (be it NULL, or what ever other values).
Very interesting discussion.
ReplyDeleteI would agree with neronotte, maybe there is a flaw in the design of the house. Maybe the fact that I want to ignore some parameters of the constructor, because I know my test won't need, them is a 'smell'? There could be a hidden object creeping around...Though not always, as it seems reasonable that not every method of an object uses every instance field.
As with the 'to assert or not' question, Effective Java, Item 38 suggests that methods that are not to be exposed should use Java's assert construct, which can be turned off. Publicly exposed methods should explicitly check and throw exceptions (as in Miško's example). That seems sensible to me. Therefore, I would check that I'm not passed null objects when I explicitely forbid them (i.e. in the javadoc) in my API, and add unit tests for those corner cases. And I'd rather use Dummy Objects instead of null (based on the test double taxonomy at http://xunitpatterns.com/Mocks,%20Fakes,%20Stubs%20and%20Dummies.html), to make the tests work in both situations. (Or maybe we have a point in favour of the setter injection here ;-)
It's interesting, however, to see how unit testing has an impact on design and coding practices. We're trying to exercise very specific parts and/or behaviours of a software that are meant to exist in the midst of other pieces. Unit testing entails extra thinking so as to be able to isolate each and every part of a software and to use them easily outside of their expected environment.
This comment has been removed by the author.
ReplyDeleteI'm totally disagree with you, each object must make sure its can operate the designed functionality, can use see if you don't have an eyes?
ReplyDeleteWell, what about
class TestHouse{
private House house;
public TestHouse(){
//create a new house object
}
@Test
public void testSecureHouse() {
//do you test
}
}
or
public void testSecureHouse() {
//Now Assert aware and will not
//Throw exceptions
Assert.setTesting(true);
//do your tests
}
Hello Misko,
ReplyDeleteOne sentence in your post really got me there, and i think worth a comment by itself: "null is a great way to tell the reader, "these are not the objects you are looking for"".
I have to say I disagree. the behaviour of null object is to cause a null pointer exception when accessed, and a possibly cryptic stack dump. there has to be a very good reason to choose this path. why not create a mock object instead of null, using constructor injection, that exports more information and possibly even communicates meaningful results back to the testing code when accessed?
(for bonus points, a framework could possibly generate these 'tripwire' items for you automagically using reflection, and even restart the test with a real item instead of a tripwire, just in case)
Compare these two outputs:
output 1:
java.lang.NullPointerException at line 44
(then, 32 more lines down the road...)
at getRoom() (line 63) (aha! we crashed on the room being null... - too bad this text isn't part of the stackdump!)
output 2:
TripWire: getRoomName() got unexpectedly accessed in testSecureHouse() - Consult business logic to see if this makes sense and fix your test or your bug. stackdump logged.
exception caught - Restarting test with real LivingRoom object.
test passed.
and the calling constructor difference?
case 1:
House house = new House(door, window,
null, null, null, null);
case 2:
House House(door, window,
TripWire(roof), TripWire(kitchen), TripWire(livingRoom), TripWire(bedRoom)).