search

centrifugal / rubycent

Ruby gem to communicate with Centrifugo HTTP API
MIT License
20 stars 16 forks source link

fix bug in js and add webmock/vcr to specs #8

Closed gingray closed 8 years ago

gingray commented 8 years ago

Basically i can't point exactly where bug was in js bcz original source was minified.

FZambia commented 8 years ago

@arrowcircle could you look at this?

arrowcircle commented 8 years ago

Hey! Thanks for commits!

Why You decided to use VCR? I ignored it to have ability to test code on actual api of real Centrifuge. Because of test instance on heroku, its not a problem to fire requests to real api.

@FZambia What do You think? Should we use VCR here or continue to test with real requests?

gingray commented 8 years ago

@arrowcircle in common its bad practice that ur test make real requests i was driven by this idea @FZambia yeah commits looks ugly was a little bit late when i try to play nice with travis. If main idea of commits is good i'll squash them to one.

FZambia commented 8 years ago

This is strange - a couple of weeks ago Github released "squash commits and merge" feature but I don't see this button here in this pull request...

Generally I agree with @gingray to not use real system for tests, but I actually do this myself in Centrifugo when testing Redis engine. But it's configured in travis config so every time tests run - new Redis instance starts. Here we don't have an easy way to start Centrifugo in travis. @arrowcircle so it's up to you to decide what is a better approach here.

gingray commented 8 years ago

@FZambia i can do squash manually. I need approve from you that everything fine or suggestions what to do better.

arrowcircle commented 8 years ago

@FZambia @gingray So lets move to VCR. Please bump version and squash commits.

gingray commented 8 years ago

@FZambia @arrowcircle done plz check this out

arrowcircle commented 8 years ago

@gingray Thank You! Merging and releasing