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

Updates the crystal version restriction #22

Merged
merged 5 commits into from
May 7, 2021

Conversation

jwoertink
Copy link
Collaborator

I ran the specs locally against Crystal 1.0, and they pass:

cable on  master [⇡]
❯ ~/Development/crystal/crystal-1.0.0-1/bin/crystal spec
..............................

Finished in 128.44 milliseconds
30 examples, 0 failures, 0 errors, 0 pending

I'm not sure how to add multiple crystal versions to Travis CI, but by updating this crystal version restriction, it can be resolved with other shards between 0.35 and 1.0.

@fernandes
Copy link
Collaborator

@jwoertink thank you very much!! 😍

on travis it errored, complaining about schedule shard version, should we --ignore-crystal-version for a while?

@jwoertink
Copy link
Collaborator Author

We could give that a try. I'm not familiar with that shard. Another option could be to go with https://github.com/spider-gazelle/tasker which allows you to do Tasker.every(2.milliseconds) { perform_action }. Essentially the same thing, but just updated recently to support crystal 1.0. I'm open to either option. Up to you.

@jwoertink
Copy link
Collaborator Author

I don't know how to tell Travis to ignore the crystal version. I also see that the Schedule shard still isn't updated. I opened up an issue, but I'm not sure how soon that will be updated.

@jwoertink
Copy link
Collaborator Author

I opened up an issue on the Schedule shard hugoabonizio/schedule.cr#19

@fernandes
Copy link
Collaborator

We are using Schedule only on https://github.com/cable-cr/cable/blob/master/src/cable/websocket_pinger.cr, we can change to tasker for sure.. choosing schedule was an arbitrary decision to have this task in parallel

so yes, let's move to tasker and make it 1.0 ready, thanks for pushing this @jwoertink ! 🙇

@jwoertink
Copy link
Collaborator Author

Interesting... That failure doesn't happen locally. I wonder what's causing that 🤔

@jwoertink
Copy link
Collaborator Author

Ok, I was able to get this to fail locally. It looks like those two messages are just reversed. I guess the question here is, does that ordering matter? Or is it just a matter of checking that those specific messages are received?

@jwoertink
Copy link
Collaborator Author

LOL! Ok, this is a little ridiculous... If I focus that spec, I can get it to fail locally. So then I swap those two lines, it passes. Then I run the entire suite, and it fails 🤦 This leads me to believe that it's a race condition. I'll update it so it just checks that those lines are in the messages, but not really care about the order.

Comment on lines +291 to +297
Cable::Logger.messages.should contain("ChatChannel is streaming from chat_1")
Cable::Logger.messages.should contain("ChatChannel is transmitting the subscription confirmation")
Cable::Logger.messages.should contain("RejectionChannel is transmitting the subscription rejection")
Cable::Logger.messages.should contain("[ActionCable] Broadcasting to chat_1: {\"foo\" => \"bar\"}")
# and here we can confirm the message was broadcasted
Cable::Logger.messages[4].should eq("[ActionCable] Broadcasting to rejection: {\"foo\" => \"bar\"}")
Cable::Logger.messages[5].should eq("ChatChannel transmitting {\"foo\" => \"bar\"} (via streamed from chat_1)")
Cable::Logger.messages.should contain("ChatChannel transmitting {\"foo\" => \"bar\"} (via streamed from chat_1)")
Cable::Logger.messages.should contain("[ActionCable] Broadcasting to rejection: {\"foo\" => \"bar\"}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The last 2 [4] and [5] seem to randomly swap. I'm not sure if the order really matters here, or if it just matters that they exist.

@fernandes fernandes self-requested a review May 7, 2021 19:53
@fernandes
Copy link
Collaborator

TYVM @jwoertink !! 👏 👏 👏

@fernandes fernandes merged commit 3253d4c into cable-cr:master May 7, 2021
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

Successfully merging this pull request may close these issues.

2 participants