em-http-request problems with queries #210

merged 1 commit into from Sep 9, 2012


None yet
2 participants

wrozka commented Sep 6, 2012

I'm using webmock to test integration with pusher (vcr, webmock, pusher, pusher-client and em-http-request) and I have noticed that webmock was breaking the em-http-requests to pusher servers.

It modified the internal request/uri from em-http request. I fixed that by duplicating the uri for purpose of creating WebMock::RequestSignature.

Sorry for specles request, but I have no time for digging more in WebMock, believe me, it is covered in my project's test suite. ;)


bblimke commented Sep 7, 2012

Hi Pawel.

Thank you for this pull request and fixing the problem.

I do believe this fixes the problem in your project and that it's covered in your project but I'm afraid I will need a spec in WebMock anyway.

The spec is needed for two reasons:

  1. It shows why this code was added in git history if someone plans to change this code in the future.
  2. It protects against regressions in WebMock if someone breaks this functionality i.e. in some
    future pull request.

If not the spec, are you able at least to show a sample code that is broken without your changes?
I need to know what's exactly being fixed before merging this code :)

Have a look at this pull request. Is it something similar?


wrozka commented Sep 7, 2012

Hi Bartosz,
I agree in 100% that test is needed, I simply had no time to add it.
That is the same problem as in the @jonleighton's pull request, but in the 0.x version. He added the spec and there is only one for both em-http-request versions so bundling with old em-http-request should reproduce the problem.


bblimke commented Sep 7, 2012

Thanks for checking that. I'll ensure this spec runs for both adapters and merge your code when I have a minute.

bblimke added a commit that referenced this pull request Sep 9, 2012

Merge pull request #210 from wrozka/master
em-http-request problems with queries

@bblimke bblimke merged commit b7ebbab into bblimke:master Sep 9, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment