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

Support API_URLs that include path #41

Merged
merged 5 commits into from
Apr 21, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions lib/webvalve/fake_service_wrapper.rb
Original file line number Diff line number Diff line change
@@ -1,18 +1,23 @@
module WebValve
class FakeServiceWrapper
# lazily resolve the app constant to leverage rails class reloading
def initialize(app_class_name)
@app_class_name = app_class_name
def initialize(service_config)
@service_config = service_config
end

def call(env)
env['PATH_INFO'] = env['PATH_INFO'].gsub(/\A#{path_prefix}/, '')
app.call(env)
end

private

def app
@app_class_name.constantize
@service_config.service_class_name.constantize
end

def path_prefix
@path_prefix ||= URI::parse(@service_config.service_url).path
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we're parsing URLs all over the place and could consolidate onto the config. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, it might be nice to have the config expose a path_prefix method instead of doing this here. it won't be used anywhere else, but it would be tidy.

end
end
end
3 changes: 2 additions & 1 deletion lib/webvalve/manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ def reset
allowlisted_urls.clear
fake_service_configs.clear
stubbed_urls.clear
WebMock.reset!
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was required to prevent global state from mucking up the test run and making this test flake:
https://github.com/Betterment/webvalve/pull/41/files#diff-4f60a691d33035e7b6c85487ba4937efR55

I was surprised that we leave existing WebMock stubs around even when we clear out the local registry of stubbed URLs. But maybe this shouldn't go here? I could put it in that one test's before hook instead? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine. i think reset is only used internally.

end

# @api private
Expand Down Expand Up @@ -132,7 +133,7 @@ def webmock_service(config)
WebMock.stub_request(
:any,
url_to_regexp(config.service_url)
).to_rack(FakeServiceWrapper.new(config.service_class_name))
).to_rack(FakeServiceWrapper.new(config))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor API change but it's an internal class right?

end

def allowlist_service(config)
Expand Down
43 changes: 33 additions & 10 deletions spec/webvalve/fake_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,44 @@ def self.name
WebValve.reset
end

it 'raise a useful error when an unmapped route is requested' do
with_env 'DUMMY_API_URL' => 'http://dummy.dev' do
WebValve.register subject.name
WebValve.setup
context 'when the service is at a root path' do
it 'raise a useful error when an unmapped route is requested' do
with_env 'DUMMY_API_URL' => 'http://dummy.dev' do
WebValve.register subject.name
WebValve.setup

expect { Net::HTTP.get(URI('http://dummy.dev/foos')) }.to raise_error(RuntimeError, /route not defined for GET/)
expect { Net::HTTP.get(URI('http://dummy.dev/foos')) }.to raise_error(RuntimeError, /route not defined for GET/)
end
end

it 'returns the result from the fake when a mapped route is requested' do
with_env 'DUMMY_API_URL' => 'http://dummy.dev' do
WebValve.register subject.name
WebValve.setup

expect(Net::HTTP.get(URI('http://dummy.dev/widgets'))).to eq({ result: 'it works!' }.to_json)
end
end
end

it 'returns the result from the fake when a mapped route is requested' do
with_env 'DUMMY_API_URL' => 'http://dummy.dev' do
WebValve.register subject.name
WebValve.setup
context 'when the service lives at a non-root path' do
it 'raise a useful error when the route is requested at the root' do
with_env 'DUMMY_API_URL' => 'http://dummy.dev/gg' do
WebValve.register subject.name
WebValve.setup

expect { Net::HTTP.get(URI('http://dummy.dev/widgets')) }
.to raise_error(WebMock::NetConnectNotAllowedError, /Real HTTP connections are disabled/)
smudge marked this conversation as resolved.
Show resolved Hide resolved
end
end

it 'returns the result from the fake when a mapped route is requested' do
with_env 'DUMMY_API_URL' => 'http://dummy.dev/gg' do
WebValve.register subject.name
WebValve.setup

expect(Net::HTTP.get(URI('http://dummy.dev/widgets'))).to eq({ result: 'it works!' }.to_json)
expect(Net::HTTP.get(URI('http://dummy.dev/gg/widgets'))).to eq({ result: 'it works!' }.to_json)
end
end
end
end
Expand Down