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 testing #23211

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@palkan
Copy link
Contributor

commented Jan 23, 2016

I've started to play with ActionCable and realized that we don't have any tools for Cable testing.
Here is a kind of scratch of what (as I think) Cable testing could be.

This test helper only deals with transmissions (just like ActiveJob's one). Example:

test 'message broadcasting block style' do
  assert_transmissions(1, "messages:#{@hello_msg.id}:comments") do
    CommentRelayJob.perform_now(@hello_comment)
  end
end

More examples can be found here.

And there are some questions to discuss:

  • How to unit test channels?
  • Do we need such testing at all?
@rails-bot

This comment has been minimized.

Copy link

commented Jan 23, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (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.

Please see the contribution instructions for more information.

@maclover7

This comment has been minimized.

Copy link
Member

commented Jan 23, 2016

Thanks for submitting this @palkan - this looks really great! I think asserting that data is being sent is the main thing that should be provided as a test helper (as you have implemented). Not sure what else we want to allow users to test... cc @dhh

@eileencodes

View changes

actioncable/lib/action_cable/subscription_adapter/test.rb Outdated

def channels_data
@channels_data ||= {}
end

This comment has been minimized.

Copy link
@eileencodes

eileencodes Jan 23, 2016

Member

Can you remove the space under private and then indent private methods like you did in the other file?

This comment has been minimized.

Copy link
@palkan

palkan Jan 23, 2016

Author Contributor

Sure

@maclover7

View changes

actioncable/lib/action_cable/test_helper.rb Outdated
assert_includes new_messages, serialized_msg, "No messages sent with #{data} to #{channel}"
end

def cable_adapter

This comment has been minimized.

Copy link
@maclover7

maclover7 Jan 23, 2016

Member

Can we call this pubsub_adapter like we do in other parts of ActionCable?

@maclover7

View changes

actioncable/lib/action_cable/test_helper.rb Outdated
end

def assert_transmissions(number, channel)
if block_given?

This comment has been minimized.

Copy link
@maclover7

maclover7 Jan 23, 2016

Member

This is checking for a block, but it's not passed in as a parameter...

This comment has been minimized.

Copy link
@palkan

palkan Jan 23, 2016

Author Contributor

Why do we need block arg here?

This comment has been minimized.

Copy link
@maclover7

maclover7 Jan 23, 2016

Member

Because you are checking for it in this line...? Could be wrong here.

This comment has been minimized.

Copy link
@palkan

palkan Jan 23, 2016

Author Contributor

No, we don't need it here, it's checked implicitly. This is how block_given? works.

@maclover7

View changes

actioncable/lib/action_cable/test_helper.rb Outdated
def assert_transmited_data(data, channel)
serialized_msg = ActiveSupport::JSON.encode(data)
new_messages = transmissions(channel)
if block_given?

This comment has been minimized.

Copy link
@maclover7

maclover7 Jan 23, 2016

Member

This is checking for a block, but it's not passed in as a parameter...

@palkan palkan force-pushed the palkan:action-cable-testing branch Jan 23, 2016

@maclover7

View changes

actioncable/lib/action_cable/subscription_adapter/test.rb Outdated
module ActionCable
module SubscriptionAdapter
# == Test adapter for Action Cable
class Test < Base

This comment has been minimized.

Copy link
@maclover7

maclover7 Jan 23, 2016

Member

Can we add a :nodoc: and also a note saying to never use this in any environment except through tests?

This comment has been minimized.

Copy link
@palkan

palkan Jan 23, 2016

Author Contributor

Yes, of course. I'll add some docs to this class and to the helper after we decide that everything is ok.

@palkan palkan force-pushed the palkan:action-cable-testing branch 2 times, most recently Jan 23, 2016

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2016

TestHelper tests added.

@maclover7

This comment has been minimized.

Copy link
Member

commented Jan 23, 2016

👍 Thank you for you work on this!

@dhh

View changes

actioncable/lib/action_cable/test_helper.rb Outdated
# ActionCable.server.broadcast 'messages', { text: 'hello' }
# end
# end
def assert_transmited_data(data, channel)

This comment has been minimized.

Copy link
@dhh

dhh Jan 24, 2016

Member

Let's flip these parameters and go with assert_transmission_on channel, data so we don't have to wrap the hash.

@dhh

View changes

actioncable/lib/action_cable/test_helper.rb Outdated
# end
def assert_transmited_data(data, channel)
serialized_msg = ActiveSupport::JSON.encode(data)
new_messages = transmissions(channel)

This comment has been minimized.

Copy link
@dhh

dhh Jan 24, 2016

Member

Add CR.


# Restore all sent messages
(old_messages + new_messages).each { |m| pubsub_adapter.broadcast(channel, m) }
end

This comment has been minimized.

Copy link
@dhh

dhh Jan 24, 2016

Member

Add CR.

@dhh

View changes

actioncable/lib/action_cable/test_helper.rb Outdated
ActionCable.server.pubsub
end

delegate :transmissions,

This comment has been minimized.

Copy link
@dhh

dhh Jan 24, 2016

Member

Single line these.

def after_teardown # :nodoc:
super
ActionCable.server.instance_variable_set(:@pubsub, @old_pubsub_adapter)
end

This comment has been minimized.

Copy link
@dhh

dhh Jan 24, 2016

Member

Why is this setup/teardown needed when we're setting the test adapter in cable.yml?

This comment has been minimized.

Copy link
@palkan

palkan Jan 25, 2016

Author Contributor

First of all, to use fresh server for every example. Of course, we can just use kind of ActionCable.server.reset! in before_setup.

Secondly, to make sure that we use test adapter and to allow users to use their preferred adapter in other test cases (e.g. acceptance testing).
Maybe it's better to provide some convenient helpers to switch between adapters and use "test" by default (from cable.yml).

@dhh

View changes

actioncable/lib/action_cable/test_helper.rb Outdated
# ActionCable.server.broadcast 'messages', { text: 'how are you?' }
# end
# end
def assert_transmissions(number, channel)

This comment has been minimized.

Copy link
@dhh

dhh Jan 24, 2016

Member

I like how tranmission is continuing to play with our metaphor, but I actually think it's probably simpler to just stick with broadcasts. Then there's parity between what you call and what you test. The broadcast->transmission part is too subtle imo.

So how about assert_broadcasts_on channel, number

This comment has been minimized.

Copy link
@dhh

dhh Jan 24, 2016

Member

(obviously this would be a rename throughout this, transmission->broadcast everywhere).

This comment has been minimized.

Copy link
@palkan

palkan Jan 25, 2016

Author Contributor

My first idea was to use word "messages" (because message is what we really send through the cable). But I took "transmissions" from existing tests.

So, the other candidate is "message". "Broadcast" or "message"?

This comment has been minimized.

Copy link
@kaspth

kaspth Jan 25, 2016

Member

Broadcasts because that's the method you're calling on the server :)

@palkan palkan force-pushed the palkan:action-cable-testing branch 2 times, most recently Jan 25, 2016

@palkan palkan force-pushed the palkan:action-cable-testing branch Feb 16, 2016

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2016

Rebase with the current master (due to Cable configuration changes).
Anything else?

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Feb 17, 2016

@rails-bot rails-bot assigned matthewd and unassigned eileencodes Feb 17, 2016

@maclover7

This comment has been minimized.

Copy link
Member

commented Apr 24, 2016

@palkan Would you be willing to rebase this, and resolve the remaining conflicts? Getting good Action Cable testing infrastructure setup for 5.0 (when Action Cable officially launches) is very important :)

@@ -0,0 +1,124 @@
module ActionCable
# Provides helper methods for testing Action Cable broadcasting
module TestHelper

This comment has been minimized.

Copy link
@maclover7

maclover7 Apr 24, 2016

Member

Can we modify this to create an ActionCable::TestCase, similar to how Action Mailer has ActionMailer::TestCase, Active Job ActiveJob::TestCase, etc.?

@jeremy jeremy added this to the 5.0.0 milestone Apr 24, 2016

@kaspth

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

@palkan unfortunately no, the betas are our feature freeze point.

Additionally, I'm going to have to bow out of reviewing these PRs I'm too exhausted at the moment to give this the attention it needs. Let's revisit once 5.2 is completely out the door.

Your dedication to this is admirable and appreciated 👏

Add ActionCable testing utils
- Add Action Cable test adapter and test helper
- Add ActionCable::TestCase

@palkan palkan force-pushed the palkan:action-cable-testing branch to 7b09f2d Jul 6, 2017

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2017

Hi, @kaspth! Maybe it's time to re-visit and prepare for 5.2?

@kaspth

This comment has been minimized.

Copy link
Member

commented Jul 6, 2017

Hey! Unfortunately I'm already booked up for 5.2 with a GSoC project and improving things I've directly contributed to 😊

@Undo1

This comment has been minimized.

Copy link

commented Aug 26, 2017

This is a highly important issue. Is anyone else able to push this forward? This is a pretty big part of an application to be untestable.

@kesha-antonov

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2017

Agreed. Need this for testing too 👍

@dhh

This comment has been minimized.

Copy link
Member

commented Aug 26, 2017

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2017

Extracted into a separate gem – https://github.com/palkan/action-cable-testing.

@dhh @kaspth I'm not closing this one and #27191, 'cause I hope to get them reviewed+merged one day.

@Preen

This comment has been minimized.

Copy link

commented Oct 31, 2017

@palkan Great work. Thanks.

@kaspth

This comment has been minimized.

Copy link
Member

commented Nov 12, 2017

Sounds good. Let's keep it in the gem for now ✌️

@Schwad

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2017

@kaspth @dhh @rafaelfranca this PR was whipped together in January of 16 and it has been in a nearly 'ready-to-roll' state it seems since Q2 of that year.

Going back and reading through the work that @palkan has put into this there are nine separate instances where requests were made by members of the team that were then immediately implemented without argument or complaint.

It was only after the better part of two years of this being open that he then put this into a gem.

As someone who is passionate about open source contributions and the community, tell me what I can do to help get this PR on to some sort of timeline. Can we give it a flag or label to make it a priority for the 5.3 milestone queue? 😃

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2017

Very good points, @Schwad!

I've already tried to find the answers. With no luck.

I would dare to say that Action Cable has no maintainers.

Maybe, "maintainer" is not the right word, 'cause, hopefully, we still have deps updates, small bug fixes and docs updates–some kind of maintenance. But we don't have any new functionality (even such crucial as testing and a lot more, see PRs and issues).

I had to extract two PRs into a gem. What should I do next time I want to improve Action Cable, propose a PR or write yet another gem? I favor the latter one now.

P.S. OSS that stops evolution gradually dies (Matz).

@matthewd

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

Maybe the gem will have helped with this: have people actually used this in real applications and found it useful? As @dhh has noted, it's solving a problem Basecamp isn't experiencing.

Does anyone have any representative real-world examples of how their tests look when using it?

Anyway, this seems to be a far less intimidating pile of code than it was last time I saw it. I'll try to have a look -- though the gem seemed like a perfectly fine idea to me.

But we don't have any new [Action Cable] functionality

Yep. It's doing its job, and people are spending their limited volunteer time elsewhere. Sorry 🤷🏻‍♂️

@matthewd matthewd assigned matthewd and unassigned jeremy Nov 16, 2017

@Preen

This comment has been minimized.

Copy link

commented Nov 16, 2017

Yep. It's doing its job, and people are spending their limited volunteer time elsewhere. Sorry

Not @palkan. So dont feel sorry. Just feel sorry its not been merged into Rails. :)

@Schwad

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

Thank you so much for being willing to have a look @matthewd :) If there's anything I can do to help on this please let me know.

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

First of all, thank you, @matthewd!

Does anyone have any representative real-world examples of how their tests look when using it?

A few examples from the wild: one, and two, and three.

There are several reasons to write unit/functional cable tests:

  • Access control (who has access to the channel? who can perform action and with what arguments? who can connect to the server?)–similar to controller tests
  • Frontend-less applications–no other way to test channels, because no system tests at all

So, system tests (the official testing technique for cable) don't solve all the problems. And also, due to the well-known reasons (slow, hard to maintain, etc.), not everyone writes system tests.

I'll try to have a look -- though the gem seemed like a perfectly fine idea to me.

I agree–having this functionality in a separate gem makes sense. It's much more easier to react on bugs/feature requests and so on. And as long as Action Cable itself is not evolving, there should not be any compatibility issues.

I'd like to propose adding the information about the gem to the guides and, maybe, even include it to the default Gemfile. @matthewd WDYT?

it's solving a problem Basecamp isn't experiencing.

Looks like Basecamp is the only Rails application. Oh, there are also Shopify and Github)

Also, "not experiencing" != "doesn't exist and impossible".

It's doing its job

Unfortunately, IMO it's not doing it well. It could be much better. I've already mentioned some problems here (e.g. #26999), I'm speaking about Action Cable and its problems from time to time (see here, for example) and I have a lot of ideas on how to improve it but... "It's doing its job". I've heard this before.

Nevertheless, I'm still here and willing to help.

@namiwang

This comment has been minimized.

Copy link

commented Aug 15, 2018

And it's Aug 2018 now.

We're using api-only-mode and actioncable to build a pure websocket server, so system tests are out of the picture, and now we're facing some untestable features.

Maybe it's time to revisit the issue?

@dhh

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

I'd like to see this get merged for Rails 6.0. Just read through the whole thing, and it seems like the only slight nit left is adding a base class for cable tests, like we have for Action Job, like:

module ActionCable
  class TestCase < ActiveSupport::TestCase
    include ActionCable::TestHelper

    ActiveSupport.run_load_hooks(:active_job_test_case, self)
  end
end

Do you have any other objections, @kaspth?

@dhh

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

Ah, I see that #27191 has the base class. That PR has just a few outstanding comments on it. @palkan if you're not burned out over how long this has dragged on, would you mind taking a look? I think we're almost at the finish line here. For realz 😄

And thanks again for your diligent and persistent work on this.

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

@dhh I'd be glad to finish this feature and finally get it merged)

I'd like to start a new PR for that, 'cause since the action-cable-testing gem has been published, I've made a bunch of fixes and a few more features, so the existing PRs are out of sync.

So, that's gonna be a huge PR – what is the best way to do that from the reviewers point of view? One parent PR and a separate PR for each smaller feature (there gonna 3-4 of them, I think)? Or a single PR with multiple commits reflecting different features? WDYT @matthewd @kaspth

@dhh

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

@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:action-cable-testing branch Aug 19, 2018

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.