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

Simplify testing with different versions on Rails #312

Merged
merged 1 commit into from Jun 28, 2013

Conversation

Projects
None yet
4 participants
@gylaz
Copy link
Contributor

gylaz commented Jun 21, 2013

  • remove Appraisal's gemfiles directory
  • don't specify gemfiles for Travis
  • use pessimistic operator in Appraisal
  • upgrade development gems, use pessimistic operator
@croaky

This comment has been minimized.

Copy link
Contributor

croaky commented Jun 21, 2013

Looks good.

@gylaz

This comment has been minimized.

Copy link
Contributor

gylaz commented Jun 21, 2013

I think, I understand the concern of @jferris points int #278 a little more now. And I think I'm leaning towards his arguments of checking in Gemfile.lock and all of the Appraisal gemfiles, plus locks.

To address some of the issues @croaky brought up:

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.

If *.gemfile.lock files are source controlled, then they shouldn't be getting updated often. Unless we want to explicitly update them. If they were getting updated often, then that seems like a different problem. I think this would only happen if developers are deleting Gemfile.lock and *.gemfile.lock files. We don't expect Gemfile.lock to get changed often on Rails apps, and this should be no different.

There seems to be a mixed amount of popular gems checking in their Gemfile.lock while others don't.

The pathnames in the appraised .lock files change every time a new developer runs the suite, polluting commits and code reviews.

This shouldn't be an issue any longer. Since we're using relative paths.

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.

I see this as a valid issue. However, we'd trade stability and consistency of the library, in development, over pollution of the history. It seems like bumping the rails patch version should be a conscious effort. With a little re-factoring we can make it so only the Appraisal's gemfiles get updated, when we choose to run rake appraisal:gemfiles to regenerate them.

Thoughts? /cc @croaky @jferris @derekprior

@derekprior

This comment has been minimized.

Copy link
Contributor

derekprior commented Jun 21, 2013

I don't think there's a perfect answer. What I said before still stands: I defer to those with more experience with library development. That said, here's how I see the pros and cons:

In the 'real world' developers are likely bundling and getting latest versions of stuff unless they have a specific reason not to. As gems that we depend on are updated we'll have to remember to manually update them with a bundle update for the appropriate appraisal files or we'll be out of date with what people are doing in production. On the other hand, this is less critical if we can count on semantic versioning of dependencies.

The ability to reproduce failing tests is certainly nice, but its not much use if what we're testing with isn't actually what our users are using. If someone is incented enough to clone the repo, update the appraisal gemfiles to be in line with what they were using when they hit a bug, and then submit a failing test case, aren't they equally capable of doing this without the appraisal lock files? For users that just want to report a bug, aren't we still going to want to see their Gemfile.lock?

When it comes to git noise, I guess I'm not overly sensitive to it. If lock files are deemed worth keeping and there are lots of checkins of the lock files in order to keep them up to date, so be it.

This shouldn't be an issue any longer. Since we're using relative paths.

This is lost if gemfiles are regenerated. We should see if we can add this to appraisal.

@derekprior

This comment has been minimized.

Copy link
Contributor

derekprior commented Jun 21, 2013

This might not actually work, but I just had an idea: What if we had one appraisal file without a checked-in lock file that was only run by Travis (or manually locally, if desired). Would this allow us to catch issues with newer versions of gems while maintaining a clean test suite for gems we know to be good?

@croaky

This comment has been minimized.

Copy link
Contributor

croaky commented Jun 21, 2013

@gylaz I'm fine with whatever direction you'd like to go. Your points seem valid and you can make the final call.

@gylaz

This comment has been minimized.

Copy link
Contributor

gylaz commented Jun 21, 2013

This is lost if gemfiles are regenerated. We should see if we can add this to appraisal.

I believe Appraisal uses relative pathing now. It's generating gemspec :path=>"../" for me, when i re-create the gemfiles.

What if we had one appraisal file without a checked-in lock file that was only run by Travis (or manually locally, if desired). Would this allow us to catch issues with newer versions of gems while maintaining a clean test suite for gems we know to be good?

I am not sure, how this would work. Can you provide an example?

Here's what might be a vialbe option: Check-in Gemfile.lock. Thus all development is done against the same versions. However, we rely on Travis to notify us of any failures against the latest gems, and update as errors are encountered. This is assuming that when appraisal:install runs, it fetches latest gems from index, and not from local available gems, which would be locked and installed through bundler and Gemfile.lock.

If the above option seems sane, then lets try it and see what pain points we encounter.

@derekprior

This comment has been minimized.

Copy link
Contributor

derekprior commented Jun 25, 2013

That's what I was trying to get at, @gylaz

@jferris

This comment has been minimized.

Copy link
Member

jferris commented Jun 27, 2013

In the 'real world' developers are likely bundling and getting latest versions of stuff unless they have a specific reason not to.

I don't think that's actually the case. I've only worked on a handful of projects where people regularly run bundle update to get the latest. Most projects are running several versions behind on almost all of their gems. This means that testing your library against the latest version of every gem is actually less realistic than testing against a pinned version. Try running bundle outdated on your projects to see.

Simplify testing with different versions on Rails
* remove Appraisal's gemfiles directory
* don't specify gemfiles for Travis
* use pessimistic operator in Appraisal
* upgrade development gems, use pessimistic operator
@gylaz

This comment has been minimized.

Copy link
Contributor

gylaz commented Jun 28, 2013

This is ready for re-review. If no objections, I'd like to give this setup a try. /cc @jferris @derekprior @croaky

@croaky

This comment has been minimized.

Copy link
Contributor

croaky commented Jun 28, 2013

Looks good to me.

@jferris

This comment has been minimized.

Copy link
Member

jferris commented Jun 28, 2013

Does this mean we won't be testing multiple Rails versions on Travis, or do I not get what's changing?

@croaky

This comment has been minimized.

Copy link
Contributor

croaky commented Jun 28, 2013

@jferris It still tests against multiple Rails versions. See https://travis-ci.org/thoughtbot/clearance/jobs/8549928 as an example. We just don't need to check the generated gemfiles into version control.

@jferris

This comment has been minimized.

Copy link
Member

jferris commented Jun 28, 2013

Ah, I just found the changes in Rakefile. I'm interested to see how this goes. No objections here.

@gylaz gylaz merged commit e075bf0 into master Jun 28, 2013

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