radiant specs

[Jim Gay-2]

In an effort to hone my skill with RSpec, and to better understand  
Radiant internals, I've created a 'spec' branch in my repo
http://github.com/saturnflyer/radiant/tree/spec

Here I'm clarifying the specs to better describe the expected Radiant  
behavior. For example, where the specs might say "should  
validate_presence_of" you're forced to read the code examples to find  
out what that spec is talking about. I've rewritten it to say "should  
not save without a name" (or whatever the relevant attribute is).

Is this something that developers will find helpful?

I've also come across places where the existing tests and models check  
for 'numericality' of things like id and page_id. Why is this necessary?
I wouldn't expect that a test for an auto-generated field like id  
would need to be tested for its type. And fields like page_id are  
never edited by a user so I wouldn't expect that to need it's type  
tested either, so why bother even having that validation in the model?

I'd appreciate any enlightenment you can offer.

-Jim

--~--~---------~--~----~------------~-------~--~----~
Radiant CMS Dev Mailing List
Post:        radiantcms-dev@...
Unsubscribe: radiantcms-dev-unsubscribe@...
Group Site:  http://groups.google.com/group/radiantcms-dev/
-~----------~----~----~----~------~----~------~--~---

[Josh French]

Hi Jim,

That's the essence of behavior-driven development right there.  
Whereas classic unit testing may be redundant in the case where  
you're asserting the behavior of the framework, a spec is meant to  
explicitly specify all the behaviors that make up your object. Under  
perfect conditions, you should be able to read my spec and recreate  
my class. Granted, that's an awfully lofty goal, but using that as my  
philosophy of spec-writing helps me stay in the right frame of mind.  
(That said, I'm pretty sure there are some real head-scratchers in  
the spec suite that I'm responsible for. I'm sure you'll stumble  
across them sooner or later.)

I think your validation example is a perfect illustration of that,  
too. Declaring behaviors in more natural language is probably truer  
to the notion of "how should this thing behave?" Relying too heavily  
on the framework's own notation (it "should validate_presence_of")  
lets you write specs by copy-and-paste, without really considering  
the intent behind your code.

I wish I could remember who recommended this, but somewhere I picked  
up the habit, when faced with a failing spec, of really reading the  
specification before diving back into the code. With a failure  
message like "it should blah, but blorted instead" I ask myself,  
well... should it blah? Really?

Thanks for your efforts -- I do think it's a worthwhile project.

j

On Jul 5, 2008, at 8:33 AM, Jim Gay wrote:

>
> In an effort to hone my skill with RSpec, and to better understand
> Radiant internals, I've created a 'spec' branch in my repo
> http://github.com/saturnflyer/radiant/tree/spec
>
> Here I'm clarifying the specs to better describe the expected Radiant
> behavior. For example, where the specs might say "should
> validate_presence_of" you're forced to read the code examples to find
> out what that spec is talking about. I've rewritten it to say "should
> not save without a name" (or whatever the relevant attribute is).
>
> Is this something that developers will find helpful?
>
> I've also come across places where the existing tests and models check
> for 'numericality' of things like id and page_id. Why is this  
> necessary?
> I wouldn't expect that a test for an auto-generated field like id
> would need to be tested for its type. And fields like page_id are
> never edited by a user so I wouldn't expect that to need it's type
> tested either, so why bother even having that validation in the model?
>
> I'd appreciate any enlightenment you can offer.
>
> -Jim
>
> >

--~--~---------~--~----~------------~-------~--~----~
Radiant CMS Dev Mailing List
Post:        radiantcms-dev@...
Unsubscribe: radiantcms-dev-unsubscribe@...
Group Site:  http://groups.google.com/group/radiantcms-dev/
-~----------~----~----~----~------~----~------~--~---

[John W. Long-3]

Jim,

I tried to leave comments on the commits, but they weren't showing up.  
I've listed the commits below with my comments under them for clarity.


http://github.com/saturnflyer/radiant/commit/cfb1169c3f10143f0002e678d9faf241df270668
http://github.com/saturnflyer/radiant/commit/e641eddb832559cc29ab0b678ee35f5c228cd855

   I'm not sure I buy into breaking out the specs in this way. Custom  
assertions or
   matchers are a great way to DRY up code. If it's hard to  
understand, perhaps a
   comment in the source code with a link to to an article about it  
would be better?

   http://wiseheartdesign.com/2006/1/16/testing-rails-validations/

   I don't completely buy the "tests should be dumb" argument. Rspec  
is a custom domain
   language for testing. Some would argue that it doesn't help with  
clarity. But it's
   only unclear to those who have not taken the time to learn the  
"domain" that Rspec
   is fitted for. Once you understand the domain and how Rspec  
addresses it, the tests
   are clear. In the same way you shouldn't be afraid of writing  
matchers for your own
   application. It can greatly speed up the time it takes to write  
tests.

   Adam Williams has done some work to create Rspec matchers for  
validations:

   http://tinyurl.com/6y4owv

   You might have a look at that and see what it would take to  
incorporate his code
   into ours.

   To add a custom matcher to Radiant, drop it in the spec/matchers  
directory.


http://github.com/saturnflyer/radiant/commit/76df906ac37a55d29bb8cbd505ce5167366673f6
http://github.com/saturnflyer/radiant/commit/3b6521158b5534ea1e9dd2112d432973047115c8

   Nice work! These are much better.


I really appreciate your efforts in this area. Keep up the great work.

--
John Long
http://wiseheartdesign.com

On Jul 5, 2008, at 8:33 AM, Jim Gay wrote:

>
> In an effort to hone my skill with RSpec, and to better understand
> Radiant internals, I've created a 'spec' branch in my repo
> http://github.com/saturnflyer/radiant/tree/spec
>
> Here I'm clarifying the specs to better describe the expected Radiant
> behavior. For example, where the specs might say "should
> validate_presence_of" you're forced to read the code examples to find
> out what that spec is talking about. I've rewritten it to say "should
> not save without a name" (or whatever the relevant attribute is).
>
> Is this something that developers will find helpful?
>
> I've also come across places where the existing tests and models check
> for 'numericality' of things like id and page_id. Why is this  
> necessary?
> I wouldn't expect that a test for an auto-generated field like id
> would need to be tested for its type. And fields like page_id are
> never edited by a user so I wouldn't expect that to need it's type
> tested either, so why bother even having that validation in the model?
>
> I'd appreciate any enlightenment you can offer.
>
> -Jim
>
> >

--~--~---------~--~----~------------~-------~--~----~
Radiant CMS Dev Mailing List
Post:        radiantcms-dev@...
Unsubscribe: radiantcms-dev-unsubscribe@...
Group Site:  http://groups.google.com/group/radiantcms-dev/
-~----------~----~----~----~------~----~------~--~---

[Sean Cribbs-2]

Jim,

I second John's kudos.  Clarity is very important in specs.  Admittedly, some of them were hurriedly converted from test/unit and didn't completely embrace the RSpec style at first.  Keep moving forward with this!

Sean

John W. Long wrote:

Jim,

I tried to leave comments on the commits, but they weren't showing up.  
I've listed the commits below with my comments under them for clarity.


http://github.com/saturnflyer/radiant/commit/cfb1169c3f10143f0002e678d9faf241df270668
http://github.com/saturnflyer/radiant/commit/e641eddb832559cc29ab0b678ee35f5c228cd855

   I'm not sure I buy into breaking out the specs in this way. Custom  
assertions or
   matchers are a great way to DRY up code. If it's hard to  
understand, perhaps a
   comment in the source code with a link to to an article about it  
would be better?

   http://wiseheartdesign.com/2006/1/16/testing-rails-validations/

   I don't completely buy the "tests should be dumb" argument. Rspec  
is a custom domain
   language for testing. Some would argue that it doesn't help with  
clarity. But it's
   only unclear to those who have not taken the time to learn the  
"domain" that Rspec
   is fitted for. Once you understand the domain and how Rspec  
addresses it, the tests
   are clear. In the same way you shouldn't be afraid of writing  
matchers for your own
   application. It can greatly speed up the time it takes to write  
tests.

   Adam Williams has done some work to create Rspec matchers for  
validations:

   http://tinyurl.com/6y4owv

   You might have a look at that and see what it would take to  
incorporate his code
   into ours.

   To add a custom matcher to Radiant, drop it in the spec/matchers  
directory.


http://github.com/saturnflyer/radiant/commit/76df906ac37a55d29bb8cbd505ce5167366673f6
http://github.com/saturnflyer/radiant/commit/3b6521158b5534ea1e9dd2112d432973047115c8

   Nice work! These are much better.


I really appreciate your efforts in this area. Keep up the great work.

--
John Long
http://wiseheartdesign.com

On Jul 5, 2008, at 8:33 AM, Jim Gay wrote:
  

In an effort to hone my skill with RSpec, and to better understand
Radiant internals, I've created a 'spec' branch in my repo
http://github.com/saturnflyer/radiant/tree/spec

Here I'm clarifying the specs to better describe the expected Radiant
behavior. For example, where the specs might say "should
validate_presence_of" you're forced to read the code examples to find
out what that spec is talking about. I've rewritten it to say "should
not save without a name" (or whatever the relevant attribute is).

Is this something that developers will find helpful?

I've also come across places where the existing tests and models check
for 'numericality' of things like id and page_id. Why is this  
necessary?
I wouldn't expect that a test for an auto-generated field like id
would need to be tested for its type. And fields like page_id are
never edited by a user so I wouldn't expect that to need it's type
tested either, so why bother even having that validation in the model?

I'd appreciate any enlightenment you can offer.

-Jim

    

--~--~---------~--~----~------------~-------~--~----~
Radiant CMS Dev Mailing List
Post:        radiantcms-dev@...
Unsubscribe: radiantcms-dev-unsubscribe@...
Group Site:  http://groups.google.com/group/radiantcms-dev/
-~----------~----~----~----~------~----~------~--~---

[Jim Gay-2]

On Jul 5, 2008, at 12:02 PM, Sean Cribbs wrote:

I second John's kudos.  Clarity is very important in specs.  Admittedly, some of them were hurriedly converted from test/unit and didn't completely embrace the RSpec style at first. 

I knew that there was quite a bit of work that went into using RSpec and I'd also be inclined to just move my tests over rather than rewrite them all just to get the release of Radiant out the door.

Keep moving forward with this!

Will do. 

Jim,

I tried to leave comments on the commits, but they weren't showing up.  
I've listed the commits below with my comments under them for clarity.


http://github.com/saturnflyer/radiant/commit/cfb1169c3f10143f0002e678d9faf241df270668
http://github.com/saturnflyer/radiant/commit/e641eddb832559cc29ab0b678ee35f5c228cd855

   I'm not sure I buy into breaking out the specs in this way. Custom  
assertions or
   matchers are a great way to DRY up code. If it's hard to  
understand, perhaps a
   comment in the source code with a link to to an article about it  
would be better?

   http://wiseheartdesign.com/2006/1/16/testing-rails-validations/

   I don't completely buy the "tests should be dumb" argument. Rspec  
is a custom domain
   language for testing. Some would argue that it doesn't help with  
clarity. But it's
   only unclear to those who have not taken the time to learn the  
"domain" that Rspec
   is fitted for. Once you understand the domain and how Rspec  
addresses it, the tests
   are clear. In the same way you shouldn't be afraid of writing  
matchers for your own
   application. It can greatly speed up the time it takes to write  
tests.

   Adam Williams has done some work to create Rspec matchers for  
validations:

   http://tinyurl.com/6y4owv

   You might have a look at that and see what it would take to  
incorporate his code
   into ours.

   To add a custom matcher to Radiant, drop it in the spec/matchers  
directory.

I wasn't sure how to handle this, and I agree that it's painful the way I wrote the specs for regular expression matching for validates_format_of. Custom matchers have been on my radar, but not something I've looked at yet. Adam Williams' example was a no-brainer. I think the syntax of assert_invalid just threw me. I'll look more closely...

http://github.com/saturnflyer/radiant/commit/76df906ac37a55d29bb8cbd505ce5167366673f6
http://github.com/saturnflyer/radiant/commit/3b6521158b5534ea1e9dd2112d432973047115c8

   Nice work! These are much better.


I really appreciate your efforts in this area. Keep up the great work.

I'm going to try to handle very small parts and send pull requests as they are closer to completion, rather than tackling the entire app in one fell swoop.

If anyone is interested in doing the same, let me know what pieces you might tackle so that we don't waste effort.

--~--~---------~--~----~------------~-------~--~----~
Radiant CMS Dev Mailing List
Post:        radiantcms-dev@...
Unsubscribe: radiantcms-dev-unsubscribe@...
Group Site:  http://groups.google.com/group/radiantcms-dev/
-~----------~----~----~----~------~----~------~--~---

[Chris Parrish-6]

Jim Gay wrote:
> I've also come across places where the existing tests and models check  
> for 'numericality' of things like id and page_id. Why is this necessary?
> I wouldn't expect that a test for an auto-generated field like id  
> would need to be tested for its type. And fields like page_id are  
> never edited by a user so I wouldn't expect that to need it's type  
> tested either, so why bother even having that validation in the model?
>  

If you were just thinking of removing the spec but keeping the
validation, I'd think the spec needed to stay (why have code that isn't
required/specified).

But are you suggesting removing these validations from the model?  If
so, I'm curious what the development team thinks of removing this whole
specification -- and therefore the validation code that would ensure
these fields to be integers.

On a completely different note, I've also been collecting a couple of
spec improvements and I'm still not set up with git.  Would you mind if
I shot them your way for you to look through and maybe incorporate into
your changes?

-Chris

--~--~---------~--~----~------------~-------~--~----~
Radiant CMS Dev Mailing List
Post:        radiantcms-dev@...
Unsubscribe: radiantcms-dev-unsubscribe@...
Group Site:  http://groups.google.com/group/radiantcms-dev/
-~----------~----~----~----~------~----~------~--~---

[Jim Gay-2]

On Jul 5, 2008, at 2:14 PM, Chris Parrish wrote:

> Jim Gay wrote:
>> I've also come across places where the existing tests and models  
>> check
>> for 'numericality' of things like id and page_id. Why is this  
>> necessary?
>> I wouldn't expect that a test for an auto-generated field like id
>> would need to be tested for its type. And fields like page_id are
>> never edited by a user so I wouldn't expect that to need it's type
>> tested either, so why bother even having that validation in the  
>> model?
>
> If you were just thinking of removing the spec but keeping the
> validation, I'd think the spec needed to stay (why have code that  
> isn't
> required/specified).
>
> But are you suggesting removing these validations from the model?  If
> so, I'm curious what the development team thinks of removing this  
> whole
> specification -- and therefore the validation code that would ensure
> these fields to be integers.

I posing this as a question. I don't see a reason to validate this,  
but there may be something that I'm missing.
If the code to validate id and page_id is there, then the specs/tests  
should stay, but why is it there?

For now, I'm being conservative and I'm going to preserve as much as  
possible, but it doesn't make sense to me to check that 'id' is an  
integer. If 'id' isn't an integer, then the database was not setup  
properly. The 'id' field is created by the framework so it would  
appear to me that we are validating framework code rather than  
application code in this particular instance, and that type of  
validation doesn't belong in the application.

Validating 'page_id' is a bit of a different story because the app  
does create it. I haven't thought about this much, but my instinct was  
that it wasn't necessary. I think my assumption is that the page_id  
will come directly from some @page.id which should always be an  
integer and if page_id isn't being sent to the model as an integer  
then there's likely a problem with some method or controller action  
sending the wrong thing.

But I'm asking because I want to know why it is necessary, not because  
I want to use the question to state that it should not be there  
(although I guess the question alone, at least, implies my thinking).

> On a completely different note, I've also been collecting a couple of
> spec improvements and I'm still not set up with git.  Would you mind  
> if
> I shot them your way for you to look through and maybe incorporate  
> into
> your changes?

Sure.
I breathed a sigh of woe when I heard that Rails was moving to Git  
because it's yet another thing to spend my time learning, but with my  
brief use I can say that it is easily better than something like  
Subversion.

-Jim

--~--~---------~--~----~------------~-------~--~----~
Radiant CMS Dev Mailing List
Post:        radiantcms-dev@...
Unsubscribe: radiantcms-dev-unsubscribe@...
Group Site:  http://groups.google.com/group/radiantcms-dev/
-~----------~----~----~----~------~----~------~--~---

[john muhl-3]

On 2008/07/05, at 06:33, Jim Gay wrote:

> In an effort to hone my skill with RSpec, and to better understand
> Radiant internals, I've created a 'spec' branch in my repo
> http://github.com/saturnflyer/radiant/tree/spec

I pulled your branch into my fork and added the full SmartyPants specs  
that I had in my markdown branch. Speccing SmartyPants is nothing  
exciting, the thing I want to point out is the matcher I added so you  
can check dependencies from the specs, which I found particularly  
helpful for speccing extensions that require stuff (the matcher is  
from http://www.shiftcommathree.com/articles/testing-your- 
dependencies). Am I correct in thinking this is a useful thing to spec  
in the first place?

> Is this something that developers will find helpful?


I think it's definitely helpful and in a similar attempt to get more  
acquainted with RSpec and Radiant I'll start trying to contribute more  
to the effort.

http://github.com/johnmuhl/radiant/tree/spec

--~--~---------~--~----~------------~-------~--~----~
Radiant CMS Dev Mailing List
Post:        radiantcms-dev@...
Unsubscribe: radiantcms-dev-unsubscribe@...
Group Site:  http://groups.google.com/group/radiantcms-dev/
-~----------~----~----~----~------~----~------~--~---