Skip to content

Testing#182

Merged
PragTob merged 8 commits intohacketyhack:masterfrom
hcarreras:testing
Feb 14, 2014
Merged

Testing#182
PragTob merged 8 commits intohacketyhack:masterfrom
hcarreras:testing

Conversation

@hcarreras
Copy link
Copy Markdown
Contributor

Added some user controller specs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the indention here is a bit wild - looks like 8 spaces. Or do you use tabs?

It's recommended and encouraged to use two spaces white space (non tab) indention in Ruby. Whitespaces are preferred over tabs because when someone has different tab settings code just starts to look weird, especially if whitespaces and tabs are mixed.

Would be glad if you could fix that up :-)

@hcarreras
Copy link
Copy Markdown
Contributor Author

Indentation fixed! :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using describe&it with parentheses is highly unusual - it's better style to leave the parentheses out here - I think I've never seen it with them... e.g. describe '#index' do etc.

@PragTob
Copy link
Copy Markdown
Member

PragTob commented Feb 14, 2014

Thanks! Left you another small remark! =)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency and update reasons you could migrate this to the expect syntax as well, e.g. expect(response).to redirect_to(root_path) should work :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it is better to use expect syntax?
Thanks for the feedback!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well expect is the new default for RSpec 3.0+ (and maybe even 2.14). So it is generally encouraged, the old syntax will still be maintained but expect is generally the way to go (among other things with expect you don't need to have monkey patching). Also it helps in some edge cases, see this: http://myronmars.to/n/dev-blog/2012/06/rspecs-new-expectation-syntax

@hcarreras
Copy link
Copy Markdown
Contributor Author

Fixed!
Thank you very much for the feedback! I really appreciate it!

Repeating my question: when it is recommended "expect" syntax instead of "should" syntax?

@PragTob
Copy link
Copy Markdown
Member

PragTob commented Feb 14, 2014

Thanks for your work on testing! =) And glad to give feedback, least I can do :-)

Little answer on expect vs. should in the comment. Some thoughts about expect vs. should from another project: shoes/shoes4#479

PragTob added a commit that referenced this pull request Feb 14, 2014
@PragTob PragTob merged commit 344b4e6 into hacketyhack:master Feb 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants