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

Add Rails 5.2 & Capybara 3 support. #378

Merged
merged 32 commits into from Apr 20, 2018
Merged

Conversation

gobijan
Copy link
Contributor

@gobijan gobijan commented Apr 12, 2018

Summary

Relax the runtime dependencies to allow railties version 5.2.

Details

Relax the runtime dependencies to allow railties version 5.2.

Motivation and Context

Allow Rails 5.2 now that it's released.

How Has This Been Tested?

No introduction of new tests. Relying on tests that are already there.

Screenshots (if appropriate):

none.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@mvz
Copy link
Contributor

mvz commented Apr 12, 2018

Maybe @gobijan and @roberts1000 can cooperate on this 😉.

@gobijan
Copy link
Contributor Author

gobijan commented Apr 12, 2018

Sure. I am experimenting on my PR and trying latest gem versions. @roberts1000 you have any findings to share for now?

@gobijan gobijan changed the title Add Rails 5.2 support. WIP: Add Rails 5.2 support. Apr 12, 2018
@gobijan gobijan changed the title WIP: Add Rails 5.2 support. [WIP] Add Rails 5.2 support. Apr 12, 2018
s.add_development_dependency('builder', ['>= 3.1.0', '< 4'])
s.add_development_dependency('bundler', '>= 1.3.5')
s.add_development_dependency('selenium-webdriver', '>= 3.4.1')
s.add_development_dependency('database_cleaner', '>= 1.0.0')
s.add_development_dependency('factory_girl', '>= 3.2')
s.add_development_dependency('rake', '>= 0.9.2.2')
s.add_development_dependency('rspec', '~> 3.0')
s.add_development_dependency('rspec', '~> 3')
Copy link
Contributor

Choose a reason for hiding this comment

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

This will allow RSpec 4, which is probably not what you want.

s.add_runtime_dependency('mime-types', ['>= 1.17', '< 4'])

# Main development dependencies
s.add_development_dependency('ammeter', ['>= 1.0.0', '!= 1.1.3'])
s.add_development_dependency('appraisal', '>= 0.5.1')
s.add_development_dependency('aruba', '~> 0.14.2')
s.add_development_dependency('aruba', '>= 0.14.2')
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest release of aruba is in the 0.14.x series, so this change has no effect currently.

@vovimayhem vovimayhem mentioned this pull request Apr 13, 2018
@@ -12,23 +12,23 @@ Gem::Specification.new do |s|

s.license = 'MIT'

s.add_runtime_dependency('capybara', ['>= 1.1.2', '< 3'])
s.add_runtime_dependency('capybara', ['>= 1.1.2', '< 4'])
Copy link
Member

Choose a reason for hiding this comment

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

This also resolves @Kosmas' request here: #375 (comment) 👍

Copy link
Member

Choose a reason for hiding this comment

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

👍

@xtrasimplicity xtrasimplicity changed the title [WIP] Add Rails 5.2 support. [WIP] Add Rails 5.2 & Capybara 4 support. Apr 14, 2018
@roberts1000
Copy link

@gobijan Glad to see someone else is taking a look at this one! I didn't hit the same test failures that you hit. I tried to avoid changing gem/ruby versions if they weren't related to Rails 5.2 so that may the reason for the difference. My PR is failing on a bootsnap loading error.

.travis.yml Outdated
@@ -1,8 +1,7 @@
rvm:
- 2.2.9
Copy link

Choose a reason for hiding this comment

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

I would suggest keeping this, as cucumber-rails still is using Ruby 2.2. If Rails 5.2 requires higher, then I suggest excluding it below in .travis.yml.

s.add_runtime_dependency('mime-types', ['>= 1.17', '< 4'])

# Main development dependencies
s.add_development_dependency('ammeter', ['>= 1.0.0', '!= 1.1.3'])
s.add_development_dependency('appraisal', '>= 0.5.1')
s.add_development_dependency('aruba', '~> 0.14.2')
s.add_development_dependency('aruba', '>= 0.14.2')
s.add_development_dependency('builder', ['>= 3.1.0', '< 4'])
s.add_development_dependency('bundler', '>= 1.3.5')
s.add_development_dependency('selenium-webdriver', '>= 3.4.1')
s.add_development_dependency('database_cleaner', '>= 1.0.0')
s.add_development_dependency('factory_girl', '>= 3.2')
Copy link

Choose a reason for hiding this comment

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

Consider changing this to factory_bot while you're here too. There will need to be some other changes made to the features (like emulate_javascript.feature too)

Copy link

Choose a reason for hiding this comment

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

I've just submitted a PR to your repo for this: gobijan#2

@radar
Copy link

radar commented Apr 17, 2018

Keen to get this merged so that we can upgrade to Rails 5.2 as well. To help with the effort, I'm opening a few PRs on @gobijan's repo too:

gobijan#1
gobijan#2

Make click_with_javascript_emulation take any args
@gobijan
Copy link
Contributor Author

gobijan commented Apr 20, 2018

I just merged the pull requests in. Let's see :)

@xtrasimplicity
Copy link
Member

xtrasimplicity commented Apr 20, 2018 via email

Copy link
Member

@xtrasimplicity xtrasimplicity left a comment

Choose a reason for hiding this comment

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

I've restarted the erroneous tests and can confirm everything is working. Let's work on the requested changes in the reviews above, then we can get this merged. :)

@@ -37,7 +37,7 @@ Gem::Specification.new do |s|
s.add_development_dependency('rdoc', '>= 3.4')
s.add_development_dependency('yard', '>= 0.8.7')

s.required_ruby_version = '>= 2.2.0'
s.required_ruby_version = '>= 2.3.0'
Copy link
Member

Choose a reason for hiding this comment

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

Cucumber-ruby supports >= 2.2, so we should change this back to keep things standardised.

@@ -16,12 +16,12 @@ Feature: Emulate Javascript
And I write to "features/step_definitions/s.rb" with:
"""
Given /^there is a widget named "([^"]*)"$/ do |name|
FactoryGirl.create(:widget, name: name)
FactoryBot.create(:widget, :name => name)
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't regress with this. :key => value is the old Ruby style. :)

@@ -73,12 +73,12 @@ Feature: Emulate Javascript
And I write to "features/step_definitions/s.rb" with:
"""
Given /^there is a widget named "([^"]*)"$/ do |name|
FactoryGirl.create(:widget, name: name)
FactoryBot.create(:widget, :name => name)
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't regress with this. :key => value is the old Ruby style. :)

@xtrasimplicity xtrasimplicity mentioned this pull request Apr 20, 2018
6 tasks
s.add_development_dependency('rake', '>= 0.9.2.2')
s.add_development_dependency('rspec', '~> 3.0')
s.add_development_dependency('rspec', '~> 3')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should become ~> 3.0 again to avoid allowing rspec 4 automatically when it arrives.

Copy link
Member

Choose a reason for hiding this comment

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

Woops. I'll fix that now - thanks @mvz!

@mvz
Copy link
Contributor

mvz commented Apr 20, 2018

Almost there \o/

@xtrasimplicity xtrasimplicity merged commit a431927 into cucumber:master Apr 20, 2018
@aslakhellesoy
Copy link
Contributor

Hi @gobijan,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

@xtrasimplicity
Copy link
Member

All merged! The last change wouldn't have any impact on the test suite. :)

@mvz mvz changed the title [WIP] Add Rails 5.2 & Capybara 3 support. Add Rails 5.2 & Capybara 3 support. Apr 20, 2018
@gobijan
Copy link
Contributor Author

gobijan commented Apr 20, 2018

Nice! Thanks everyone for contributing on this 😀 Was fun to see open source at it's best.
Also thanks for adding me to the cucumber org @aslakhellesoy I'm happy to help out here and there :)

radar pushed a commit to radar/geckodriver-helper that referenced this pull request Apr 21, 2018
When working on cucumber/cucumber-rails#378, we would sometimes see that the builds would be unable to connect to the Geckodriver. On further investigation, this seems to be caused by the Geckodriver not being downloaded in the first place. I think that this is due to GitHub's API rate limiting certain build machines from accessing their API.

A better approach would be to hardcode the URLs for the assets -- since they're easily predicatable -- and then to bump this gem whenever a new release comes out.
@radar
Copy link

radar commented Apr 21, 2018

Wonderful :)

I've opened a PR on DevicoSolutions/geckodriver-helper#5 which should hopefully put an end to the intermittent builds too.

@radar
Copy link

radar commented Apr 21, 2018

In the meantime, could we please get a new gem release containing this PR's fixes? I believe that would allow codebases that use cucumber-rails to also upgrade to Rails 5.2.

@xtrasimplicity
Copy link
Member

@radar, we'll need to finalise the changelog first (#385), but once we've done this, we'll have to get @mvz, @olleolleolle, or @aslak to proceed with a release (when they're ready).

@mvz
Copy link
Contributor

mvz commented Apr 21, 2018

@xtrasimplicity That's a different Aslak 🙂

@xtrasimplicity
Copy link
Member

xtrasimplicity commented Apr 21, 2018

Woops! Thanks @mvz.

@aslakhellesoy Would you be able to build the latest release and push it to rubygems.org, when you get a chance? (Or alternatively, could I have access to do this? 😀) Thanks heaps!

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.

None yet

8 participants