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

Conversation

smudge
Copy link
Member

@smudge smudge commented Apr 17, 2020

/domain @samandmoore @effron
/platform @samandmoore @effron

Will annotate below.

Build is currently failing intermittently due to a test order thing. Will debug that but wanted to get the code up in the meantime.

@nanda-prbot
Copy link

Needs somebody from @samandmoore and @effron to claim domain review
Needs somebody from @samandmoore and @effron to claim platform review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

HOW TO: Claim a Review

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.

@@ -132,7 +132,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?

@@ -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.

@samandmoore
Copy link
Member

<<domain platform tafn

@nanda-prbot
Copy link

@smudge needs to incorporate feedback from @samandmoore. Bump when done.

HOW TO: Resolve Feedback

@smudge
Copy link
Member Author

smudge commented Apr 20, 2020

bump @samandmoore (and @jmileham)

I got cute and wrote this test (and it passed) but then I deleted it and changed the ^ to \A:

      it 'is immune to newlines that are injected in the path' 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/gg/banana/%0Agg/widgets')) }
            .to raise_error(RuntimeError, /route not defined for GET/)
          expect { Net::HTTP.get(URI("http://dummy.dev/gg/banana/\ngg/widgets")) }
            .to raise_error(URI::InvalidURIError, /bad URI\(is not URI\?\)/)
        end
      end

@nanda-prbot
Copy link

Needs @samandmoore to provide platform review
Needs @samandmoore to provide domain review

When you finish a round of review, be sure to say you've finished or sign off on the PR, e.g.:

TAFN or DomainLGTM

If you're too busy to review, unclaim the PR, e.g.:

@myname >> domain

HOW TO: Give Feedback

@jmileham
Copy link
Member

i assume you deleted the test because you finally proved to yourself once and for all that it doesn't make sense to use an operator that means "match the front of the current line," in a construct that doesn't support multiple lines :trollface:

@samandmoore
Copy link
Member

just that one last suggestion

then domainlgtm platformlgtm

i think we can cut this as the official 1.0.0 release :)

@nanda-prbot
Copy link

Approved! 👏 🙌 🌮

expect(subject.path_prefix).to eq '/welcome'
end
with_env 'DUMMY_API_URL' => 'http://zombo.com/welcome/' do
expect(subject.path_prefix).to eq '/welcome' # Ignores trailing '/'
Copy link
Member Author

@smudge smudge Apr 21, 2020

Choose a reason for hiding this comment

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

I’m sort of testing URI::parse but I wanted a loose demonstration of basic URL/path expectations in case the underlying parsing implementation ever changes.

@smudge smudge merged commit 28c666f into Betterment:master Apr 21, 2020
@smudge smudge deleted the service-path-mounting branch April 21, 2020 23:12
@samandmoore
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants