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 channels unit-testing #27191

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@palkan
Copy link
Contributor

commented Nov 27, 2016

Extracted from #23211.

Example:

class ChatChannelTest < ActionCable::Channel::TestCase
  def test_subscribed_with_room_number
    # Simulate a subscription creation
    subscribe room_number: 1

    # Asserts that the subscription was successfully created
    assert subscription.confirmed?

    # Asserts that the channel subscribes connection to a stream
    assert "chat_1", streams.last
  end
  
  def test_subscribed_without_room_number
     subscribe

     # Asserts that the subscription was rejected
     assert subscription.rejected?
  end
  
  def test_perform_speak
    subscribe room_number: 1

    perform :speak, message: "Hello, Rails!"
    
    assert_equal "Hello, Rails!", transmissions.last["message"]["text"]
  end
end
@rails-bot

This comment has been minimized.

Copy link

commented Nov 27, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @chancancode (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@palkan palkan changed the title ActionCable channel unit-testing ActionCable channels unit-testing Nov 27, 2016

@palkan palkan force-pushed the palkan:ac-channels-testing branch Nov 27, 2016

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2016

@kaspth
So, I've got rid of explicit identifier construction and removed assertions. I hope it looks much more clear now)

@kaspth

This comment has been minimized.

Copy link
Member

commented Nov 27, 2016

Thanks, I'll take a look tomorrow 😄

actioncable/lib/action_cable/channel/test_case.rb Outdated
def initialize(name)
super "Unable to determine the channel to test from #{name}. " +
"You'll need to specify it using tests YourChannel in your " +
"test case definition"

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward Nov 28, 2016

Member

definition.

actioncable/lib/action_cable/channel/test_case.rb Outdated
end
end

# Superclass for Action Cable channels functional tests.

This comment has been minimized.

Copy link
@vipulnsward
actioncable/lib/action_cable/channel/test_case.rb Outdated
# assert "chat_1", streams.last
# end
#
# def test_subscribed_without_room_number

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward Nov 28, 2016

Member

test_does_not_subscribe_without_room_number

@kaspth
Copy link
Member

left a comment

All right, this is about what I made it to tonight. I'll look more in the coming days.

actioncable/lib/action_cable/channel/test_case.rb Outdated
end

class ConnectionStub
attr_reader :transmissions, :identifiers, :subscriptions

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

Let's stick the broadcasts naming: transmissions -> broadcasts

This comment has been minimized.

Copy link
@palkan

palkan Dec 4, 2016

Author Contributor

There can be a confusion: transmissions list here contains messages transmitted to the socket directly, not through broadcasting.

actioncable/lib/action_cable/channel/test_case.rb Outdated
end

def logger
@logger ||= ::ActiveSupport::Logger.new(StringIO.new)

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

Won't the logger need to support ActiveSupport::TaggedLogging?

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

Or maybe we should reuse new_tagged_logger from the actual Connection.

end

@subscriptions = ActionCable::Connection::Subscriptions.new(self)
@identifiers = identifiers.keys

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

Why can't we reuse this?

def identifiers
subscriptions.keys
end

This comment has been minimized.

Copy link
@palkan

palkan Dec 4, 2016

Author Contributor

These are different identifiers: here we accessing the method argument (within a channel instance).

include ActiveSupport::Testing::ConstantLookup

included do
class_attribute :_channel_class

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

Why prepend the underscore here?

actioncable/lib/action_cable/channel/test_case.rb Outdated
attr_reader :subscription, :connection
delegate :transmissions, to: :connection
delegate :streams, to: :subscription
setup :setup_connection_stub

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

Why do we need this setup? Isn't it more likely users will need to make their own connection_stub with the needed identifiers?

This comment has been minimized.

Copy link
@palkan

palkan Nov 29, 2016

Author Contributor

Agree, the case of connection w/o identifiers is very unlikely.

actioncable/lib/action_cable/channel/test_case.rb Outdated

included do
class_attribute :_channel_class
attr_reader :subscription, :connection

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

new line before

actioncable/lib/action_cable/channel/test_case.rb Outdated
ActiveSupport.run_load_hooks(:action_cable_channel_test_case, self)
end

module ClassMethods

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

Is there precedent for adding this in the Behavior module? I'd rather we just force people to call self.channel_class = :some_channel_class and then move the lookup behavior there.

This comment has been minimized.

Copy link
@palkan

palkan Nov 29, 2016

Author Contributor

I've been using ActionMailer::TestCase and ActionController::TestCase as examples.
So, I've tried to preserve kind of consistency)

The same goes for #27191 (comment) and #27191 (comment) .

channel = determine_constant_from_test_name(name) do |constant|
Class === constant && constant < ActionCable::Channel::Base
end
raise NonInferrableChannelError.new(name) if channel.nil?

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

This isn't something people are likely to call on their own. So why don't we just let the nil through and let it be caught in tests?

def test_stream_with_params
subscribe id: 42

assert_equal "test_42", streams.last

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

@dhh should people have access to the streams here? Or is it better that they test what get's streamed?


identifiers.each do |identifier, val|
define_singleton_method(identifier) { val }
end

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 28, 2016

Member

This seems like it's overly reaching into how identifiers work. (And doing it improperly since it seems to use attr_accessor not singleton methods).

This comment has been minimized.

Copy link
@palkan

palkan Dec 4, 2016

Author Contributor

I don't think we should care here about how identifiers really work. We only want to know that identifier has a value and can be accessed by its name. This is exactly what a channel knows about the connection identifiers.

And this makes our ConnectionStub more abstract. It's just quacking like a connection.

@palkan palkan force-pushed the palkan:ac-channels-testing branch Dec 4, 2016

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2016

@vipulnsward @kaspth Thanks for the reviews! I've updated the PR.

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2016

@kaspth Ping! Anything I can do here to get it merged AFAP? And that one #23211 too, btw.

@Undo1

This comment has been minimized.

Copy link

commented Feb 14, 2017

It'd be awesome to have this implemented. Has there been any progress?

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2017

@Undo1 nothing from my side, waiting for the core team feedback( /cc @kaspth

@palkan palkan closed this Feb 16, 2017

@palkan palkan reopened this Feb 16, 2017

@palkan palkan force-pushed the palkan:ac-channels-testing branch to 9e40836 Jul 6, 2017

@palkan palkan referenced this pull request Oct 24, 2017

Closed

ActionCable testing #23211

@palkan palkan referenced this pull request Aug 19, 2018

Merged

Action cable testing #33659

6 of 6 tasks complete
@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2018

Closed in favour of #33659

@palkan palkan closed this Aug 19, 2018

@palkan palkan deleted the palkan:ac-channels-testing branch Aug 19, 2018

palkan added a commit to palkan/rails that referenced this pull request Sep 24, 2018

Add ActionCable::Channel::TestCase
ActionCable::Channel::TestCase provides an ability
to unit-test channel classes.

There are several reasons to write unit/functional cable tests:
- Access control (who has access to the channel? who can perform action and with which argument?
- Frontend-less applications have no system tests at all–and we still need a way to test channels logic.

See also rails#27191

palkan added a commit to palkan/rails that referenced this pull request Sep 24, 2018

Add ActionCable::Channel::TestCase
ActionCable::Channel::TestCase provides an ability
to unit-test channel classes.

There are several reasons to write unit/functional cable tests:
- Access control (who has access to the channel? who can perform action and with which argument?
- Frontend-less applications have no system tests at all–and we still need a way to test channels logic.

See also rails#27191

jeremy added a commit that referenced this pull request Sep 27, 2018

Add ActionCable::Channel::TestCase
ActionCable::Channel::TestCase provides an ability
to unit-test channel classes.

There are several reasons to write unit/functional cable tests:
- Access control (who has access to the channel? who can perform action and with which argument?
- Frontend-less applications have no system tests at all–and we still need a way to test channels logic.

See also #27191
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.