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

Delegate StubConnection#pubsub and StubConnection#config to ActionCable.server #48706

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

julianfssen
Copy link
Contributor

@julianfssen julianfssen commented Jul 10, 2023

Motivation / Background

Previously, ActionCable::Channel::ConnectionStub would throw a NoMethodError when the channel under test calls stop_stream_from, as shown in the test case below:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "actioncable", "~> 7.0.0"
end

require "minitest/autorun"
require "action_cable"

ActionCable.server.config.cable = { "adapter" => "test" }

class ChatChannel < ActionCable::Channel::Base
  def subscribed
    stream_from "test"
  end 

  def unsubscribed
    stop_stream_from "test"
  end 
end

class ChatChannelTest < ActionCable::Channel::TestCase
  test "unsubscribe" do
    stub_connection

    subscribe
    unsubscribe # raises a NoMethodError

    assert_no_streams
  end 
end

This happens because stop_stream_from calls pubsub when unsubscribing from a channel, which is missing from the ConnectionStub implementation.

Detail

This PR fixes this issue by assigning the ActionCable.server singleton to a @server instance variable in
ActionCable::Channel::ConnectionStub.

This lets us delegate the config and pubsub method calls to it, hence fixing the NoMethodError issue.

I updated config to be delegated to @server as well since config defaults to ActionCable::Server::Configuration.new.

We do something similar in ActionCable::TestHelper#pubsub_adapter as well:

def pubsub_adapter # :nodoc:
ActionCable.server.pubsub
end

Additional information

I'm not sure if I should add a changelog entry for this. But, I added it anyway since I feel adding@server = ActionCable.server qualifies as a behavior change.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@julianfssen julianfssen changed the title Delegate StubConnection#pubsub and StubConnection#config to ActionCab… Delegate StubConnection#pubsub and StubConnection#config to ActionCable.server Jul 10, 2023
…le.server

Previously, `ActionCable::Channel::ConnectionStub` would
throw a `NoMethodError` when the channel under test calls
`stop_stream_for`, as shown in the example below:

```ruby
class ChatChannel < ActionCable::Channel::Base
  def subscribed
    stream_from "test"
  end

  def unsubscribed
    stop_stream_from "test"
  end
end

class ChatChannelTest < ActionCable::Channel::TestCase
  test "unsubscribe" do
    stub_connection

    subscribe

    # `unsubscribe` raises `NoMethodError` as `pubsub` does not
      exist on `ActionCable::Channel::ConnectionStub`
    unsubscribe

    assert_no_streams
  end
end
```

Calling `unsubscribe` causes an exception as `stop_stream_from` calls
`pubsub` when unsubscribing, which is not implemented in
`ActionCable::Channel::ConnectionStub`.

This commit fixes this issue by assigning the `ActionCable.server`
singleton to a `@server` instance variable in
`ActionCable::Channel::ConnectionStub`. This lets us delegate
the `config` and `pubsub` method calls to it.
@julianfssen julianfssen force-pushed the add-pubsub-to-connection-stub branch from b4a25a2 to 9c5175e Compare July 15, 2023 05:05
@julianfssen
Copy link
Contributor Author

Good morning, @byroot !

I saw you were involved with merging the last few ActionCable-related PRs to main. I was wondering if you'd have the time to review this PR?

It involves a test-related issue so it's not a big issue and I understand if it's not crucial to merge in for now. Thank you!

@byroot
Copy link
Member

byroot commented Jul 17, 2023

LGTM.

@byroot byroot merged commit e18791c into rails:main Jul 17, 2023
8 of 9 checks passed
@julianfssen
Copy link
Contributor Author

LGTM.

Awesome! Thank you, @byroot 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants