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

Better test to ensure that query strings are not being rewritten. #66

Merged
merged 1 commit into from Nov 13, 2012

Conversation

pdg137
Copy link
Contributor

@pdg137 pdg137 commented Sep 27, 2012

No description provided.

… previous version of this test would pass on revision 8153c07 with Ruby 1.9 due to ordered hashes; this one checks both argument order and URL-encoding.
@brynary
Copy link
Collaborator

brynary commented Nov 11, 2012

Thanks for the PR. Looks good, but can you help me understand the motivation. Did you run into a bug here?

@pdg137
Copy link
Contributor Author

pdg137 commented Nov 12, 2012

The test should make sure that the order of arguments is not changed from the original query string. I was doing my original testing on Ruby 1.8.7, which uses unordered hashes, so if my arguments were split up into a hash, they got randomized, causing the test to fail with high probability, as desired. In 1.9, the hashes are ordered, which I believe results in an alphabetically-ordered list of params. Since my original simple query string was already in alphabetical order, it did not get rearranged, making it a useless test on 1.9.

This new version uses a query string with arguments that are not in alphabetical order, so it should be more rigorous. Also, I added the +%20 to test whether any kind of URL-decoding/encoding is being applied, which catches it even if the argument order does not change.

brynary added a commit that referenced this pull request Nov 13, 2012
Better test to ensure that query strings are not being rewritten.
@brynary brynary merged commit 5652789 into rack:master Nov 13, 2012
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.

None yet

2 participants