-
-
Notifications
You must be signed in to change notification settings - Fork 707
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
chore(deps-dev): bump webmock from 3.18.1 to 3.19.1 #11470
Conversation
The changelog and code change looks good but there are failing specs. Maybe it's some inter-gem incompatibility. Needs investigating. |
7703dad
to
7ecf43a
Compare
380e559
to
8a8099b
Compare
I'm still struggling with
even if ignoring host I'm blocked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a solution. 😄
stub_request(:get, ->(uri) { uri.to_s.include? "/css/mail" }) | ||
# Ignore requests for CSS pack like: 'http://test.host/packs-test/css/mail-1ab2dc7f.css' | ||
VCR.configure do |config| | ||
config.ignore_hosts('test.host') | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the rendering is trying to access CSS in test.host. Configuring VCR to ignore that request just means that it's still handled by Webmock and it doesn't allow external requests. Well, since test.host
is not a valid domain name, it fails resolving it. The original stub solved this by intercepting the request and not returning anything.
So when we have the stub in there, we get the frozen string error but the stack trace is limited to our code. I put a binding.pry
in front of that line and called wicked_pdf_stylesheet_pack_tag(source)
within the console which revealed the same error but now pointing to the source:
Wicked PDF tries to read the URL and gets a string. But that string is provided by Webmock. It tries to force the encoding on the string which would modify it but it's not allowed to because Webmock is using a frozen string in this new version.
I tried defining the returned string but it's frozen by default because we add frozen_string_literal: true
at the top of our files. A workaround is to duplicate it to a non-frozen string:
stub_request(:get, ->(uri) { uri.to_s.include? "/css/mail" }).to_return(body: "".dup)
This works. And we don't need to ignore the host in VCR. I reckon that this should be fixed in wicked_pdf but webmock could also be patched to be compatible with this.
I think this commit introduced it:
Maybe it's this line that could be changed:
Patching Webmock works but I'm not sure if they would accept my patch. So we should go with our workaround for now, especially because it's only needed in combination with wicked_pdf.
Bumps [webmock](https://github.com/bblimke/webmock) from 3.18.1 to 3.19.1. - [Changelog](https://github.com/bblimke/webmock/blob/master/CHANGELOG.md) - [Commits](bblimke/webmock@v3.18.1...v3.19.1) --- updated-dependencies: - dependency-name: webmock dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
``` Failed to open TCP connection to test.host:80 ```
8a8099b
to
9ea6fc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, finally. Thank you!
Bumps webmock from 3.18.1 to 3.19.1.
Changelog
Sourced from webmock's changelog.
Commits
f5726d6
Version 3.19.18401c6e
When passing lambda as body, to to_return_json, it is evaluated at the time o...d707719
Version 3.19.09305ab5
Merge pull request #1033 from technicalpickles/frozen-string-literald4afbd9
Merge pull request #1029 from baweaver/baweaver/performance/normalize_headers...19917a3
Merge pull request #1030 from Yuki-Inoue/masterd933001
Merge pull request #1012 from inkstak/feature-to_return_json_accepts_array_body0d422a1
Merge pull request #1026 from baweaver/patch-287786a2
Merge pull request #1006 from marcrohloff/fix-version-checking0e9519c
Merge branch 'master' into fix-version-checkingYou can trigger a rebase of this PR by commenting
@dependabot rebase
.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditions
will show all of the ignore conditions of the specified dependency@dependabot ignore this major version
will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version
will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)