Skip to content
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

Venue testing #233

Merged
merged 11 commits into from Nov 14, 2014

Conversation

Projects
None yet
3 participants
@nblackburn87
Copy link
Contributor

commented Nov 9, 2014

Testing venue creation and editing following the style of Events.

This is still a work in progress. Chat is still welcome. :)

check('venue_closed')
check('venue_force_geocoding')

click_on 'Update Venue'

This comment has been minimized.

Copy link
@jc00ke

jc00ke Nov 9, 2014

Contributor

Any value in checking the updates succeeded?

This comment has been minimized.

Copy link
@nblackburn87

nblackburn87 Nov 11, 2014

Author Contributor

This was the last bit I worked on at the code sprint. I intend on adding the page.should have_value stuff soon.

This comment has been minimized.

Copy link
@jc00ke

jc00ke Nov 11, 2014

Contributor

Cool. Do you want to add to this PR or open a new one with those additions? Either way works for me.

This comment has been minimized.

Copy link
@nblackburn87

nblackburn87 Nov 11, 2014

Author Contributor

I'll just add to this PR when it's done. It shouldn't take too long.

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2014

Okay. So I've added the expectations for venue editing. There are a few boxes in both creation and editing that I've commented out because I'm either unsure about how to test them or whether they should even exist. Mainly, that's Venue tags like #230, and things like Lat/Long and the Geocoding checkbox.

In addition, I've used the new expect(page).to have_content syntax since should is deprecated. Thoughts?

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2014

Hmm, that spec fails. Might have something to do with This venue is no longer open for business?

Also, we should probably use a factory here as a test harness. Want to give that a go?

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2014

I got a bit ahead of myself. I'll get this cleaned up and comment again. The closed check box is likely the hang point.

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2014

Well, it looks like the address editing is the actual hang point.

fill_in 'Full Address', with: '350 5th Ave, New York, NY 10118, US' works for creating a new venue but its equivalent when editing results in the nonsense address created by let(:venue) { create(:venue) } remaining. All other fill_ins are working.

Also, I'm still a new Rails developer. Could you clarify what you mean by using a factory as a test harness? @jc00ke

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2014

Alright. So address editing is solved for the moment, though I think there are some systemic changes that could be made to the Venue Edit form to make it cleaner.

I'm still unsure of how to use a factory as a test harness. Input welcome.

@@ -0,0 +1,43 @@
require 'rails_helper'
require 'pry'

This comment has been minimized.

Copy link
@jc00ke

jc00ke Nov 12, 2014

Contributor

Need to remove this require.

This comment has been minimized.

Copy link
@nblackburn87

nblackburn87 Nov 12, 2014

Author Contributor

Thanks. I pulled the gems back out but forgot the require.

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2014

Cool, looks like the specs pass!

As for using the factories, we can use the venue factory to build out test data.

feature 'Venue Editing' do
  let(:venue) { create(:venue) }
  let(:new_venue) { build(:venue) } # <-- building out a test harness

  scenario 'A user edits an existing venue' do
    visit "/venues/#{venue.id}"
    click_on 'edit'

    find_field('Venue Name').value.should have_content "#{venue.title}"
    fill_in 'Venue Name', with: new_venue.name
    # ...
    check('venue_wifi') if new_venue.wifi
    # ...
    check('venue_closed') if new_venue.closed

    click_on 'Update Venue'

    expect(page).to have_content 'Venue was successfully saved.'
    expect(page).to have_content new_venue.name
    # ...
    expect(page).to have_content 'Public WiFi'
    expect(page).to have_content 'This venue is no longer open for business.'
  end
end
feature 'Venue Creation' do
scenario 'User adds a new venue' do

visit '/venues'

This comment has been minimized.

Copy link
@jc00ke

jc00ke Nov 12, 2014

Contributor

We should use venue_path here.


scenario 'A user edits an existing venue' do

visit "/venues/#{venue.id}"

This comment has been minimized.

Copy link
@jc00ke

jc00ke Nov 12, 2014

Contributor

We should use venue_path(venue) here.

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2014

@nblackburn87 this PR is taking shape!

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2014

@jc00ke I see what we're doing here. The code takes the form it does right now because I was following the format of add_event_spec.rb and managing_event_spec.rb. I know a couple of contributors put a good amount of time into those. Do you think we should go back and build in the factory functionality for that too, or follow the original standards?

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2014

Yes, I do think we should rewrite those specs, but let's wrap up this PR first, then tackle refactoring the others.

Hard-coding those values ends up being a pain later, when one needs to change the specs. That's the main reason I'm advocating we switch. Not a huge deal though.

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2014

Cool. I can definitely do it, I just wanted to check and make sure that was the plan. I see the value of this over hardcoded responses for sure.

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2014

Well, the specs still pass with the harness, which is good. The relative pathing is causing problems though. I've been off in Python land and so it's been a bit since I touched this much Rails and the relative routes are coming back slowly. The larger issue is that Calagator's routes.rb file is more complicated than anything I've ever built. I ran rake routes and got this that seems to be the relevant part:

venue GET /venues/:id(.:format) venues#show, PUT /venues/:id(.:format) venues#update, DELETE /venues/:id(.:format) venues#destroy

The venues#show here should be venue_path(venue) like you indicated. But Capybara can't seem to find the edit button on the page when I specify the path like that.

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2014

Hmm, it might be that the url helpers aren't available in the request features. See this SO answer for more details. We can handle that in a separate PR.

So, to wrap up this PR, push the use of factories and then we'll go from there. Ideally we'd squash these commits into a single commit, but it might be easier to open up a new PR with a clean commit. Your call.

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2014

It looks like that's exactly what's up. I tried a few different searches but I guess I didn't ask the right thing. Pushing the factories.

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2014

@jc00ke Hmm. Travis errors on Ruby 2.0.0 with Postgres. Everything else is green. Travis docs seem to suggest that this is fine.

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2014

Yeah, it's a shame the build errors when solr can't start up. I'll open a separate issue for that.

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2014

Cool, this looks good! Squash into one commit and I'll merge!

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2014

Use rebase -i to squash? Also, what's the advantage of squashing?

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2014

In cases like this, where ideally this would have been a single commit AND there were build failures and errors, I like to squash to the latest commit or open a new PR. It keeps the project history cleaner.

@shawnacscott

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2014

I'm wondering what the value of squashing is here. I understand it's nice to keep things tidy, commit history-wise, but I also would like to balance this with not rewriting history when we don't need to.

I also wonder about asking one contributor to squash when it seems to not be asked of others. Especially since one of the core tenets of this project is to help newcomers to open source, and those little green boxes can be really motivating.

Maybe we need a clear contribution guideline on this documented somewhere, so contributors know what to expect from the beginning?

@reidab, @aeschright, @jc00ke thoughts?

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2014

In this case I would squash these commits 1) to keep the history clean, as you mentioned and 2) to avoid committing into master code that is known to fail. We aren't rewriting history yet... if we rebased master we would be, but this PR is a malleable chunk, so it's OK to clean up when appropriate. We ask contributors to clean up commits on Rubinius from time to time, when appropriate.

I'd have to see specific examples, but most PR's don't need to be squashed. Incrementally developing or refactoring a feature doesn't necessarily warrant squashing. That being said, I do suppose the merge commit gives us a single point to revert, should we want/need to in the future (not with this PR, just in general)

Guidelines would be nice. I wrote some up for Rubinius a while back and they've helped encourage new contributors to contribute in a productive way. I'm not sure how exactly we would address squashing, since it comes up rarely, but maybe we should.

My goal in giving feedback was to help make the final product, the code that actually gets merged back into master, as good as it can be. Beyond squashing, I've mentioned several times that a new PR could be opened. I've seen that tactic used on several other notable projects, and it's one I've used myself. For me, a clean, understandable history is important. It communicates intent in its own way.

I'd love to hear the thoughts of others.

@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2014

Please understand. I'm not suggesting we do anything that might harm the project and I'm open to squashing some or all the commits if that's what really should be done. Perhaps just the failed builds? I think that it's very possible to squash some things while still giving some sense of the steps I took and how this PR progressed.

One of my goals at the moment as a very new developer is not just to work on projects (though that's valuable) but also to build a history on open source projects and elsewhere. With this in mind I'd prefer to not have this end up as just one commit on my personal history. Perhaps that's not the most noble goal but it's where I am.

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2014

OK, I see where you're coming from now... sorry it took me so long! I didn't think of this being an addition to your personal history; an oversight on my part. That's something I'll have to look for in the future.

jc00ke added a commit that referenced this pull request Nov 14, 2014

@jc00ke jc00ke merged commit 8073ffe into calagator:master Nov 14, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details
@nblackburn87

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2014

Oh wow. I was feeling very anxious about this and I'm glad you understood my point of view. Thanks so much @jc00ke

@jc00ke

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2014

Thank you @nblackburn87! I certainly never meant for you to feel anxious. I'm glad we got this sorted out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.