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

Include broadcastable assertions automatically in ActiveSupport::TestCase #565

Merged

Conversation

swanson
Copy link
Contributor

@swanson swanson commented Feb 2, 2024

There is a note in the Campfire codebase test_helper:

image

And I noticed I had the same patch in my own codebase:

image

In #466 it explicitly mentioned the Broadcastable::TestHelpers are "not automatically included" -- but I'm not sure why. @seanpdoyle any insights?

@swanson
Copy link
Contributor Author

swanson commented Feb 2, 2024

I could also see Turbo::TestAssertions including Turbo::Broadcastable::TestHelpers as well so we just include one module in the railtie if that would be preferable.

@seanpdoyle
Copy link
Contributor

I don't remember specifically, but in retrospect, I think the transitive ActionCable::TestHelper made me hesitant.

Since that isn't included globally, it seemed appropriate to follow suit with these helpers too.

@afcapel
Copy link
Contributor

afcapel commented Feb 8, 2024

Thanks @swanson

I think we can include this change as part of a minor release, as it won't cause any harm to people that were already including the module themselves.

module Greetings
  def greet
    "Hello!"
  end
end

class Person
  include Greetings
  include Greetings # Including the module a second time
end

person = Person.new
puts person.greet # Outputs: "Hello!"

@afcapel afcapel merged commit ef4a624 into hotwired:main Feb 8, 2024
14 of 15 checks passed
@edariedl
Copy link

edariedl commented Feb 9, 2024

@swanson @afcapel Hi, we are using turbo-rails gem in our Rails app but we are not using the actioncable.

With Rails 7.1 upgrade we had to start to including ActionCable into the app, because turbo-rails failed during initialization without it in production environment. Change in this PR is now causing the following error in our tests probably because of a missing configuration (because we don't need one for our app).

Error:
InvoicePaymentTest#test_pay_foreign_invoice:
NoMethodError: undefined method `fetch' for nil
    /Users/edadriedl/.rbenv/versions/3.3.0-rc1/lib/ruby/gems/3.3.0+0/gems/actioncable-7.1.3/lib/action_cable/server/configuration.rb:38:in `pubsub_adapter'
    /Users/edadriedl/.rbenv/versions/3.3.0-rc1/lib/ruby/gems/3.3.0+0/gems/actioncable-7.1.3/lib/action_cable/server/base.rb:86:in `block in pubsub'
    /Users/edadriedl/.rbenv/versions/3.3.0-rc1/lib/ruby/gems/3.3.0+0/gems/actioncable-7.1.3/lib/action_cable/server/base.rb:86:in `synchronize'
    /Users/edadriedl/.rbenv/versions/3.3.0-rc1/lib/ruby/gems/3.3.0+0/gems/actioncable-7.1.3/lib/action_cable/server/base.rb:86:in `pubsub'
    /Users/edadriedl/.rbenv/versions/3.3.0-rc1/lib/ruby/gems/3.3.0+0/gems/actioncable-7.1.3/lib/action_cable/test_helper.rb:10:in `before_setup'
    /Users/edadriedl/.rbenv/versions/3.3.0-rc1/lib/ruby/gems/3.3.0+0/gems/activerecord-7.1.3/lib/active_record/test_fixtures.rb:11:in `before_setup'
    /Users/edadriedl/.rbenv/versions/3.3.0-rc1/lib/ruby/gems/3.3.0+0/gems/activesupport-7.1.3/lib/active_support/testing/setup_and_teardown.rb:40:in `before_setup'

Is there a way to fix it without adding the complete infratructure for action cable into our app? Thank you.

seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this pull request Feb 9, 2024
Closes [hotwired#573][]
Related to [hotwired#565][]

Re-structure the automatic inclusion of the
`Turbo::Broadcastable::TestHelper` module so that it's only
automatically loaded and installed when Action Cable is available to the
runtime.

[hotwired#573]: hotwired#573
[hotwired#565]: hotwired#565 (comment)
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this pull request Feb 9, 2024
Closes [hotwired#573][]
Related to [hotwired#565][]

Re-structure the automatic inclusion of the
`Turbo::Broadcastable::TestHelper` module so that it's only
automatically loaded and installed when Action Cable is available to the
runtime.

[hotwired#573]: hotwired#573
[hotwired#565]: hotwired#565 (comment)
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this pull request Feb 9, 2024
Closes [hotwired#573][]
Related to [hotwired#565][]

Re-structure the automatic inclusion of the
`Turbo::Broadcastable::TestHelper` module so that it's only
automatically loaded and installed when Action Cable is available to the
runtime.

[hotwired#573]: hotwired#573
[hotwired#565]: hotwired#565 (comment)
afcapel pushed a commit that referenced this pull request Feb 9, 2024
Closes [#573][]
Related to [#565][]

Re-structure the automatic inclusion of the
`Turbo::Broadcastable::TestHelper` module so that it's only
automatically loaded and installed when Action Cable is available to the
runtime.

[#573]: #573
[#565]: #565 (comment)
luisjose1996 added a commit to luisjose1996/Turbo-Rails that referenced this pull request May 10, 2024
Closes [#573][]
Related to [#565][]

Re-structure the automatic inclusion of the
`Turbo::Broadcastable::TestHelper` module so that it's only
automatically loaded and installed when Action Cable is available to the
runtime.

[#573]: hotwired/turbo-rails#573
[#565]: hotwired/turbo-rails#565 (comment)
@swanson swanson deleted the matt/include-broadcastable-test-helpers branch August 13, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants