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

Fixes misleading error message #705

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lazylester
Copy link

The test may not be as elegant as the rest of the code, but it does the job!

@lazylester
Copy link
Author

hmmm not sure why Travis CI fails... the tests are passing on my dev machine.

@bblimke
Copy link
Owner

bblimke commented Jun 30, 2017

Thank you for the pull request.

Any reason why the test has been added to test and not to a spec? Tests are mainly to ensure compatibility with minitest and that's it. All webmock behaviours should be tested in rspec.

Have you checked if the changed you made have any impact on how utf-8 body is printed?
E.g error message for body that contains non ascii characters.

Can you please explain what's so specific to the two body fles you added to tests. Why do these fail and not others?

@lazylester
Copy link
Author

Not sure why I put the new test in the test dir and not the spec dir. Which spec do you suggest is appropriate for testing this anomaly using rspec? I'll look into moving the test there if you are interested in merging this PR.

The rendering of the UTF-8 body text is perfectly legible in the error message, but the binary data in the other request is butchered, of course, as there is no reasonable way to present it on a terminal.

It's a bit hard to know what particular aspect of the two files triggers the problem. Ordinarily one might figure it out by elimination... edit the files until the problem disappears, but I don't know how to edit these files, a text editor doesn't work.

The first file is produced by the postgres pg_dump utility, and I have found no details about its format, except that it is encoded the same as the database encoding (UTF-8 in my case). The second file is the output of gzip; in the test I used the output from gzip of an empty file; I assume that it is a binary file with an ascii encoding.

It surprised me a little that the output of the pg_dump could be rendered so well in the error message, as it's supposed to be compressed, which suggests binary data to me!

Having isolated some strings that would trigger the problem, I didn't take the next step of identifying the minimum strings that would trigger the problem, which is probably the way to get insight and understand the exact mechanism. I am certainly curious, as you probably are.

@bblimke
Copy link
Owner

bblimke commented Jul 1, 2017

I wonder if the problem could be isolated to e.g. a specific single byte that causes the problem,
as I understand not every binary file causes the issue?

Perhaps a hex editor could be used for that (e.h 0xED on macos) or split -b to split file in half and do a binary search for the byte that causes the problem.

It can be a specific byte sequence, not a single byte though.

@lazylester
Copy link
Author

I had hoped it would be sufficient to reproduce the problem with any test data that could be found to trigger the issue. So I have offered a fix based on that premise. This gets me past the issue so that it's no longer blocking my progress.

I will try to find some time to dig a little deeper. But no guarantees.

No worries if you don't feel that you can merge my PR. Hopefully I contributed a little to the understanding and it's documented here for posterity.

@bblimke
Copy link
Owner

bblimke commented Jul 12, 2017

@lazylester yes, thank you for that.

I'd like to narrow it down to make it clear what was the issue and what exactly has been fixed,
rather than adding a change that works on "some binary files". I'd like each change to be deterministic.
Therefore I if it's easy to check what bytes cause the issue. Perhaps with binary search of these binary files (binary binary) ;), then I'd prefer to do that first, before merging.

@lazylester
Copy link
Author

@bblimke you're absolutely right. I appreciate your rigorous approach. I will try to get a more definitive analysis for you.

@timon
Copy link

timon commented May 13, 2022

I have just stumbled upon this issue
My setup boils down to the following (I'm trying to simplify my case as it is an end to end test for a workflow and actual number of requests involved is quite high):

  • Add two mocks, one for POST request with JSON data, another for POST request with PDF upload
  • Perform PDF upload with actual PDF file
  • Perform JSON data POST request
  • Verify that JSON POST was fired twice

Failing expectation for request count on one of the mocks triggers RequestRegistry.instance.to_s which tries to mix the actual PDF post data with the other call.

P.S. I see the point of stringifying request registry, but for this use case I'm actually not interested in seeing binary PDF contents in rspec's output

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.

3 participants