Rails 5.1 compatibility #1790

fables-tales · 2017-02-24T16:32:21Z

No description provided.

Rails 5.1 removes `rails:template` in favour of `app:template` to generate templates. This change sets our rakefile up to usee the `app:template` command if the rails version is greater than or equal to 5.1

fables-tales · 2017-03-23T13:01:40Z

@yui-knk btw, here's the current failure output when I run RAILS_VERSION=5.1.0.beta1 ./script/run_build in my clone of RSpec Rails: https://gist.github.com/samphippen/bcc02764dca82bb8f416a798f9561d8e

myronmarston · 2017-03-23T14:06:05Z

@samphippen any reason not to update this to 5.1.0.rc1? That's going to be closer to the 5.1 final release, and it's possible the build failures are related to a regression in beta1 that's fixed in rc1.

fables-tales · 2017-03-23T14:45:04Z

No reason, just haven't had the time. Sent from my phone please excuse my brevity.

…

On 23 Mar 2017, at 10:06, Myron Marston ***@***.***> wrote: @samphippen any reason not to update this to 5.1.0.rc1? That's going to be closer to the 5.1 final release, and it's possible the build failures are related to a regression in beta1 that's fixed in rc1. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

From Rails 5.1, a form view which is generated by scaffold command uses `form_with`. `form_with` does not append id attribute for form tags or input tags. So our templates should not use id selectors. rails/rails@d3b798b

yui-knk · 2017-03-24T07:21:05Z

I create #1797. Can you merge this to run CI?

Bump up Rails version to 5.1.0.rc1

`render :text` has been deprecated for a while now, and is removed in rails 5.1. It has been replaced by either `render :plain` or `render :html` (depending on which `Content-Encoding` header you want). This patch uses our existing cucumber tags `@rails_pre_5` and `@rails_post_5` to switch which we use depending on the Rails verison. We can't simply replace all the references here because Rails 3 does not have this option.

fables-tales · 2017-03-25T21:15:19Z

@rspec/rspec this is now ready for review. I've tested it locally against 5.1 and 5.0 and everything still works. Travis is cooking the other versions, but I don't expect much variance there.

JonRowe · 2017-03-26T23:57:14Z

features/request_specs/request_spec.feature

+    When I run `rspec spec/requests/widget_management_spec.rb`
+    Then the example should pass
+
+


Extra ws here

JonRowe · 2017-03-26T23:57:28Z

features/matchers/have_http_status_matcher.feature

@@ -136,7 +162,6 @@ Feature: `have_http_status` matcher
          visit "/widgets/new"
          expect(page).to have_http_status(200)

-          fill_in "Name", :with => "My Widget"


has no effect and breaks in Rails 5.1 for really weird reasons.

Is that indicative that we have an issue here?

There's no issue here. In rails 5.1 they've changed the form generation logic so this finder fails. However, this fill_in has nothing to do with the test. I think we should remove it

for compatibility

because it doesn't aid in being documenting of this feature

JonRowe · 2017-03-26T23:58:18Z

lib/generators/rspec/scaffold/templates/edit_spec.rb

@@ -16,8 +16,12 @@
    assert_select "form[action=?][method=?]", <%= ns_file_name %>_path(@<%= ns_file_name %>), "post" do
 <% for attribute in output_attributes -%>
      <%- name = attribute.respond_to?(:column_name) ? attribute.column_name : attribute.name %>
+<% if Rails.version.to_f >= 5.1 -%>


Worth indenting to match above? Makes it slightly clearer too.

The problem here is that indentation here affects the output. We want the output to look 'normal' do we don't indent here.

You could indent inside the tag if thats your concern?

Can you show how you'd like this to look, I'm not sure I fully follow?

JonRowe · 2017-03-26T23:58:28Z

lib/generators/rspec/scaffold/templates/new_spec.rb

@@ -15,8 +15,12 @@
    assert_select "form[action=?][method=?]", <%= index_helper %>_path, "post" do
 <% for attribute in output_attributes -%>
      <%- name = attribute.respond_to?(:column_name) ? attribute.column_name : attribute.name %>
+<% if Rails.version.to_f >= 5.1 -%>


JonRowe · 2017-03-26T23:59:21Z

Seems sane to me but I've not run it, if you're happy with it I'm happy with it. I've left a few non blocking comments on style.

yui-knk · 2017-03-27T01:11:29Z

Rakefile

@@ -11,6 +11,15 @@ require 'rspec'
 require 'rspec/core/rake_task'
 require 'cucumber/rake/task'

+def rails_template_command
+  require "rails"
+  if Rails.version.to_f >= 5.1


I think if Rails.version.to_f >= 5.0 is better, because "rails:template" was deprecated on Rails 5.0.

if it passes on CI it's good with me :)

fables-tales · 2017-04-02T23:37:19Z

@JonRowe I've just pushed some review tweaks and answered a few of your review pieces. WDYT?

fables-tales · 2017-04-09T23:46:50Z

@JonRowe I'm gonna go ahead and merge this. If you feel particularly strongly on the indentation or the deleted line, I think we can open a fresh PR.

Sam Phippen added 2 commits February 24, 2017 11:31

Use app:template instead of rails:template in Rails 5.1

2235c19

Rails 5.1 removes `rails:template` in favour of `app:template` to generate templates. This change sets our rakefile up to usee the `app:template` command if the rails version is greater than or equal to 5.1

Add 5.1 to the travis.yml

c0a1f61

fables-tales mentioned this pull request Mar 23, 2017

Switch template task name depending on Rails version #1796

Closed

yui-knk added 2 commits March 24, 2017 10:33

Bump up Rails version to 5.1.0.rc1

9d7bbee

Omit an id attribute from "scaffold/templates"

84cfefb

From Rails 5.1, a form view which is generated by scaffold command uses `form_with`. `form_with` does not append id attribute for form tags or input tags. So our templates should not use id selectors. rails/rails@d3b798b

Sam Phippen added 4 commits March 24, 2017 11:59

Merge pull request #1797 from yui-knk/rails51rc1

3e28547

Bump up Rails version to 5.1.0.rc1

Get cukes passing on 5.1

2e4420d

Fix scaffold generator

f53ee46

fables-tales changed the title ~~WIP: Rails 5.1 compatibility~~ Rails 5.1 compatibility Mar 25, 2017

JonRowe reviewed Mar 26, 2017

View reviewed changes

yui-knk reviewed Mar 27, 2017

View reviewed changes

Review tweaks

915e2c3

fables-tales merged commit 23c891e into master Apr 9, 2017

fables-tales deleted the rails-5.1-compat branch April 9, 2017 23:47

fables-tales pushed a commit that referenced this pull request Apr 9, 2017

Add a changelog entry for #1790

1b37ac6

This was referenced Mar 20, 2018

Bump rspec-rails, rails, fastly-rails and dotenv-rails rubytogether/rubytogether.org#178

Merged

Bump rspec-rails from 3.6.1 to 3.7.2 sumyapp/basecamp3_slack_notify#26

Merged

sebjacobs pushed a commit to futurelearn/rspec-rails that referenced this pull request Mar 15, 2019

Add a changelog entry for rspec#1790

376b493

		When I run `rspec spec/requests/widget_management_spec.rb`
		Then the example should pass

Uh oh!

Rails 5.1 compatibility #1790

Rails 5.1 compatibility #1790

Uh oh!

Conversation

fables-tales commented Feb 24, 2017

Uh oh!

fables-tales commented Mar 23, 2017

Uh oh!

myronmarston commented Mar 23, 2017

Uh oh!

fables-tales commented Mar 23, 2017 via email

Uh oh!

yui-knk commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fables-tales commented Mar 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonRowe Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonRowe commented Mar 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fables-tales commented Apr 2, 2017

Uh oh!

fables-tales commented Apr 9, 2017

Uh oh!

Uh oh!

yui-knk commented Mar 24, 2017 •

edited

Loading

JonRowe Apr 3, 2017 •

edited

Loading