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.

Sign up for GitHub

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

Jump to bottom

Tests fail with new bundler on CI #816

Closed
tijmenb opened this issue Mar 6, 2017 · 13 comments
Closed

Tests fail with new bundler on CI #816

tijmenb opened this issue Mar 6, 2017 · 13 comments

Comments

@tijmenb
Copy link
Contributor

tijmenb commented Mar 6, 2017

We're seeing these errors on master:

LoadError: cannot load such file -- active_model/translation
/var/lib/jenkins/bundles/manuals-publisher/deployed-to-production/ruby/2.2.0/gems/activemodel-3.2.22.3/lib/active_model/validations.rb:49:in `block in <module:Validations>'
/var/lib/jenkins/bundles/manuals-publisher/deployed-to-production/ruby/2.2.0/gems/activesupport-3.2.22.3/lib/active_support/concern.rb:121:in `class_eval'
/var/lib/jenkins/bundles/manuals-publisher/deployed-to-production/ruby/2.2.0/gems/activesupport-3.2.22.3/lib/active_support/concern.rb:121:in `append_features'

This has also been noted in 0a674b5.
tijmenb added a commit to alphagov/govuk-content-schemas that referenced this issue Mar 6, 2017
@tijmenb
Remove manuals-publisher as downstream app
ba2ea53 
Manuals publisher tests are currently failing, holding up work on this
repo. See alphagov/manuals-publisher#816.
@tijmenb tijmenb mentioned this issue Mar 6, 2017
Closed
@tijmenb
Copy link
Contributor Author

tijmenb commented Mar 6, 2017

alphagov/govuk-content-schemas#555 should be reverted after this is fixed.

@h-lame
Copy link
Contributor

h-lame commented Mar 6, 2017

Don't really know what the cause is but it works with bundler 1.12.6 (latest 1.12 series) and fails with bundler 1.13.0 (first in bundler 1.13 series). This might help with tracking it down - although nothing obvious jumps out at me from the bundler changelog.

@h-lame
Copy link
Contributor

h-lame commented Mar 6, 2017

You can trigger it by running bundle exec rake spec:javascript or bundle exec rake cucumber. bundle exec rake spec works, bundle exec rake test runs the specs and then fails when it tries to run the cucumber features. The problem may lurk in the cucumber and jasmine rakefiles and how they require things.

@floehopper
Copy link
Contributor

floehopper commented Mar 7, 2017 •
edited
Loading

I assume this has been triggered by the upgrade of Bundler for Ruby v2.2.4 from v1.10.6 to v1.14.5 in alphagov/govuk-puppet@857083c.

I think the problem is to do with the mongoid_rails_migrations gem; more specifically because the gem calls Bundle.require when it is required by Bundler - see this line.

It looks as if someone attempted a proper fix for this in adacosta/mongoid_rails_migrations#3 6 years ago, but the author didn't seem to understand the problem. I suggest we either find a fork where this has been fixed or implement a new version of that PR in our own fork.

@h-lame
Copy link
Contributor

h-lame commented Mar 7, 2017 •
edited
Loading

The problem seems to be: rubygems/bundler#5500. Both cucumber and jasmine-rails fork the ruby process when running their rake tasks and these "nested" bundle exec calls seem to mess up the bundle environment. Note that jasmine-rails only if the RAILS_ENV isn't already test so we can avoid the problem by running RAILS_ENV=test bundle exec rake spec:javascript. We can avoid the problem with cucumber too by changing t.fork = true to t.fork = false in lib/tasks/cucumber.rake and also running with an explicit RAILS_ENV (e.g. RAILS_ENV=test bundle exec rake cucumber). There's a fix in rubygems/bundler#5504 that asks bundler not to reset (this change introduced in bundler 1.13.x) and doing the same change locally fixes the problem without needing to change our rake tasks, or be explicit about RAILS_ENV when running the rake tasks.

@floehopper
Copy link
Contributor

@h-lame: Do the changes you recommend mean that we end up with "non-standard" rake tasks? Would it not be better to fix the problem in the mongoid_rails_migration gem?

@h-lame
Copy link
Contributor

h-lame commented Mar 7, 2017

I don't think we should "fix" the rake tasks because we can't really; they need to be run with different arguments. I'm not entirely sure that using a fixed mongoid_rails_migration gem solves the problem, but I could be wrong. I think we need to have a bigger debate about rolling back the bundler update until rubygems/bundler#5504 is merged and released.

@h-lame
Copy link
Contributor

h-lame commented Mar 7, 2017

Ofc, using a fixed mongoid_rails_migration gem might mean we need to upgrade to a newer mongoid which means a newer rails. Not trivial, but also not useless.

floehopper added a commit to freerange/mongoid_rails_migrations that referenced this issue Mar 7, 2017
@floehopper
Avoid calling Bundler.require in library code
50b9137 
This was causing us problems [1] in an application which uses Bundler
when we upgraded to Bundler >= v1.13.0.

I think the right solution is something along the lines of adacosta#3, but I'm
trying this as a temporary quick fix.

[1]: alphagov/manuals-publisher#816
floehopper added a commit that referenced this issue Mar 7, 2017
@floehopper
Use fork of mongoid_rails_migrations to fix build
ffeee9c 
* This is an attempt to fix the problem described in #816. It uses a
forked version of mongoid_rails_migrations. The branch in that fork
was branched off the v1.0.0 tag, which is the version of the gem that
was specified in our `Gemfile`. This is so that we keep the changes to
our app to a minimum.

* This fix is a bit of a bodge - the proper fix would be along the
lines of adacosta/mongoid_rails_migrations#3, but given that the failing
build is a significant blocker, it feels as if this is worth a try.
If it works OK, we can work on getting the proper fix in place.

* I installed v1.14.5 of Bundler which is the version now specified in
govuk-puppet [1] for Ruby v2.2.4. Hence the change to `BUNDLED WITH`
in the `Gemfile`.

* I've run a migration locally to ensure that the gem still works OK
even with this patch in place.

[1]: https://github.com/alphagov/govuk-puppet/blob/e2d61705aaf6da180a781b6f46f83235fd890df0/modules/govuk_rbenv/manifests/all.pp#L46
@floehopper floehopper mentioned this issue Mar 7, 2017
Merged
@floehopper
Copy link
Contributor

I've just pushed up a bodgy fix: #822.

floehopper added a commit to alphagov/mongoid_rails_migrations that referenced this issue Mar 7, 2017
@floehopper
Avoid calling Bundler.require in library code
4db2344 
This was causing us problems [1] in an application which uses Bundler
when we upgraded to Bundler >= v1.13.0.

I think the right solution is something along the lines of adacosta#3, but I'm
trying this as a temporary quick fix.

Note that this branch is based off the v1.0.0 tag which is the version
of the gem that we are currently using in our app.

[1]: alphagov/manuals-publisher#816
floehopper added a commit that referenced this issue Mar 7, 2017
@floehopper
Use fork of mongoid_rails_migrations to fix build
a80f96c 
* This is an attempt to fix the problem described in #816. It uses a
forked version of mongoid_rails_migrations. The branch in that fork
was branched off the v1.0.0 tag, which is the version of the gem that
was specified in our `Gemfile`. This is so that we keep the changes to
our app to a minimum.

* This fix is a bit of a bodge - the proper fix would be along the
lines of adacosta/mongoid_rails_migrations#3, but given that the failing
build is a significant blocker, it feels as if this is worth a try.
If it works OK, we can work on getting the proper fix in place.

* I installed v1.14.5 of Bundler which is the version now specified in
govuk-puppet [1] for Ruby v2.2.4. Hence the change to `BUNDLED WITH`
in the `Gemfile`.

* I've run a migration locally to ensure that the gem still works OK
even with this patch in place.

[1]: https://github.com/alphagov/govuk-puppet/blob/e2d61705aaf6da180a781b6f46f83235fd890df0/modules/govuk_rbenv/manifests/all.pp#L46
@floehopper
Copy link
Contributor

In the short term, we're not planning on trying to improve this fix unless we run into specific problems with it. I'm going to leave this issue open for the moment as a reminder that in the longer run it would be good to come up with a better fix.

@chrisroos
Copy link
Contributor

I'm going to leave this issue open for the moment as a reminder that in the longer run it would be good to come up with a better fix.

@floehopper: How about opening a new issue to capture any improvement given that this issue has been fixed?

@floehopper
Copy link
Contributor

@chrisroos: Sure. Be my guest.

@chrisroos chrisroos mentioned this issue Mar 15, 2017
Open
@chrisroos
Copy link
Contributor

I've created issue #874 to remind us that we might want to improve the fix in mongoid_rails_migrations.

I can now close this issue.

floehopper added a commit to alphagov/mongoid_rails_migrations that referenced this issue May 25, 2017
@floehopper
Avoid calling Bundler.require in library code
4affab7 
This was causing us problems [1] in an application which uses Bundler
when we upgraded to Bundler >= v1.13.0.

I think the right solution is something along the lines of adacosta#3, but I'm
trying this as a temporary quick fix.

Note that this branch is based off the v1.0.0 tag which is the version
of the gem that we are currently using in our app.

[1]: alphagov/manuals-publisher#816
floehopper added a commit to alphagov/mongoid_rails_migrations that referenced this issue May 25, 2017
@floehopper
Avoid calling Bundler.require in library code
e3e0374 
This was causing us problems [1] in an application which uses Bundler
when we upgraded to Bundler >= v1.13.0.

I think the right solution is something along the lines of these pull
requests [2,3], but since these haven't been merged by the maintainer,
I'm using this commit as a temporary fix.

[1]: alphagov/manuals-publisher#816
[2]: adacosta#3
[3]: adacosta#40
floehopper added a commit to alphagov/mongoid_rails_migrations that referenced this issue May 25, 2017
@floehopper
Avoid calling Bundler.require in library code
7dc2c19 
This was causing us problems [1] in an application which uses Bundler
when we upgraded to Bundler >= v1.13.0.

I think the right solution is something along the lines of these pull
requests [2,3], but since these haven't been merged by the maintainer,
I'm using this commit as a temporary fix.

[1]: alphagov/manuals-publisher#816
[2]: adacosta#3
[3]: adacosta#40
floehopper added a commit that referenced this issue May 25, 2017
@floehopper
Upgrade mongoid_rails_migrations gem to v1.1.0+
598f669 
This upgrade from v1.0.0 includes a few changes to improve compatibility
with more recent versions of Mongoid and Rails. However, I haven't been
able to use the v1.1.0 release version of the gem for three reasons:

* Since we're now on Mongoid v6, we need the fix [1] in the latest
commit on master in the canonical repo in order for
`Mongoid::Migration.connection` method to work. I've incorporated this
commit in the new branch in the alphagov fork.

* Since we're now on Rails v5, we need a fix [2] for the absence of
`alias_method_chain`. I've incorporated the commit from this PR [3] into
the new branch in the alphagov fork.

* The problem described in #816 still exists. The maintainer of the gem
still hasn't fixed the problem. This earlier suggested fix [4] has been
closed without explanation. A new fix [5] has been proposed and I've
added a comment to explain why I think it should be merged. Therefore
I've incorporated my original fix as the last commit in the new branch
in the alphagov fork.

I've checked that I can generate and run the following migration:

    class Foo < Mongoid::Migration
      def self.up
        p SectionEdition.count
        p SectionEdition.collection
        p connection.database.collection_names
      end

      def self.down
      end
    end

I've also updated the existing migration so that I think they would
work. However, I haven't tested them.

[1]: adacosta/mongoid_rails_migrations@0074dbf
[2]: stormz/mongoid_rails_migrations@b9158cd
[3]: adacosta/mongoid_rails_migrations#44
[4]: adacosta/mongoid_rails_migrations#3
[5]: adacosta/mongoid_rails_migrations#40
@floehopper floehopper mentioned this issue May 25, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@h-lame @floehopper @chrisroos @tijmenb