New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perceived enhancements to the sign up feature specs #25

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@dbernar1
Copy link
Contributor

dbernar1 commented Feb 8, 2014

Hello,

I've made these changes to the sign up spec, which I perceive as enhancements. I am mainly measuring the enhancements through use of:

rspec --format documentation --order defined

Originally, that looked something like:

Sign up
  When signing up
    Should flash success and log in

After the changes I did, it looks like this:

Sign up
  The sign up page
    has a form with email address, password, and password confirmation fields
    has a link to the sign in page, for people who already have an account
  When the user signs up successfully
    they end up on the home page of the site
    they get signed in, so a sign out link is present
    a confirmation notice is shown
  Failure cases
    When required fields are not filled in
      Missing email address
        the user is notified that the email address is required

Please let me know what you think!

@dbernar1

This comment has been minimized.

Copy link
Contributor

dbernar1 commented Feb 8, 2014

And props to @burke for showing us rspec --format documentation at his recent presentation.

end

context "When the user signs up successfully" do
def fill_out_signup_form_and_submit

This comment has been minimized.

@swalberg

swalberg Feb 8, 2014

Contributor

If you're up for it, this could be extracted to a capybara helper. See http://www.elabs.se/blog/51-simple-tricks-to-clean-up-your-capybara-tests and http://robots.thoughtbot.com/automatically-wait-for-ajax-with-capybara for some details. That would let any capybara spec use this step.

This comment has been minimized.

@dbernar1

dbernar1 Feb 8, 2014

Contributor

Thank you very much for the tip, Sean!

At this time, I am not inclined to apply that refactoring. As a rule of thumb, I do not extract functions into separate files until 2 files need it. This way, if I need to reference what it does or modify it I do not have to open a separate file, and others don't have to "fish for it" in the code base.

@winnipegrb committers: if you prefer it as a capybara helper, just let me know I should adjust it before you can accept the pull request, or just adjust it yourself after merging.

Thanks again for the tip, though. I am sure it will come in handy in the near future.

Dan Bernardic
Learn more about me at http://gravatar.com/dbernar1

On Feb 8, 2014, at 1:55 PM, Sean Walberg notifications@github.com wrote:

In spec/features/sign_up_spec.rb:

  •  # fill out form and submit
    
  • it "has a form with email address, password, and password confirmation fields" do
  •  [ 'email', 'password', 'password_confirmation' ].each do |field_name|
    
  •    expect(page).to have_selector "input#user_" + field_name
    
  •  end
    
  • end


Reply to this email directly or view it on GitHub.

@swalberg

This comment has been minimized.

Copy link
Contributor

swalberg commented Feb 8, 2014

I like the general idea of what you did here, there's definitely an improvement in the tests.

Two things come to mind:

  • While the output of rspec is better, some of the code doesn't read well anymore. e.g. it "they end up on the home page of the site" do
  • Rather than visit somehelper and checking for the existence of link with href of helper, shouldn't the test focus more on what the user sees? That is, instead of going to the registration page by specifying the helper, what about:
visit '/'
click_link 'Sign up'

If you go down the path of extracting your other code to a capybara helper, you could do the same for some of these:

# somewhere under spec/support
def visit_sign_in_page
  visit '/'
  click_link 'Sign in'
end

# in our test
before do
  visit_sign_in_page
end
@dbernar1

This comment has been minimized.

Copy link
Contributor

dbernar1 commented Feb 8, 2014

Thanks again for the collab.

I agree that it reads poorer. It seems to me that that is rspec's fault, not mine. If i didnt have to put "it" there, it would read great, it seems. :)

As for the "facility to get to the sign up page from some other page", it seems to me that that would belong in the feature spec of that page

I will name the process of visiting the sign up page as you suggested, thanks!

Dan Bernardic
Learn more about me at http://gravatar.com/dbernar1

On Feb 8, 2014, at 3:07 PM, Sean Walberg notifications@github.com wrote:

I like the general idea of what you did here, there's definitely an improvement in the tests.

Two things come to mind:

While the output of rspec is better, some of the code doesn't read well anymore. e.g. it "they end up on the home page of the site" do
Rather than visit somehelper and checking for the existence of link with href of helper, shouldn't the test focus more on what the user sees? That is, instead of going to the registration page by specifying the helper, what about:
visit '/'
click_link 'Sign up'
If you go down the path of extracting your other code to a capybara helper, you could do the same for some of these:

somewhere under spec/support

def visit_sign_in_page
visit '/'
click_link 'Sign in'
end

in our test

before do
visit_sign_in_page
end

Reply to this email directly or view it on GitHub.

dbernar1 added some commits Feb 9, 2014

Pulled out a signup test helper. Extracted the specifics of how we co…
…nfirm that a set of possible signup failure cases is handled into a helper.
@dbernar1

This comment has been minimized.

Copy link
Contributor

dbernar1 commented Feb 9, 2014

I would like to invite review and feedback of the signup feature spec from the people who have contributed code to Winni Vote so far: @floridaelago @stungeye @amirci @brett-anderson @gmcgibbon @swalberg @thetamind @celsodantas @jeffreyfultonca. When/if you have the time, please take a look.

This is now at a point where I'm happy with it. The only thing I can think of as a further enhancement would be to wrap the signup helpers in spec/features/support/signup.rb in a module, so that they are not in the global namespace, but I could not be bothered figuring out how to make that work, and it does not work when I simply wrap them in a module. Please let me know if you know how to achieve that, or any other feedback to make the specs better.

You are the people who I know are working on this project - please invite other group members if you feel they would benefit from looking at the example of how this feature is spec'ed out, or would have feedback.

Thank you for your time.

@swalberg

This comment has been minimized.

Copy link
Contributor

swalberg commented Feb 9, 2014

Dan, you need to add something like this to the spec_helper.rb: https://github.com/thoughtbot/suspenders/blob/master/templates/spec_helper.rb#L11

end

describe "Failure cases" do
confirm_these_possible_signup_failure_cases_are_handled [

This comment has been minimized.

@swalberg

swalberg Feb 9, 2014

Contributor

Dan, I think what you want here is an Rspec custom matcher

That said, we're at a spot where this is an improvement over what we had before and we should merge this and improve upon it later, maybe as an exercise at the next meetup. You are touching on a bunch of things that rspec/capybara can do to make testing much nicer and it would be good if more people could see.

@swalberg

This comment has been minimized.

Copy link
Contributor

swalberg commented Feb 9, 2014

Comments inline. Have a look at the spec_helper.rb thing and squash to one commit, please.

@dbernar1

This comment has been minimized.

Copy link
Contributor

dbernar1 commented Feb 9, 2014

Good morning,

spec_helper.rb: we have the same thing on the same file and line already. Am I misunderstanding something there?

I've reviewed the list of commits, and I quite like the fact that most of the commits I made are separate. I've committed only when a logical step of the work I did was complete, except that failure cases are across e4716cf and 3d7950b. In other words, it seems that to squash the commits into one would remove insight into the steps I took to arrive at the current version. I think that insight can be valuable to others - what do you think?

Thanks again for your time.

@swalberg

This comment has been minimized.

Copy link
Contributor

swalberg commented Feb 9, 2014

Ah, ok, I must have misinterpreted what you said earlier on the spec_helper.rb file. Nevermind, then. When you wrapped it in a module, you probably didn't include that module explicitly.

A separate list of commits is great during code review as it helps us understand the process that got you to the finished product. But when it's in master that information isn't as useful -- it can even be noisy. Just like the separate commits in the feature branch show your thought process, the larger commits in master should show how the product was built.

As a more practical example of why squashing is encouraged, sometimes a feature makes it to master and is found to be buggy in production. It's the feature that's buggy, not a particular commit from that feature. We want to be able to understand the feature as a whole and possibly git revert it until we can bring it back into master. If it were broken out into separate commits we'd have to revert all of them.

@dbernar1

This comment has been minimized.

Copy link
Contributor

dbernar1 commented Feb 9, 2014

Thanks!

I'd never even heard of "squashing" before. This will be very useful to me 🐱, thanks for explaining it. I've been battling reviewing the whole diff of a branch merge after someone at work finishes work in the branch. I think this is exactly what this will allow me to do.

I've created a new pull request: #28 , which I think is properly set up for merging this in.

@dbernar1 dbernar1 closed this Feb 9, 2014

@dbernar1 dbernar1 referenced this pull request Feb 9, 2014

Closed

Fix tests to be more clear #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment