Conversation
ericsciple
commented
Jan 23, 2020
ericsciple
commented
- https://github.com should match no_proxy=github.com:443
- https://github.com:8080 should match no_proxy=github.com
…no_proxy=foo.com:443 and 2) https://foo.com:8080 should match no_proxy=foo.com
| }) | ||
|
|
||
| it('does not return proxyUrl if variables not set', () => { | ||
| it('getProxyUrl does not return proxyUrl if variables not set', () => { |
There was a problem hiding this comment.
To simplify the unit tests related to bypass scenarios, I added another exported function checkBypass(string) to proxy.ts
Therefore I prefixed the test names to clarify which function was being exercised.
| }) | ||
|
|
||
| it('does not return proxyUrl if only host as no_proxy list', () => { | ||
| it('getProxyUrl does not return proxyUrl if https_proxy set and in no_proxy list', () => { |
There was a problem hiding this comment.
Many of the existing no_proxy tests moved down to the section that exercises the checkBypass function.
I kept a few basic tests here:
- https_proxy in no_proxy
- https_proxy not in no_proxy
- http_proxy in no_proxy
- http_proxy not in no_proxy
| it('returns proxyUrl if https_proxy set and not in no_proxy list', () => { | ||
| process.env["https_proxy"] = "https://myproxysvr"; | ||
|
|
||
| it('checkBypass returns true if host with port in no_proxy list without port', () => { |
There was a problem hiding this comment.
This test is genuinely new and failed before.
| expect(bypass).toBeTruthy(); | ||
| }) | ||
|
|
||
| it('checkBypass returns true if host in no_proxy list with default https port', () => { |
There was a problem hiding this comment.
This test is genuinely new and failed before.
Since the no_proxy list is shared between https_proxy and http_proxy, this scenario should work.
| * Returns the proxy URL, depending upon the supplied url and proxy environment variables. | ||
| * @param serverUrl The server URL where the request will be sent. For example, https://api.github.com | ||
| */ | ||
| export function getProxyUrl(serverUrl: string): string { |
There was a problem hiding this comment.
Public function now exported. I'm planning to use this in checkout to add the server url to the git config
| reqPort = 443 | ||
| } | ||
|
|
||
| // Format the request hostname and hostname with port |
There was a problem hiding this comment.
The trick here is generate two host values to check against the no_proxy list.
When reqUrl is like https://foo.com this will create two strings to check against the bypass list: FOO.COM and FOO.COM:443
This fixes the two bugs before:
- https://github.com should match no_proxy=github.com:443
- https://github.com:8080 should match no_proxy=github.com
| upperReqHosts.push(`${upperReqHosts[0]}:${reqPort}`) | ||
| } | ||
|
|
||
| // Compare request host against noproxy |
There was a problem hiding this comment.
also note i check uppercase rather than lowercase which i believe is supposed to be better b/c of Turkish i
|
|
||
| // Determine the request port | ||
| let reqPort: number | ||
| if (reqUrl.port) { |
There was a problem hiding this comment.
Url.port is empty for urls like http://foo.com so i have a few if blocks to set based on the default port for the protocol
ericsciple
changed the title
|
LGTM |
2 participants
