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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Action Cable testing #2113

Merged
merged 3 commits into from May 15, 2019
Merged

Add Action Cable testing #2113

merged 3 commits into from May 15, 2019

Conversation

palkan
Copy link
Contributor

@palkan palkan commented Apr 26, 2019 •
edited

This PR migrates action-cable-testing gem RSpec functionality into rspec-rails (since core functionality has been merged into Rails 6).

You can check the existing gem documentation (RSpec-like) to see what's coming: https://relishapp.com/palkan/action-cable-testing/docs.

What's inside:

Closes #1606

@palkan palkan marked this pull request as ready for review April 26, 2019 22:20
@palkan palkan changed the title Add Action Cable testing Add Action Cable testing [WIP] Apr 26, 2019
@palkan palkan changed the title Add Action Cable testing [WIP] Add Action Cable testing Apr 28, 2019
@palkan
Copy link
Contributor Author

palkan commented Apr 28, 2019

@benoittgt @samphippen Hey folks, what should I do to make Travis show me the failures?)

@benoittgt
Copy link
Member

Hello @palkan

Sorry busy weekend. Maybe you can comment those lines?
https://github.com/rspec/rspec-rails/blob/master/.travis.yml#L210-L213

I will have a look on your PR tomorrow. Thanks for the hard work.

@fables-tales
Copy link
Member

@palkan please rebase this on top of the 4-0-dev branch, as this'll have to make it in to 4-0 at this point.

@palkan
Copy link
Contributor Author

palkan commented Apr 28, 2019

@samphippen

please rebase this on top of the 4-0-dev branch

Done!

benoittgt
benoittgt reviewed Apr 29, 2019
View reviewed changes
Copy link
Member

@benoittgt benoittgt left a comment

Choose a reason for hiding this comment

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

Very good job. I have few questions related to documentation but also where matcher are defined.

features/matchers/have_broadcasted_matcher.feature Show resolved Hide resolved
@palkan
Copy link
Contributor Author

palkan commented May 1, 2019

@benoittgt @JonRowe thanks for the feedback! All fixed.

@samphippen The build seems green: https://travis-ci.org/rspec/rspec-rails/builds/526676636 (Rails 4.2 specs are failing for non-related to this PR reasons, looks it's no longer supported?).
Also, to make the build pass the RuboCop check I had to freeze the version (~> 0.67.0), the new one breaks it (yeah, as always). That should be fixed in the base branch, I think.

@JonRowe
Copy link
Member

JonRowe commented May 3, 2019

@samphippen can you give me a final review for this, you're more familiar with action cable!

fables-tales
fables-tales requested changes May 12, 2019
View reviewed changes
Copy link
Member

@fables-tales fables-tales left a comment

Choose a reason for hiding this comment

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

This is overall a high quality PR, I had a few nits, but if you can address them, I'll merge this down!

lib/rspec/rails/example/channel_example_group.rb Outdated
end
end

if ::Rails::VERSION::MAJOR > 5
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about >= 6?

lib/rspec/rails/matchers/action_cable.rb Outdated
# expect {
# ActionCable.server.broadcast "messages", text: 'Hi!'
# }.to have_broadcasted_to("messages").with(text: 'Hi!')

Copy link
Member

Choose a reason for hiding this comment

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

you need to kill this blankline.

@fables-tales
Copy link
Member

@palkan BTW you should feel free to DM me on twitter if you need my attention, it is by far and away my fastest response time channel.

@fables-tales
Copy link
Member

LGTM, will merge on green!

@palkan
Copy link
Contributor Author

palkan commented May 14, 2019 •
edited

Looks like we're blocked by RuboCop again (0.69 release notes)

image

@benoittgt @samphippen Is anyone going to fix this in 4-0-dev?

@benoittgt
Copy link
Member

@palkan We merged #2126 this afternoon sorry. It should be good. 馃槉

@palkan
Copy link
Contributor Author

palkan commented May 14, 2019

@benoittgt My bad, I should have checked(

Anyway, we're green 馃帀

@benoittgt benoittgt merged commit ac64a6b into rspec:4-0-dev May 15, 2019
@benoittgt
Copy link
Member

Thanks a lot @palkan. Awesome job! 馃憦

@palkan palkan mentioned this pull request May 15, 2019
Merged
@palkan palkan deleted the feat/action-cable-testing branch May 15, 2019 15:36
@benoittgt benoittgt mentioned this pull request May 17, 2019
Merged
benoittgt added a commit that referenced this pull request May 18, 2019
@benoittgt
Changelog for #2119, #2113, #2125
0b877c5
benoittgt added a commit that referenced this pull request Aug 21, 2019
@benoittgt
Changelog for #2119, #2113, #2125
df4ce36
benoittgt added a commit to benoittgt/rspec-rails that referenced this pull request Aug 24, 2019
@benoittgt
Changelog for rspec#2119, rspec#2113, rspec#2125
8b5f08d
@pirj pirj mentioned this pull request Aug 21, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benoittgt benoittgt benoittgt left review comments

@fables-tales fables-tales fables-tales approved these changes

@JonRowe JonRowe JonRowe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@palkan @benoittgt @fables-tales @JonRowe