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

Multiple-gemfile testing and travis configuration #240

Merged
merged 2 commits into from
Mar 4, 2013

Conversation

urbanautomaton
Copy link

Per the discussion in #237 and #238, this implements multi-gemfile testing locally and on travis. The test suite now runs against the latest versions of:

  • Rails 3.0, 3.1 and 3.2
  • Capybara 1.1 and 2.0

I haven't been able to get the tests running against Rails 4.0 yet because mongoid doesn't support it (and its gemspec blocks installation). The Ruby 2.0 tests only run against Rails 3.2, to save on travis cycles.

I've removed from the gemspec anything that was there solely to provide dependencies for auto-generated apps. This is so Appraisal can omit anything that other rails versions don't need (or clash with, e.g. coffee-rails and rails 3.0). These dependencies now live in the main Gemfile above the gemspec declaration.

Apart from adding a Rails ~> 3.0 specification in the gemspec, I haven't touched any gem versions; the main Gemfile.lock is excluded from the repo anyway, so contributors will automatically get the most recent matching gems at the time of install. The lock files for the appraisal gemfiles are committed, however, so we know the build is being run against a consistent set of gems.

The default test task still runs against Rails 3.2 only. For contributors, the default workflow is unchanged:

$ git clone https://github.com/cucumber/cucumber-rails.git
$ bundle install
$ rake

I've added rake tasks for running the different gemfile tests:

$ rake gemfiles:install        # roughly "bundle install" for gemfiles/*.gemfile
$ rake test:all                # run test suite against gemfiles/*.gemfile
$ rake test:gemfile[rails_3_0] # run test suite against named gemfile

I think that about covers it - now to see what travis makes of it all.

@os97673
Copy link
Member

os97673 commented Mar 1, 2013

It looks like some builds failed :( https://travis-ci.org/cucumber/cucumber-rails
For some reasons github doesn't show this in the PR (very inconvenient).

@urbanautomaton
Copy link
Author

That's an outdated build - I cut down the build matrix a bit but forgot to remove the related gemfiles, so both of those failing jobs are ones that should no longer exist. But when I squashed and force-pushed the branch, travis didn't pick up the latest changes for some reason. I'll make a minor change and try to re-trigger it.

Edit: yep, there we go, green now.

The test suite now runs (and passes!) against the latest versions of:

* Rails 3.0, 3.1 and 3.2
* Capybara 1.1 and 2.0

The default test task is unchanged, and runs only against Rails 3.2. See
the README for details of running all (or selected) tests.

Addresses issues cucumber#237 and cucumber#238.
@os97673
Copy link
Member

os97673 commented Mar 1, 2013

The build is fine now.
One more question: do you think we should add .lock files under source control?

@urbanautomaton
Copy link
Author

The main one, no (and I've left it gitignored, as before).

The appraisal ones, I think we do. That means we can have a locked-down "official" build against a consistent set of gems, while not overspecifying things in the gemspec. Then the appraisal gemfiles can be periodically rebuilt to test against updated dependencies.

It's a tradeoff, though; this way the build is consistent and won't randomly break if a dependency changes, so it's easier to track regressions. On the other hand, you don't get as quick feedback that something's wrong if a dependency changes, and someone has to remember to manually update the build every so often.

@os97673
Copy link
Member

os97673 commented Mar 1, 2013

I'd vote for "build with all new gems" strategy as we do now.
@aslakhellesoy @mattwynne what do you think about this?

@mattwynne
Copy link
Member

On 1 Mar 2013, at 11:35, Oleg Sukhodolsky [email protected] wrote:

One more question: do you think we should add .lock files under source control?

What would be the reason for doing that? I've always understood it's a bad idea to do that for gems, because you could end up building / testing against older versions of dependencies than your users will have when they install.

@urbanautomaton
Copy link
Author

What would be the reason for doing that?

I left the appraisal-generated lock files in there so the travis build would be stable. Devs running the tests locally would run them against the latest versions, as Gemfile.lock is still gitignored. I'm happy to exclude the generated lock files too though if that's the consensus.

Edit: by way of comparison I note that paperclip does not commit its appraisal-generated locks.

@mattwynne
Copy link
Member

On 1 Mar 2013, at 14:18, Simon Coffey [email protected] wrote:

What would be the reason for doing that?

I left the appraisal-generated lock files in there so the travis build would be stable. Devs running the tests locally would run them against the latest versions, as Gemfile.lock is still gitignored. I'm happy to exclude the generated lock files too though if that's the consensus.

I think that makes sense.

@os97673
Copy link
Member

os97673 commented Mar 1, 2013

@urbanautomaton could you please clarify one more thing for me:
when a developer will need to add one more gem (as development dependency) in which file she should add it?
Before the change we place all dependencies to .gemspec, but now some of them moved to Gemfile and it is unclear (at least for me) how to decide where every dependency should be declared :(

@mattwynne
Copy link
Member

Would this get simpler if we said we wouldn't (make any guarantee to) support anything other than the most recent minor release? Then we could manage these different Gemfiles in branches, because we'll have a different branch of the repo for Rails 2, Rails 3 and Rails 4 support.

Would that make things easier for us? Would it be unreasonable for our users?

@urbanautomaton
Copy link
Author

Dammit, I just tried to edit my comment, tried to cancel my edit, and bloody deleted it. Sorry, chaps. To summarise:

  1. Direct development dependencies (cucumber, rspec, ammeter, appraisal) go in the gemspec, loosely specified.
  2. Dependencies of generated rails apps go as appropriate in the main Gemfile, and the respective appraisal sections. So coffee-rails is a dependency of rails 3.1 and 3.2 apps, so goes in the main Gemfile (which is 3.2), and the 3.1 and 3.2 appraisal sections.

@urbanautomaton
Copy link
Author

@mattwynne I realise this sounds complex, but I don't think in practice it will require as much maintenance as it seems. The maintenance overhead is:

  1. When a new minor version of rails comes out, add an appraisal section with its minimal dependencies
  2. When adding a feature that requires a gem, add it to each appraisal section and the Gemfile, as opposed to just the gemspec.

To me this seems like less hassle than managing releases and version numbers across multiple branches, but as I said in the other thread, I readily admit I'm no expert on managing a library. It's just been my experience that most gems I've seen that have had multi-branch releases have tended to abandon all but the edge branch very quickly.

@mattwynne
Copy link
Member

@urbanautomaton I'm only talking about having three branches - one for Rails 2, 3 and 4. If you think this new system can obviate that, then I'm willing to see us give it a go. 👍

@os97673
Copy link
Member

os97673 commented Mar 2, 2013

@urbanautomaton Thank you for explanation, it is more complicated then now but at least we can test our code with all rails at once
@mattwynne having three different branches means we will have to maintain three copy of our code and (more important) almost nobody will test her changes against all rails we support :( exception Travis, but this may be too late)

@os97673
Copy link
Member

os97673 commented Mar 2, 2013

@urbanautomaton BTW when I add new gem to an existing appraisal do I need to regenerate gemfile so Travis would use an updated version or I've missed the place where those gemfiles are regenerated automatically?

@mattwynne
Copy link
Member

On 2 Mar 2013, at 05:02, Oleg Sukhodolsky [email protected] wrote:

@mattwynne having three different branches means we will have to maintain three copy of our code and (more important) almost nobody will test her changes against all rails we support :( exception Travis, but this may be too late)

Yeah. My experience though is that this project doesn't change too often. What mostly changes is that a new version of Rails / Capybara comes out and we have to change this project to support that new version. Having branches would help avoid having lots of if-statements for each version in our code.

For example, see what happened here: #213. How would this system support merging a patch like that?

So is this ticket mutually exclusive with #238 then?

@os97673
Copy link
Member

os97673 commented Mar 2, 2013

@mattwynne I'd say the ticket is providing us with test environment which will show if we can leave with one branch for all supported rails or we have to create a dedicated branch for one of them.

@urbanautomaton
Copy link
Author

@os97673 Yes, if anything changes in the gemspec or appraisals, you would need to run rake gemfiles:rebuild to regenerate the appraisal gemfiles, and commit them with the other changes.

@mattwynne My original motivation for submitting this was to avoid the gemspec unnecessarily over-specifying the versions of its dependencies, while also addressing @aslakhellesoy's concern that the gemspec would then specify a range of dependencies that aren't fully tested. As this is a relatively small rails integration gem, your suggestion of just weakening the support guarantee is definitely another possibility - it's a tradeoff between the extra complexity of this PR, and potentially getting more issues raised in the future, I guess.

As for major rails versions: at the moment this PR only tackles a single Rails major version, 3.x. I already had trouble getting the same test suite to work across major versions of rails (as 4.0 can't be tested yet with mongoid, thanks to the latter's own restrictive gemspec), and didn't even attempt to make it work with rails 2.x. So on reflection, I don't think this approach will avoid the need for per-major-rails-version branches without significant extra code and test complexity.

@os97673
Copy link
Member

os97673 commented Mar 2, 2013

@os97673 Yes, if anything changes in the gemspec or appraisals, you would need to run rake gemfiles:rebuild to regenerate the appraisal gemfiles, and commit them with the other changes.

Could you please add couple lines about this to README.md (I have very bad memory ;)

Kosmas added a commit that referenced this pull request Mar 4, 2013
Multiple-gemfile testing and travis configuration
@Kosmas Kosmas merged commit ac9d843 into cucumber:master Mar 4, 2013
@Kosmas Kosmas mentioned this pull request Mar 4, 2013
@urbanautomaton
Copy link
Author

Hmm - sorry, I didn't get time to add the extra documentation before this was merged - can it be re-merged from here, or do I need to open a new PR?

@os97673
Copy link
Member

os97673 commented Mar 4, 2013

I think a new doc-only PR is better (not sure if it is possible to re-merge PR)

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.

4 participants