Skip to content

Fix rubocop issue for action cable -- ostruct#47409

Merged
rafaelfranca merged 1 commit intorails:mainfrom
hahmed:ha/move-test-server-to-fix-faiing-test
Mar 3, 2023
Merged

Fix rubocop issue for action cable -- ostruct#47409
rafaelfranca merged 1 commit intorails:mainfrom
hahmed:ha/move-test-server-to-fix-faiing-test

Conversation

@hahmed
Copy link
Copy Markdown
Contributor

@hahmed hahmed commented Feb 15, 2023 •
edited
Loading

Motivation / Background

Action cable is breaking when creating a simple test like below, because of an error NameError: uninitialized constant ActionCable::Channel::ConnectionStub::TestServer. The test_server is now inside the lib folder, alongside a the success_adapter which it depends on.

require "test_helper"

class RandomChannelTest < ActionCable::Channel::TestCase
  test "subscribes" do
    subscribe
    assert subscription.confirmed?
  end
end

This Pull Request has been created because #47377 which relates to a change merged in #47300

The second commit addresses the rubocop performance issue where I added a fake config class that inherits from ActionCable::Server::Configuration. The subscription_adapter is added so it can be overridden in tests, this seems like a a better option than using a struct in which a whole bunch of vars need to be assigned/overridden.

Detail

Fix rubocop issue with using ostruct.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@hahmed hahmed force-pushed the ha/move-test-server-to-fix-faiing-test branch 2 times, most recently from 3e35a8a to 0e4e498 Compare February 15, 2023 21:27
@zzak
Copy link
Copy Markdown
Member

zzak commented Feb 16, 2023

@hahmed Can you look at fixing the rubocop failures?

actioncable/lib/action_cable/test_server.rb:15:28: C: Performance/OpenStruct: Consider using Struct over OpenStruct to optimize the performance.
[12](https://github.com/rails/rails/actions/runs/4188289466/jobs/7259226717#step:4:13)
      @config = OpenStruct.new(log_tags: [], subscription_adapter: subscription_adapter, filter_parameters: [])
[13](https://github.com/rails/rails/actions/runs/4188289466/jobs/7259226717#step:4:14)
                           ^^^
[14](https://github.com/rails/rails/actions/runs/4188289466/jobs/7259226717#step:4:15)

@zzak zzak linked an issue Feb 16, 2023 that may be closed by this pull request
Using ActionCable::Channel::TestCase causes NameError: TestServer is missing #47377
Closed
@hahmed hahmed changed the title Fixing an issue with action cable Fixing an issue with action cable not able to run tests Feb 16, 2023
@hahmed hahmed changed the title Fixing an issue with action cable not able to run tests Fixing issue with action cable not able to run tests Feb 16, 2023
@hahmed hahmed changed the title Fixing issue with action cable not able to run tests Fixes issue with action cable not able to run tests Feb 16, 2023
@st0012 st0012 mentioned this pull request Feb 18, 2023
Merged
@hahmed hahmed force-pushed the ha/move-test-server-to-fix-faiing-test branch 2 times, most recently from bd397ee to b3546f0 Compare February 20, 2023 22:14
zzak
zzak reviewed Feb 21, 2023
View reviewed changes
zzak
zzak reviewed Feb 21, 2023
View reviewed changes
@hahmed hahmed force-pushed the ha/move-test-server-to-fix-faiing-test branch 2 times, most recently from 1a026e3 to f696dd7 Compare February 22, 2023 13:56
@palkan palkan mentioned this pull request Feb 23, 2023
Merged
4 tasks
@palkan
Copy link
Copy Markdown
Contributor

palkan commented Feb 23, 2023

The test_server is now inside the lib folder, alongside a the success_adapter which it depends on.

I don't like the idea of exposing library internals just for the sake of fixing an accidentally introduce regression. SuccessAdapter doesn't make any sense for applications as well as TestServer. Channel::TestCase is meant for isolated testing, no server should be involved at all.

Here is an alternative fix: #47483

@hahmed
Add a fake configuration with the ability to override the
627322e 
subscription_adapter, so we no longer have to use openstruct.
@hahmed hahmed force-pushed the ha/move-test-server-to-fix-faiing-test branch from f696dd7 to 627322e Compare February 23, 2023 23:24
@hahmed hahmed changed the title Fixes issue with action cable not able to run tests Fix rubocop issue for action cable -- ostruct Feb 23, 2023
zzak
zzak reviewed Mar 3, 2023
View reviewed changes
actioncable/test/stubs/test_server.rb
@@ -1,18 +1,28 @@
# frozen_string_literal: true

require "ostruct"
Copy link
Copy Markdown
Member

@zzak zzak Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but I do like removing ostruct 👍

@rafaelfranca rafaelfranca merged commit 37bd59a into rails:main Mar 3, 2023
@hahmed hahmed deleted the ha/move-test-server-to-fix-faiing-test branch March 4, 2023 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zzak zzak zzak left review comments

Assignees

No one assigned

Labels

actioncable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using ActionCable::Channel::TestCase causes NameError: TestServer is missing

4 participants

@hahmed @zzak @palkan @rafaelfranca