tags:

views:

376

answers:

3

I've accidentally stumbled upon an old article by Luke Redpath that presents a very simple BDD example (very short and easy to follow even for non-Ruby programmers like me). I found the end result very incomplete, thus making the example pretty useless.

The end result is a single test which verifies that a user with preset attributes is valid. In my view, this is simply not enough to verify the validation rules correctly. For example, if you change

validates_length_of :password, :in => 6..12, :allow_nil => :true

to

validates_length_of :password, :in => 7..8, :allow_nil => :true

(or even remove password length validation completely) the test will still pass, but you can obviously see the code is now violating the initial requirements.

I just think the last refactoring of putting all the individual tests into a single one is simply not enough. He tests only the "happy path" which doesn't guarantee much. I would absolutely have all the tests that verify that the correct error is triggered with certain values. In the case of the password, I would test that a password of length less than 6 and greater than 12 is invalid and triggers the appropriate error. The "happy path" test would be there as well, but not alone by itself as it's in the article.

What's your opinion? I'm just trying to figure out why the guy did it the way he did and whether he simply overlooked the problem or it was his intention. I may be missing something.

A: 

I don't have time to read the article, so I can't verify your claims, but the general answer in my opinion is that if the password validation rule is a concrete requirement, it should be verified with one or more tests for that specific requirement (at least one per "part" of the requirement).

Håvard S
+1  A: 

BDD (and TDD) are design activities. The tests are meant to drive the design of the code, not guarantee that it is completely bug-free. There should be independent testers for that. So we need a decent degree of coverage, to ensure that our code works as expected and handles exceptions in a clean fashion. But TDD doesn't demand that we write unit tests for every conceivable edge case.

With regard to the specific example you cite, perhaps he should have coded two tests, one with a password of six characters, one with a passowrd of twelve characters. But what would be the point? We know that the requirement is the password must be between six and twelve characters in length. If we have misunderstood the requirements and think the rule ought to be ...

validates_length_of :password, :in => 7..8, :allow_nil => :true

... then we're going to write our test data to make a test which passes our incorrect interpretation. So writing more tests would only give us misplaced confidence. That's why proponents of TDD and BDD favour other XP techniques like pair programming as well: to catch the errors we introduce into our unit tests.

Similarly, we could remove the test validating the password length altogether, but what would be the point? The tests are there to help us correctly implement the spceification. If we don't have tests for every piece of code we write then we are not doing TDD/BDD.

APC
It seems to me that nobody bothers to RTFA. `validates_length_of :password, :in => 6..12, :allow_nil => :true` is not an assertion at all. It is the implemenation (it's model code).
Ree
Actually I did read the article but I don't know Ruby, so I kinda skimmed the actual code. I apologise for misunderstanding/misrepresenting Luke's implementation.
APC
I completely agree with APC's first paragraph here. We ran across this "we only test good cases?" ourselves lately, and I explained it to my team that BDD is a DESIGN activity - you are designing the Behaviour of your code with BDD. I argued that, "If you want to build in some error logic within the object you are designing, then write another Scenerio that describes that error Context, and define the behaviours of what happens during that error in your Specs." If you want complete code coverage, you write seperate unit tests for all of that - TDD style if you like.
eduncan911
+1  A: 

I don't quite understand your question. The specs do contain expectations about the password lenght, both for the happy path and the two different failure modes (password too long and password too short):

specify "should be valid with a full set of valid attributes" do
  @user.attributes = valid_user_attributes
  @user.should_be_valid
end

This takes care of the happy path, since valid_user_attributes contains a valid password.

specify "should be invalid if password is not between 6 and 12 characters in length" do
  @user.attributes = valid_user_attributes.except(:password)
  @user.password = 'abcdefghijklm'
  @user.should_not_be_valid
  @user.password = 'abcde'
  @user.should_not_be_valid
end

And this tests the two failure modes.

Granted, there is one boundary case missing (12 characters), but that's not too bad.

Jörg W Mittag
Yes, they did until the author refactored all specs into a single one (the first snippet in your answer), thus leaving only the "happy path" test in place.
Ree
Huh? According to the article, the last refactoring step is to *extract* the redundant expectations into a single spec, instead of repeating the exact same expectation over and over again in each spec. At no point in the entire article does it say that all the specs are *replaced* by that single one. Indeed, it would be *impossible* to refactor all specs into this single one, because refactoring *must* always be behavior-preserving, and deleting all the specs does obviously *not* preserve the behavior, as you so rightly pointed out.
Jörg W Mittag