Testing #182

Merged
merged 8 commits into from Feb 14, 2014

Conversation

Projects
None yet
2 participants
Contributor

hcarreras commented Feb 13, 2014

Added some user controller specs

@PragTob PragTob commented on an outdated diff Feb 14, 2014

spec/controllers/user_controller_spec.rb
@@ -0,0 +1,53 @@
+require 'spec_helper'
+
+describe UsersController do
+ let(:bob){Fabricate(:user)}
+ let(:mozart){Fabricate(:user)}
+
+ describe('#index') do
+ context "When user is no moderator" do
@PragTob

PragTob Feb 14, 2014

Member

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 :-)

Contributor

hcarreras commented Feb 14, 2014

Indentation fixed! :)

@PragTob PragTob commented on an outdated diff Feb 14, 2014

spec/controllers/user_controller_spec.rb
@@ -0,0 +1,53 @@
+require 'spec_helper'
+
+describe UsersController do
+ let(:bob){Fabricate(:user)}
+ let(:mozart){Fabricate(:user)}
+
+ describe('#index') do
@PragTob

PragTob Feb 14, 2014

Member

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.

Member

PragTob commented Feb 14, 2014

Thanks! Left you another small remark! =)

@PragTob PragTob and 1 other commented on an outdated diff Feb 14, 2014

spec/controllers/user_controller_spec.rb
@@ -0,0 +1,53 @@
+require 'spec_helper'
+
+describe UsersController do
+ let(:bob){Fabricate(:user)}
+ let(:mozart){Fabricate(:user)}
+
+ describe('#index') do
+ context "When user is no moderator" do
+ it "try to get index" do
+ get :index
+ response.should redirect_to(root_path)
@PragTob

PragTob Feb 14, 2014

Member

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 :)

@hcarreras

hcarreras Feb 14, 2014

Contributor

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

@PragTob

PragTob Feb 14, 2014

Member

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

Contributor

hcarreras commented Feb 14, 2014

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?

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 PragTob added a commit that referenced this pull request Feb 14, 2014

@PragTob PragTob Merge pull request #182 from hcarreras/testing
Testing
344b4e6

@PragTob PragTob merged commit 344b4e6 into hacketyhack:master Feb 14, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment