ActionCable channels unit-testing #27191

palkan · 2016-11-27T20:31:52Z

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 · 2016-11-27T20:31:54Z

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 · 2016-11-27T20:35:02Z

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

kaspth · 2016-11-27T20:45:22Z

Thanks, I'll take a look tomorrow 😄

vipulnsward · 2016-11-28T05:50:55Z

actioncable/lib/action_cable/channel/test_case.rb

+ 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"


definition.

vipulnsward · 2016-11-28T07:45:00Z

actioncable/lib/action_cable/channel/test_case.rb

+ end
+ end
+
+ # Superclass for Action Cable channels functional tests.


vipulnsward · 2016-11-28T07:46:04Z

actioncable/lib/action_cable/channel/test_case.rb

+ # assert "chat_1", streams.last
+ # end
+ #
+ # def test_subscribed_without_room_number


test_does_not_subscribe_without_room_number

kaspth

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

kaspth · 2016-11-28T19:45:49Z

actioncable/lib/action_cable/channel/test_case.rb

+ end
+
+ class ConnectionStub
+ attr_reader :transmissions, :identifiers, :subscriptions


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

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

kaspth · 2016-11-28T19:47:00Z

actioncable/lib/action_cable/channel/test_case.rb

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


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

Or maybe we should reuse new_tagged_logger from the actual Connection.

kaspth · 2016-11-28T19:51:57Z

actioncable/lib/action_cable/channel/test_case.rb

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


Why can't we reuse this?

rails/actioncable/lib/action_cable/connection/subscriptions.rb

Lines 56 to 58 in 06d2049

def identifiers

subscriptions.keys

end

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

kaspth · 2016-11-28T19:55:32Z

actioncable/lib/action_cable/channel/test_case.rb

+ include ActiveSupport::Testing::ConstantLookup
+
+ included do
+ class_attribute :_channel_class


Why prepend the underscore here?

kaspth · 2016-11-28T19:56:14Z

actioncable/lib/action_cable/channel/test_case.rb

+ attr_reader :subscription, :connection
+ delegate :transmissions, to: :connection
+ delegate :streams, to: :subscription
+ setup :setup_connection_stub


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

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

kaspth · 2016-11-28T19:56:47Z

actioncable/lib/action_cable/channel/test_case.rb

+
+ included do
+ class_attribute :_channel_class
+ attr_reader :subscription, :connection


new line before

kaspth · 2016-11-28T19:59:21Z

actioncable/lib/action_cable/channel/test_case.rb

+ ActiveSupport.run_load_hooks(:action_cable_channel_test_case, self)
+ end
+
+ module ClassMethods


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.

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) .

kaspth · 2016-11-28T20:01:24Z

actioncable/lib/action_cable/channel/test_case.rb

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


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?

kaspth · 2016-11-28T20:04:38Z

actioncable/test/channel/test_case_test.rb

+ def test_stream_with_params
+ subscribe id: 42
+
+ assert_equal "test_42", streams.last


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

kaspth · 2016-11-28T20:06:11Z

actioncable/lib/action_cable/channel/test_case.rb

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


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).

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 · 2016-12-04T14:53:41Z

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

palkan · 2016-12-20T13:57:08Z

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

Undo1 · 2017-02-14T00:32:51Z

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

palkan · 2017-02-16T08:34:58Z

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

palkan · 2018-08-19T23:35:51Z

Closed in favour of #33659

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

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

rails-bot assigned chancancode Nov 27, 2016

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

palkan force-pushed the ac-channels-testing branch from c4a2df1 to 16120f7 Compare November 27, 2016 20:33

maclover7 added the actioncable label Nov 27, 2016

vipulnsward reviewed Nov 28, 2016

View reviewed changes

kaspth reviewed Nov 28, 2016

View reviewed changes

palkan force-pushed the ac-channels-testing branch from 16120f7 to 9ff9f06 Compare December 4, 2016 14:53

palkan mentioned this pull request Jan 11, 2017

Decouple ActionCable application logic from low-level logic #27648

Closed

palkan closed this Feb 16, 2017

palkan reopened this Feb 16, 2017

maclover7 added the needs feedback label Feb 16, 2017

Add ActionCable::Channel::TestCase

9e40836

palkan force-pushed the ac-channels-testing branch from 9ff9f06 to 9e40836 Compare July 6, 2017 14:39

palkan mentioned this pull request Oct 24, 2017

ActionCable testing #23211

Closed

palkan mentioned this pull request Aug 19, 2018

Action cable testing #33659

Merged

6 tasks

palkan closed this Aug 19, 2018

palkan deleted the ac-channels-testing branch August 19, 2018 23:36

palkan mentioned this pull request Sep 24, 2018

Add ActionCable::Channel::TestCase #33969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ActionCable channels unit-testing #27191

ActionCable channels unit-testing #27191

palkan commented Nov 27, 2016

rails-bot commented Nov 27, 2016

palkan commented Nov 27, 2016

kaspth commented Nov 27, 2016

vipulnsward Nov 28, 2016

vipulnsward Nov 28, 2016

vipulnsward Nov 28, 2016

kaspth left a comment

kaspth Nov 28, 2016

palkan Dec 4, 2016

kaspth Nov 28, 2016

kaspth Nov 28, 2016

kaspth Nov 28, 2016

palkan Dec 4, 2016

kaspth Nov 28, 2016

kaspth Nov 28, 2016

palkan Nov 29, 2016

kaspth Nov 28, 2016

kaspth Nov 28, 2016

palkan Nov 29, 2016

kaspth Nov 28, 2016

kaspth Nov 28, 2016

kaspth Nov 28, 2016

palkan Dec 4, 2016

palkan commented Dec 4, 2016

palkan commented Dec 20, 2016

Undo1 commented Feb 14, 2017

palkan commented Feb 16, 2017

palkan commented Aug 19, 2018

ActionCable channels unit-testing #27191

ActionCable channels unit-testing #27191

Conversation

palkan commented Nov 27, 2016

rails-bot commented Nov 27, 2016

palkan commented Nov 27, 2016

kaspth commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspth 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palkan commented Dec 4, 2016

palkan commented Dec 20, 2016

Undo1 commented Feb 14, 2017

palkan commented Feb 16, 2017

palkan commented Aug 19, 2018