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

Action Cable: JavaScript test updates #27123

Merged
merged 2 commits into from Nov 21, 2016
Merged

Conversation

@javan
Copy link
Member

@javan javan commented Nov 20, 2016

Copy link
Member

@maclover7 maclover7 left a comment

Overall big 👍 👍 to adding more tests for Action Cable's JS! If possible, can you make each of the test's names more descriptive? While I was reading through, it was a little tough to determine what each of the tests was doing 😬

actioncable/test/javascript/src/unit/subscription_test.coffee Outdated

broadcast(message_type: "confirmation")


This comment has been minimized.

@maclover7

maclover7 Nov 20, 2016
Member

too many empty lines here? 😬

actioncable/test/javascript/src/unit/subscription_test.coffee Outdated
broadcast(message_type: "confirmation")


broadcast = (data = {}, callback) ->

This comment has been minimized.

@maclover7

maclover7 Nov 20, 2016
Member

Might be a bit early, but should we extract the broadcast function out to where the rest of the "helper" functions are?

@maclover7 maclover7 self-assigned this Nov 20, 2016
@javan
Copy link
Member Author

@javan javan commented Nov 21, 2016

@maclover7 done and done

@javan javan force-pushed the javan:actioncable/js-test-updates branch to 7083fa2 Nov 21, 2016
@kaspth kaspth merged commit bb2a832 into rails:master Nov 21, 2016
2 checks passed
2 checks passed
codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth
Copy link
Member

@kaspth kaspth commented Nov 21, 2016

Nice!

@javan javan deleted the javan:actioncable/js-test-updates branch Nov 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants