palkan
Errors with Anycable check
When running specs the AnyCable::Compatibility checks are raising an error for setting @_streams (Channel instance variables are not supported by AnyCable, but were set: @_streams).
The adapter is set to test as suggested in the readme, but the compatibility checks are loaded via an initializer, so they get applied (and should be as per the documentation). I am not sure if there is something missing/wrong in our setup, but i think this is a problem in either anycable (not knowing about testing instance variable(s)) or action-cable-testing (using a workaround for tracking streams that violate anycable expectations).
Both seem to be weak arguments, so i am not sure which one is the better path, or if there are other (better) alternatives.
Just to be clear, by anycable i rather mean anycable-rails as that is where these compatibility checks live.
On second thought maybe this (anycable-rails) is the best place to add the conditions, e.g. something like:
unless diff.empty? || (::Rails.env.test? && diff == ['@_streams'])
Obviously this has the extra burden of keeping it up to date with whatever action-cable-testing (or others) will introduce, but i feel anycable-rails already knows about rails, so it's the right place (given this gem is going to be merged into rails proper it will be somewhat part of the api). If you agree with that approach i am happy to create a PR.
Thanks for the report!
anycable-rails already knows about rails, so it's the right place
Yep, that should be handled by anycable-rails.
One of the options could be is to add a whitelist of instance variables (and add @_streams by default if ActionCable::Channel::TestCase is loaded).
yes good point, if you agree i can create a PR sometimes tomorrow on anycable-rails. do you prefer creating an issue on anycable-rails also?
if you agree i can create a PR sometimes tomorrow on anycable-rails
Sounds good!
do you prefer creating an issue on anycable-rails also?
No, no need.
Trying to understand what is the best way to check if the testing part is applied, i am not sure if i understood you correctly, initially i thought we could go with something like defined? ActionCable::Channel::TestCase (and have an ALLOWED_INSTANCE_VARIABLES in Compatibility), but i've realised it does not ensure connection/channel stubbing (as we don't know in Channel::Base - where the Compatibility patching is - if we are "running inside" such a test case or not).
I could test for self.singleton_class.include?(ActionCable::Channel::ChannelStub), but that would run on every invocation.
If the action-cable-testing will be merged into rails the first test (checking defined?) will always be true (i think), so not sure if it is meaningful. Allowing @_streams unconditionally does introduce an edge-case where people could use that instance variable name ("accidentally") for other purposes (it would lead to problems/failures in test results, but wouldn't raise an error while testing or development).
That is why i think maybe we should try to exclude @_streams only when we know for sure it is defined by the stubbing (so it would still not raise error while testing, but would raise in development - for when @_streams is used explicitly by the developer).
Maybe using a different instance variable name like @_stubbed_streams would lower the name clash risk so that it can be used unconditionally (and even documented in testing guide so users do not accidentally use it).
Edit: another possibility is if we add some extra method to ChannelStub like stubbed_channel? that we can test for in Compatibility (it would be on every invocation like with singleton_class, or it could be a class method).