The Problem
If you have watched this video: https://www.youtube.com/watch?v=PJjHfa5yxlU you will see around 30 mins mark, the code that looks like this:
This test does not test anything. She has stubbed her test, so it is meaningless. The test proves that a double class you created returns 47 in the assertion. You should never stub yourself out in a test. State based testing in this case is not the right choice. You can also pass junk values for the starting and ending values, the test will still pass. You can even pass empty params, the test will still pass.
You want exactly one test to fail for any given bug. This test will fail if you change 47 to any other number, string, nil or anything else. The emphasis of the current test is not on whether the message to total was sent or not.
Tests that are not written with their role as specifications in mind can be very confusing to read. The difficulty in understranding what they are testing can greatly reduce the velocity at which a codebase can be changed. Nat Pryce and Steve Freeman "Are your tests really driving your development?"
Actually this test tests that Foo#sales_total:
ReplyDelete- Takes 2 arguments
- Calls ::total method of a second argument
- Returns the result of ::total
It doesn't test only how Foo::sales_total forms :within argument for model::total, but probably this should be put into another test for separation of test responsibilities
Sandi stubbed Sale model because she was testing only Foo#sales_total, not Sale. She stubbed the dependancy of a method under test. Probably, Sale is an interface to some kind of a storage, so potentially Sale::total can be a heavy call which can unnecessarily slow down the test. There is no need to perform it when unittesting another model.
DateRange is another dependancy of a method under test, but it looks like forming a date range is not a heavy operation and probably it won't slow down the test. Ofcourse one can stub it as well, but I think it's not worth it.
I see that she is testing the sales_total, still you don't need to create a fake class to stub out the total method. You need to make sure the total method gets called when you invoke the sales_total. I still think state based testing in this scenario is a wrong choice. You should mock in this case.
DeleteSorry, I haven't read your article carefully enough. But I still think that she is right here. You can safely mock methods which belong to an thirdparty library because probably they won't change often, but when you mocking methods from your own app you can eventually change the interface in one place and forget to do so in some other. If somebody will ever rename or remove Sale::total, but forget to change Foo#sales_total, this test will still pass because of a mock, but the app will be broken.
ReplyDeleteSandi used double instead of a mock. When you introduce a double, you create another kind of a thing, another Totalizable in this case. Totalizable has an interface: it responds to ::total. Normally one would write a test for this interface and run this test for each Totalizable to ensure the interface is still supported. It will help to prevent an error that I described above.
Sandi writes about it in her book "Practical Object-Oriented Design in Ruby"
You should not be mocking a third-party library. You don't own the interface. Read the xunit test patterns by gerard meszaros or growing object-oriented software guided by tests by Steve Freeman for more details. However, you can stub them.
DeleteThe test does not check the total method, but the sales_total method, so stubbing it out is ok. Otherwise your unit test would implicitly test the behavior of total as well.
ReplyDeleteI feel like you've missed the point she makes *immediately* after that slide in the video.
ReplyDeleteYou say "the test proves that a double class you created returns 47 in the assertion". It does more than that. It proves that you can pass any class into `sales_total`, so long as it has a `total` method. That says a lot about the behaviour of Foo.
As Sandi mentions in the following few sentences, the *actual* behaviour of `Sale.total` (that it makes a Rails database query) is something that belongs in the `Sale` test and `Foo` shouldn't care about.
> "It may be that I have a test on total over there [in SaleTest], that actually runs a query"