Skip to content

[#539] Use non-destructive gsub, fix test stubbing - #540

Merged
jnunemaker merged 1 commit into
jnunemaker:masterfrom
rsullivan00:rs/encoding-issues
May 18, 2017
Merged

[#539] Use non-destructive gsub, fix test stubbing#540
jnunemaker merged 1 commit into
jnunemaker:masterfrom
rsullivan00:rs/encoding-issues

Conversation

@rsullivan00

@rsullivan00 rsullivan00 commented May 18, 2017

Copy link
Copy Markdown
Contributor

Closes #539

Apparently I like breaking stuff with my changes 😭 . This PR fixes gsub! from breaking on frozen string literals.

Some of the tests around this were actually stubbing the body call directly, instead of setting the data. I've changed these tests to inject the data instead. As a high-level note, a lot of tests for Parser are stubbing private methods, which I don't think is a good way to structure tests.

We should be able to test all use cases of Parser through the external API. When we stub, or test directly, internal methods, any implementation change can break any number of other tests, even if the change doesn't affect external behavior.

@jnunemaker

Copy link
Copy Markdown
Owner

As a high-level note, a lot of tests for Parser are stubbing private methods, which I don't think is a good way to structure tests.

Agreed. I wasn't the maintainer when most of the parsing stuff went in (took some time away) and just haven't had the energy to clean things up. Thanks!

@jnunemaker
jnunemaker merged commit 2c29c7e into jnunemaker:master May 18, 2017
@henrik

henrik commented May 19, 2017

Copy link
Copy Markdown
Contributor

I guess this is the fix for this issue?

07:45:09 >> HTTParty.get("https://auctionet.com/api/v2/companies.json").parsed_response
Encoding::CompatibilityError: incompatible encoding regexp match (UTF-8 regexp with ASCII-8BIT string)
	from /Users/henrik/.asdf/installs/ruby/2.4.1/lib/ruby/gems/2.4.0/gems/httparty-0.15.2/lib/httparty/parser.rb:120:in `gsub'

EDIT: Yes, it is.

Thanks for fixing it! Was just about to report it.

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.

The change made in 0.15.4 breaks httparty if frozen_string_literal is set to true

3 participants