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

Improve testing of different Ruby, Rails versions #278

Closed
wants to merge 1 commit into from

Conversation

croaky
Copy link
Contributor

@croaky croaky commented Mar 18, 2013

  • Remove Gemfile.lock file from version control.
  • Remove gemfiles from version control. Let Appraisal generate them.
  • Use Semantic Versioning (http://semver.org/) in Appraisal.

This intends to solve the following problems:

  • The Gemfile.lock and gemfiles/*.gemfile.lock files were being
    updated regularly during normally development work, polluting commits
    and code reviews with irrelevant changes like tzinfo changing from
    0.3.35 to 0.3.37.
  • The pathnames in the appraised .lock files change every time a new
    developer runs the suite, polluting commits and code reviews.
  • We needed to manually update patch versions in Appraisal to test
    against the latest versions, polluting the git history, instead of
    just triggering a new Travis build. This is the most important
    combination of dependencies and therefore should be quick and easy to
    test.

http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/

Also:

  • Clarify versions of Ruby/Rails supported in README

  • Loosen development dependency versions to 2-digit SemVer.

  • Fix config.paths for Rails 4.

  • Fix controller specs on Rails 4.

  • Don't fork the Cucumber suite (leftover from Guard/Spork days?).

  • Fix Cucumber::Rails::Database error:

    uninitialized constant Cucumber::Rails::Database (NameError)

  • Use Capybara 2.0 spec/features convention.

  • Add RSpec as an explicit dependency to avoid this errro:

    require': cannot load such file -- rspec (LoadError)

thoughtbot/shoulda-matchers#262

@jferris
Copy link
Contributor

jferris commented Mar 18, 2013

I'm still opposed to the "use whatever dependencies are available" approach to development.

I wrote about this when blogging about appraisal: http://robots.thoughtbot.com/post/12068553267/appraisal-find-out-what-your-gems-are-worth

The two primary issues:

  • Without a Gemfile.lock, the likelihood that a developer pulls and gets a broken setup increases greatly, which in turn greatly decreases the likelihood that developers will participate in a project.
  • Without lockfiles for appraisals, the situations aren't actually reproducible, which means regressions can reappear and a bug that occurs on one machine will be invisible on another.

s.add_development_dependency 'sqlite3', '1.3.6'
s.add_development_dependency 'timecop', '0.3.5'
s.add_development_dependency 'appraisal'
s.add_development_dependency 'aruba'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not care what each of these gem's version requirements are anymore? Could I use this against shoulda-matchers 1.0 (instead of 1.2.0) and still have a passing suite? What about factory_girl_rails 1.7.0? As far as I can tell, both of these have no dependencies on other dev dependencies and could therefore pass at a significantly lower gem version; is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshuaclayton I think we care that they're relatively up-to-date. I just pushed fcc6785 to peg them with a pessimistic operator against two digits of precision. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@croaky
Copy link
Contributor Author

croaky commented Mar 18, 2013

@jferris What if we added a bundle update to the Rakefile? Idea there that
we create a reproducible situation of using the latest available gems that
Bundler can reconcile together, testing against the latest. If regressions
occur in this scenario, they would be regressions we want to know about.

On Monday, March 18, 2013, Joe Ferris wrote:

I'm still opposed to the "use whatever dependencies are available"
approach to development.

I wrote about this when blogging about appraisal:
http://robots.thoughtbot.com/post/12068553267/appraisal-find-out-what-your-gems-are-worth

The two primary issues:

  • Without a Gemfile.lock, the likelihood that a developer pulls and
    gets a broken setup increases greatly, which in turn greatly decreases the
    likelihood that developers will participate in a project.
  • Without lockfiles for appraisals, the situations aren't actually
    reproducible, which means regressions can reappear and a bug that occurs on
    one machine will be invisible on another.


Reply to this email directly or view it on GitHubhttps://github.com//pull/278#issuecomment-15054035
.

@@ -1,11 +1,9 @@
if RUBY_VERSION >= '2.0'
rails_versions = ['3.2.13.rc2']
name, version = 'ruby.2.support', '>= 3.2.13.rc2'
Copy link
Contributor

Choose a reason for hiding this comment

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

We can make this '>= 3.2.13' now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done in 41ba797.

* Remove `Gemfile.lock` file from version control.
* Remove `gemfiles` from version control. Let Appraisal generate them.
* Use Semantic Versioning (http://semver.org/) in `Appraisal`.

This intends to solve the following problems:

* The `Gemfile.lock` and `gemfiles/*.gemfile.lock` files were being
  updated regularly during normally development work, polluting commits
  and code reviews with irrelevant changes like `tzinfo` changing from
  0.3.35 to 0.3.37.
* The pathnames in the appraised `.lock` files change every time a new
  developer runs the suite, polluting commits and code reviews.
* We needed to manually update patch versions in `Appraisal` to test
  against the latest versions, polluting the git history, instead of
  just triggering a new Travis build. This is the most important
  combination of dependencies and therefore should be quick and easy to
  test.

http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/

Also:

* Clarify versions of Ruby/Rails supported in README
* Loosen development dependency versions to 2-digit SemVer.
* Fix config.paths for Rails 4.
* Fix controller specs on Rails 4.
* Don't fork the Cucumber suite (leftover from Guard/Spork days?).
* Fix `Cucumber::Rails::Database` error:

    uninitialized constant Cucumber::Rails::Database (NameError)

* Use Capybara 2.0 spec/features convention.
* Add RSpec as an explicit dependency to avoid this errro:

    require': cannot load such file -- rspec (LoadError)

thoughtbot/shoulda-matchers#262
@croaky
Copy link
Contributor Author

croaky commented Mar 20, 2013

Ready for re-review.

@derekprior
Copy link
Contributor

Looks good to me. I have fixes for the shoulda-matcher deprecation warnings. Do you want me to push them here or wait until this lands?

I understand @jferris' concerns, but on the developer confusion/frustration point I'd counter that developers would be initially confused by the lock files being present/modified because tracking them runs counter to common advice. That said, I don't have nearly as much experience with library development to weigh in heavily on either side.

else
rails_versions = ['3.0.20', '3.1.11', '3.2.12']
name, version = 'rails.3.support', '~> 3.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Rails breaks SemVer enough that I could envision this being a problem. Don't we need to explicitly test 3.0.x, 3.1.x, and 3.2.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen libraries like client_side_validations only support only minor
revision in the past (Rails 3.1 while Rails 3.2 is the current stable).
Maybe we go '~> 3.1'?

On Friday, March 22, 2013, Derek Prior wrote:

In Appraisals:

else

  • rails_versions = ['3.0.20', '3.1.11', '3.2.12']
  • name, version = 'rails.3.support', '~> 3.0'

Rails breaks SemVer enough that I could envision this being a problem.
Don't we need to explicitly test 3.0.x, 3.1.x, and 3.2.x?


Reply to this email directly or view it on GitHubhttps://github.com//pull/278/files#r3490197
.

Copy link
Contributor

Choose a reason for hiding this comment

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

My problem is that bundler is always going to choose 3.2.latest, so while we can say we support 3.1 (or even 3.0), we won't be actually testing it. Appraisal is meant to address this very issue, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @derekprior that we probably want to test against the minor versions of Rails 3.x (i.e. 3.0.x, 3.1.x, 3.2.x), or just explicitly decide that we are supporting only the latest 3.x release. As this PR stands, Travis will only run against the latest 3.2.x.

If we want to support the minor versions of Rails 3, then we should specify each of the three versions in Appraisal file. Would be best if we can use the pessimistic operator for each minor versions. @jferris is that possible in Appraisal?

@derekprior
Copy link
Contributor

Found an issue when I was working on top of this branch to try out Rails 4 and verified it was introduced with this branch. I can't successfully run bundle exec rake appraisal:rails.3.support. Running a full bundle exec rake works as expected, but running a specific appraisal yields:

cannot load such file -- cucumber/rake/task
/Users/derek/src/clearance/Rakefile:8:in `require'

I'm perplexed.

.bundle
Gemfile.lock
bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we add bin/ to this? For binstubs? Others could not be using binstubs, or install them to another directory. This should go into a global gitignore on per user basis.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gylaz it looks like it was just reordered alphabetically. We'll want to reconsider this when Rails 4 drops, since bin/rails, bin/rake, and bin/bundle will need to be included in the repo for it to be deployed and running on Heroku (as of right now).

@gylaz
Copy link
Contributor

gylaz commented Jun 19, 2013

I think this PR has too much stuff in it. It's not just removing .lock files, but also cleaning up some things. This should probably be split up into two.

@gylaz
Copy link
Contributor

gylaz commented Jun 21, 2013

Going to close this. Opened #312 for just the Appraisal related changes.

@gylaz gylaz closed this Jun 21, 2013
tjhosford added a commit to crowdtap/clearance that referenced this pull request Jul 19, 2013
@derekprior derekprior deleted the dc-remove-lock-files branch April 18, 2014 14:43
tjhosford added a commit to crowdtap/clearance that referenced this pull request May 28, 2015
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.

6 participants