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

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

Open
robmathews opened this issue Aug 21, 2020 · 15 comments
Open

Comments

@robmathews
Copy link

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
Copy link
Member

pirj commented Aug 21, 2020

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

@pirj
Copy link
Member

pirj commented Aug 21, 2020

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
Copy link
Author

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

@palkan
Copy link
Contributor

palkan commented Aug 24, 2020

There is a similar issue I have for action-cable-testing: palkan/action-cable-testing#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
Copy link
Member

JonRowe commented Aug 24, 2020

@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
Copy link
Author

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

@robmathews
Copy link
Author

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
Copy link
Contributor

palkan commented Aug 26, 2020

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 #2377 (comment)) 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
Copy link
Member

JonRowe commented Aug 26, 2020

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
Copy link
Contributor

palkan commented Aug 26, 2020

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
Copy link
Member

pirj commented Aug 26, 2020

@palkan Thanks!

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

@JonRowe
Copy link
Member

JonRowe commented Aug 26, 2020

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.

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

No branches or pull requests

4 participants