Prevent "have_http_status" from using deprecated methods in Rails 5.2+ #1951

wbreeze · 2018-01-31T13:49:25Z

This alternative PR (to #1945) Provides a minimum change to solve the problem of response status methods deprecated in Rails 5.2, as raised by issue #1857

This does so without deprecating or changing any status matchers in RSpec, so a Rails 5.2 upgrade will be transparent, at least with respect to not causing deprecation warnings for the :success, :error, and :missing values of has_http_status.

A major or minor release that eliminates the version check will either require Rails 5.2 and map :success to the successful call, or require everyone to change their tests.

JonRowe · 2018-02-01T02:20:06Z

You'll need to rebase this so you don't bring across a load of 3-7-maintenance commits across to master

wbreeze · 2018-02-01T13:37:30Z

Thank you for prompting that, Jon. Agreed and done. The commit history looked terrible, but the diff was okay. With your prompting, I rebased, squashed, and force pushed. Now you have one commit to backport if you want it.

My view of the migration strategy is:

Now: Patch or minor release with the version check
With Rails 6 or someday: major release that requires 5.2 (or later), eliminates the version check, and maps the old syntax to the new method.

With that, no-one has to port forty tests when they upgrade. (I can only guess the number of extant has_http_status :success tests, but doubt that it's a trivial count.)

wbreeze · 2018-02-01T13:48:31Z

lib/rspec/rails/matchers/have_http_status.rb

@@ -18,7 +18,12 @@ module HaveHttpStatus
        # @return response matcher instance
        def self.matcher_for_status(target)
          if GenericStatus.valid_statuses.include?(target)


Rather than check valid statuses and then doing the mapping, this could do the mapping, then check valid statuses. status_method = GenericStatus.map_status(target); if (status_method) ... Then GenericStatus encapsulates all of the logic. I'm willing to make that change.

I'd still like to see this class done away with, and the lookup method defined depending on the version of rails. See below.

JonRowe

Looking good but I'd still like to see a refactor here.

Also we'd be releasing this in the next minor version as we class it an enhancement, rather than a patch.

We're also unlikely to drop the old functionality for some time. We like to work with as many rails versions as possible.

JonRowe · 2018-02-15T07:57:13Z

lib/rspec/rails/matchers/have_http_status.rb

@@ -255,12 +260,11 @@ def initialize(type)
            @invalid_response = nil
          end

-          # @return [Boolean] `true` if Rack's associated numeric HTTP code matched
-          #   the `response` code
+          # @return [Boolean] value of response status method


I'd like to see the original doc restored, with maybe an additional " or the named response status matches"

JonRowe · 2018-02-15T07:57:46Z

lib/rspec/rails/matchers/have_http_status.rb

@@ -18,7 +18,12 @@ module HaveHttpStatus
        # @return response matcher instance
        def self.matcher_for_status(target)
          if GenericStatus.valid_statuses.include?(target)


I'd still like to see this class done away with, and the lookup method defined depending on the version of rails. See below.

JonRowe · 2018-02-15T08:01:04Z

lib/rspec/rails/matchers/have_http_status.rb

+
+          def check_expected_status(test_response, expected)
+            test_response.send("#{expected}?")
+          end


Basically this would become something like:

if if 5 < ::Rails::VERSION::MAJOR || (::Rails::VERSION::MAJOR == 5 && 2 <= ::Rails::VERSION::MINOR) def check_expected_status(test_response, expected) test_response.send("#{expected}?") end else RESPONSE_METHODS = { success: 'successful', error: 'server_error', missing: 'not_found' }.freeze def check_expected_status(test_response, expected) test_response.send("#{RESPONSE_METHODS.fetch(response_symbol, response_symbol)}?") end end

JonRowe · 2018-02-15T08:01:19Z

lib/rspec/rails/matchers/have_http_status.rb

@@ -327,6 +337,33 @@ def type_codes
                            end
          end
        end
+
+        class GenericStatusWithLookup < GenericStatus


And this would go away.

Document the newly added internal method, "response_method" Add the Rails 5.2 beta as a test environment for travis Avoid defining tests specific to rails 5.2+ when not relevant Conditionally define method lookup for Rails 5.2 deprecated response status methods Add context for Rails 5.2 http status deprecation tests Remove stray print statement left in have_http_status_spec

wbreeze · 2018-02-19T19:23:22Z

Jon:

Thank you for the further guidance on the change that you would like to see.

I'm afraid I went a little further, and made the change forward compatible as well as backward compatible. Allowing the new syntax for any version didn't seem right unless it worked for any version.

The two variants failing on Travis are failing in build.

JonRowe · 2018-02-20T00:22:19Z

lib/rspec/rails/matchers/have_http_status.rb

+            [
+              :error, :success, :missing,
+              :server_error, :successful, :not_found,
+              :redirect


Almost there! Can we just get some specs covering the usage of the extra methods, these should run on all versions of Rails to show they're being mapped correctly on older versions. Thank you so much for your patience!

Thank you for prompting those additional tests. What could go wrong? It turns out that the type codes came back empty in the messages for those new generic methods.

I couldn't resist drying up the tests with a shared_examples block. They were very repetitive.

…cessful

wbreeze · 2018-02-20T18:34:23Z

spec/rspec/rails/matchers/have_http_status_spec.rb

+        to(negated_message)
+    end
+  end
+


The diff from here down looks terrible. All that happened is:

the repeated code in this shared example was replaced with invocation of this shared example

new tests using the shared example were added for :not_found, :server_error, and :successful

wbreeze · 2018-02-20T18:35:42Z

spec/rspec/rails/matchers/have_http_status_spec.rb

+    it "raises an ArgumentError" do
+      expect{ have_http_status(nil) }.to raise_error ArgumentError
+    end
+  end



Everything from here on is actually new, not changed lines.

wbreeze · 2018-02-21T22:32:34Z

@JonRowe The two failing combinations are gem dependency errors from bundler. If you hate the refactoring, I'll redo it copy-paste.

Let me know if you would like any further changes.

JonRowe · 2018-02-22T10:54:45Z

Thanks for the effort getting this over the line!

[skip ci]

jesperronn · 2018-03-03T23:24:54Z

In my opinion, this calls for a release, since the deprecation warning is REALLY annoying when upgrading rails projects.

I would suggest a minor version bump, since it's an added feature for Rails 5.2 and it doesn't break existing behaviour.

JonRowe · 2018-03-03T23:31:12Z

/cc @samphippen

[skip ci]

jonekdahl · 2018-04-13T14:23:59Z

I second @jesperronn's suggestion for a point release. Now that Rails 5.2 is out, it would be neat if there was a released version of rspec-rails that doesn't not trigger deprecation warnings.

jfi · 2018-04-19T08:01:41Z

Any news on a release for this?

This resolves the deprecation warning: > The success? predicate is deprecated and will be removed in Rails 6.0. Please use successful? as provided by Rack::Response::Helpers. There is a [fix for this that has been merged](rspec/rspec-rails#1951) for rspec-rails, but is [not yet released](https://github.com/rspec/rspec-rails/blob/678b313cf3cc3bdb2c59149ee3e290fd590460d1/Changelog.md). We could revert it back afterwards if we're worried, but we expect this to be a `200` response at the moment, not a `202` or anything so this is fine I think.

Aesthetikx · 2018-06-18T19:55:36Z

@samphippen Sorry to bump but I would also appreciate a point release including this for our Rails 5.2 upgrade ❤️

Prevent "have_http_status" from using deprecated methods in Rails 5.2+

LeeXGreen · 2018-06-22T15:50:03Z

I would also really appreciate a point release with this fix.

We're currently pointing to 3.8.pre via Git SHAs, but it would be much nicer to just pull this from Rubygems.

[skip ci]

wbreeze mentioned this pull request Jan 31, 2018

Prevent "have_http_status" from using deprecated methods in Rails 5.2+ #1945

Closed

wbreeze commented Feb 1, 2018

View reviewed changes

JonRowe requested changes Feb 15, 2018

View reviewed changes

wbreeze added 8 commits February 19, 2018 11:15

Allow interpreter to define method based on rails version

72b9dce

Resolve git usage error; allow 5.6+ syntax on 5.6+

d6de331

Restore documentation matches method

37244c4

Test the new response status methods

14bad0d

Test the 5.2.0 release candidate in place of the beta

f97c177

Use old style hash for older Ruby

21cdb1a

Remove end of line whitespace from change log

6db0717

JonRowe reviewed Feb 20, 2018

View reviewed changes

wbreeze added 2 commits February 20, 2018 15:22

Dry up and add Generic code tests for :not_found, :server_error, :suc…

2196e44

…cessful

Provide type codes for :not_found, :server_error, :successful

811e390

wbreeze commented Feb 20, 2018

View reviewed changes

JonRowe mentioned this pull request Feb 20, 2018

Refactor spec for have_http_status.rb #1957

Closed

JonRowe approved these changes Feb 22, 2018

View reviewed changes

JonRowe merged commit 9bb089a into rspec:master Feb 22, 2018

JonRowe added a commit that referenced this pull request Feb 22, 2018

Changelog for #1951

14357eb

[skip ci]

benoittgt pushed a commit to benoittgt/rspec-rails that referenced this pull request Mar 4, 2018

Changelog for rspec#1951

17a209c

[skip ci]

wbreeze mentioned this pull request Mar 13, 2018

Remove the version check in have_http_status #1965

Merged

dependabot-preview bot mentioned this pull request Mar 21, 2018

Bump rspec-rails from 3.6.0 to 3.6.1 tnir/gitlabhq#102

Closed

jonekdahl mentioned this pull request Apr 19, 2018

Rails 6 will remove success? predicate in favor of successful? #1857

Closed

ykzts mentioned this pull request Apr 21, 2018

Use raw status code on have_http_status mastodon/mastodon#7214

Merged

MaicolBen mentioned this pull request May 2, 2018

Upgrade to Rails to 5.2 rootstrap/rails_api_base#120

Merged

2 tasks

colszowka mentioned this pull request May 11, 2018

Bump to Rails 5.2 rubytoolbox/rubytoolbox#214

Merged

2 tasks

Aesthetikx pushed a commit to Revela/rspec-rails that referenced this pull request Jun 18, 2018

Merge pull request rspec#1951 from wbreeze/successful

2a2e148

Prevent "have_http_status" from using deprecated methods in Rails 5.2+

wbreeze mentioned this pull request Jun 22, 2018

Cherry pick 1951 for 3-7 maintenance #2004

Closed

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

Changelog for rspec#1951

8ff3029

[skip ci]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent "have_http_status" from using deprecated methods in Rails 5.2+ #1951

Prevent "have_http_status" from using deprecated methods in Rails 5.2+ #1951

wbreeze commented Jan 31, 2018

JonRowe commented Feb 1, 2018

wbreeze commented Feb 1, 2018

wbreeze Feb 1, 2018

JonRowe Feb 15, 2018

JonRowe left a comment

JonRowe Feb 15, 2018

JonRowe Feb 15, 2018

JonRowe Feb 15, 2018

JonRowe Feb 15, 2018

wbreeze commented Feb 19, 2018

JonRowe Feb 20, 2018

wbreeze Feb 20, 2018

wbreeze Feb 20, 2018 •

edited

wbreeze Feb 20, 2018

wbreeze commented Feb 21, 2018

JonRowe commented Feb 22, 2018

jesperronn commented Mar 3, 2018

JonRowe commented Mar 3, 2018 via email

jonekdahl commented Apr 13, 2018 •

edited

jfi commented Apr 19, 2018

Aesthetikx commented Jun 18, 2018

LeeXGreen commented Jun 22, 2018

Prevent "have_http_status" from using deprecated methods in Rails 5.2+ #1951

Prevent "have_http_status" from using deprecated methods in Rails 5.2+ #1951

Conversation

wbreeze commented Jan 31, 2018

JonRowe commented Feb 1, 2018

wbreeze commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonRowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbreeze commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbreeze Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbreeze commented Feb 21, 2018

JonRowe commented Feb 22, 2018

jesperronn commented Mar 3, 2018

JonRowe commented Mar 3, 2018 via email

jonekdahl commented Apr 13, 2018 • edited

jfi commented Apr 19, 2018

Aesthetikx commented Jun 18, 2018

LeeXGreen commented Jun 22, 2018

wbreeze Feb 20, 2018 •

edited

jonekdahl commented Apr 13, 2018 •

edited