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

Errors with Anycable check #53

Open
eLod opened this issue May 9, 2019 · 6 comments
Open

Errors with Anycable check #53

eLod opened this issue May 9, 2019 · 6 comments

Comments

@eLod
Copy link

eLod commented May 9, 2019

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.

@eLod
Copy link
Author

eLod commented May 9, 2019

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.

@palkan
Copy link
Owner

palkan commented May 9, 2019

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

@eLod
Copy link
Author

eLod commented May 9, 2019

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?

@palkan
Copy link
Owner

palkan commented May 9, 2019

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.

@eLod
Copy link
Author

eLod commented May 10, 2019

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

@eLod
Copy link
Author

eLod commented May 10, 2019

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

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

No branches or pull requests

2 participants