rspec / rspec-rails

RSpec for Rails 6+
https://rspec.info
MIT License
5.16k stars 1.03k forks source link

actioncable channel helpers not being included for rails 5.2 projects #2377

Open robmathews opened 4 years ago

robmathews commented 4 years ago

What Ruby, Rails and RSpec versions are you using?

Ruby version: 2.7.1 Rails version: 5.2.4.3 RSpec version: rspec-rails (4.0.1)

Observed behaviour

I write a channel rspec like this:

RSpec.describe GmiChannel do
  before do
    stub_connection current_user: nil
  end
  it 'fails' do
  end
end

and it fails with

     NoMethodError:
       undefined method `stub_connection' for #<RSpec::ExampleGroups::GmiChannel::OlderRun:0x00007fa138fcd498>

Expected Behaviour

I expect stub_connection to be defined. Perhaps the version check here should allow rails 5.2:

   26:       def has_action_cable_testing?
   27          defined?(::ActionCable) && ActionCable::VERSION::MAJOR >= 6
   28        end

tl; dr section

I've tracked this down to a mismatch in version numbers. rspec-rails has the following method:

rspec-rails-4.0.1/lib/rspec/rails/matchers.rb:

   29: if RSpec::Rails::FeatureCheck.has_action_cable_testing?
   30    require 'rspec/rails/matchers/action_cable'
   31  end

And also,

rspec-rails-4.0.1/lib/rspec/rails/feature_check.rb:

   26:       def has_action_cable_testing?
   27          defined?(::ActionCable) && ActionCable::VERSION::MAJOR >= 6
   28        end

In my project, rails is '5.2.4.3' and so actioncable is version 5.2.4.3.

Now, I'm happy to continue using action-cable-testing, but unfortunately, it detects that rspec defines " has_action_cable_testing?", issues a deprecation warning, and then doesn't load.

Maybe I should file the bug report over there? Not sure, plus I know that he's deprecating that gem, so starting here.

pirj commented 4 years ago

As far as I remember action-cable-testing was merged into rspec-rails, so it doesn't make much sense to report there.

What happens if you add

require 'rspec/rails/matchers/action_cable'

to your spec/rails_helper.rb?

I couldn't find any reason why we only support it for Rails 6. @palkan, do you think it's safe to adjust the condition to support at least 5.2, or remove the version check altogether and rely on defined?(ActionCable)? I remember we have had issues with ActiveRecord being defined, but not actually used, but I don't think we have any special initialization code that would affect users in such way Active Record initialization attempts do.

robmathews commented 4 years ago

Well, it does define that method, but something else is missing. I get further, into my test and get this error

  1) GmiChannel older run stuff
     Failure/Error:
       expect {
         index_run.broadcast!
       }.to have_broadcasted_to(user).with(message)

       expected to broadcast exactly 1 messages to Z2lkOi8vY29kZS9Vc2VyLzk1 with {"type"=>"index_updated", "payload"=>{"stale"=>false, "gmi"=>69, "latest"=>false, "calculating"=>false, "date"=>"2014-04-28", "id"=>111279, "calculation_state"=>"available", "timezone"=>"America/New_York", "meal_plan_id"=>nil}}, but broadcast 0

Note that this is a test that used to pass, and when I run the code locally (testing manually), it does work, so there is something else broken in the test.

Not sure what part of actioncable testing does the broadcasting?

robmathews commented 4 years ago

the complete spec is

  describe "older run" do
    let(:index_run) { other_index_run }
    it "stuff" do
      subscribe
      expect {
        index_run.broadcast!
      }.to have_broadcasted_to(user).with(message)
    end
  end

(so it's not missing the subscribe)

robmathews commented 4 years ago

Ok, so maybe I'm stumbling across the next error and should file a separate ticket for that?

pirj commented 4 years ago

Are you up for some debugging? I suggest setting a breakpoint here https://github.com/rspec/rspec-rails/blob/main/lib/rspec/rails/matchers/action_cable/have_broadcasted_to.rb#L120 and check why the broadcasted message doesn't match exactly. Let's confirm that there's a bug first.

robmathews commented 4 years ago

of course! just let me sleep on it and get back to you tomorrow ;)

palkan commented 4 years ago

There is a similar issue I have for action-cable-testing: https://github.com/palkan/action-cable-testing/issues/76

do you think it's safe to adjust the condition to support at least 5.2, or remove the version check altogether and rely on defined?(ActionCable)?

I'm afraid it could make things worse.

Probably, a better alternative would be to check a specific class presence not a version, say:

def has_action_cable_testing?
  return false unless defined?(::ActionCable)

  begin
    # trigger autoload here
    ActionCable::Channel::TestCase
  rescue NameError
    false
  end
end
JonRowe commented 4 years ago

@palkan I'm confused, does Rails 5.2 have your patches to support testing channels? Or is this a case of your gem provides the patches and we don't (so your gem works with Rails 5.2, but we can't?)

robmathews commented 4 years ago

@prij when I set a breakpoint in there, messages is an empty array.

robmathews commented 4 years ago

near as I can tell, lib/action_cable/subscription_adapter/test.rb isn't being loaded or called. (in the action-cable-testing gem)

palkan commented 4 years ago

does Rails 5.2 have your patches to support testing channels?

Nope, Rails 5.2 has no action cable testing built-in. The action-cable-testing gem provides both Rails test classes (has been merged into Rails 6.0) and RSpec integration (has been merged into Rails 5.2). So, the gem could be used with any combination of Rails and RSpec Rails only to add the missing parts. In theory. In practice, however, it doesn't work out-of-the-box with RSpec Rails 4, because RSpec uses versions for feature checking (and not actual feature checking).

The current workaround is to patch FeatureCheck like this:

require "action_cable/testing"
require "rspec/rails/feature_check"

RSpec::Rails::FeatureCheck.module_eval do
  module_function

  def has_action_cable_testing?
    true
  end
end

require "rspec/rails"

Should we make rspec-rails checks version-independent (see https://github.com/rspec/rspec-rails/issues/2377#issuecomment-679106872) or should we add a patch to action-cable-testing to handle this? I think, the latter one is better: feature checking is good when it works and confusing when it doesn't (e.g., someone can add a fake ActionCable::TestCase class).

JonRowe commented 4 years ago

Ok, can you suggest a feature check for detecting either your gem or the Rails 6 functionality? I'm guessing the ActionCable constant itself is not enough because on its own 5.2 will have that but won't have your helpers?

palkan commented 4 years ago

The following should work:

def has_action_cable_testing?
  defined?(::ActionCable) && (ActionCable::VERSION::MAJOR >= 6 || defined?(::ActionCable::Channel::TestCase))
end

Rails 6 setups autoload for ActionCable::Channel::TestCase, while action-cable-testing requires it on load, thus, checking for either Rails 6 version or test case class presence should be sufficient.

pirj commented 4 years ago

@palkan Thanks!

@robmathews Can you please check if the above work for you on Rails 5.2 and action-cable-testing loaded?

JonRowe commented 4 years ago

This change is up on action-cable-flag-for-5-2 without any tests should anyone want to try it within their own suite without forking, if someone else could put together some test scenarios (in a smoke test prehaps?) that would be great.