Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Fix test_connect_ipv6_addr. #75

Merged
merged 2 commits into from
May 6, 2019
Merged

Fix test_connect_ipv6_addr. #75

merged 2 commits into from
May 6, 2019

Conversation

RatanShreshtha
Copy link
Member

I have added a check in for ipv6 host in _build_tunnel_request and accordingly modify the target, is there a better way ?

@RatanShreshtha RatanShreshtha requested a review from njsmith April 14, 2019 03:42
@codecov
Copy link

codecov bot commented Apr 14, 2019

Codecov Report

Merging #75 into bleach-spike will increase coverage by <.01%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           bleach-spike      #75      +/-   ##
================================================
+ Coverage         98.36%   98.37%   +<.01%     
================================================
  Files                30       30              
  Lines              1901     1905       +4     
================================================
+ Hits               1870     1874       +4     
  Misses               31       31
Impacted Files Coverage Δ
src/urllib3/_async/connection.py 96.41% <100%> (+0.07%) ⬆️

1 similar comment
@codecov
Copy link

codecov bot commented Apr 14, 2019

Codecov Report

Merging #75 into bleach-spike will increase coverage by <.01%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           bleach-spike      #75      +/-   ##
================================================
+ Coverage         98.36%   98.37%   +<.01%     
================================================
  Files                30       30              
  Lines              1901     1905       +4     
================================================
+ Hits               1870     1874       +4     
  Misses               31       31
Impacted Files Coverage Δ
src/urllib3/_async/connection.py 96.41% <100%> (+0.07%) ⬆️

Copy link
Member

@njsmith njsmith left a comment

Choose a reason for hiding this comment

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

Looks like a good approach to me!

This is one of those classic ones...
Writing the patch: 10 minutes
Figuring out which patch to write: 10 hours

socket.inet_pton(socket.AF_INET6, host)
target = "[%s]:%d" % (host, port)
except OSError:
target = "%s:%d" % (host, port)
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably write it like:

    try:
        socket.inet_pton(socket.AF_INET6, host)
    except OSError:
        # Not a raw IPv6 address
        target = "%s:%d" % (host, port)
    else:
        # raw IPv6 address
        target = "[%s]:%d" % (host, port)

It doesn't make a big difference here, but it's a good habit in cases like this to only put the minimal stuff inside the try block, to make sure you don't catch any unexpected exceptions by accident.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks it seem more readable to me.

@njsmith
Copy link
Member

njsmith commented Apr 14, 2019

Hmm. I bet we're also going to need a similar check somewhere to set up the Host: header correctly when doing a regular (no-proxy) request to a raw IPv6 address...

@RatanShreshtha
Copy link
Member Author

I will look where we need to set-up such check for Host: header.

@pquentin
Copy link
Member

Hmm. I bet we're also going to need a similar check somewhere to set up the Host: header correctly when doing a regular (no-proxy) request to a raw IPv6 address...

That does not exist at all in the urllib3 tests currently, so maybe we can address this in a future pull request? I opened #87 to this effect

@njsmith njsmith merged commit 3ad210b into python-trio:bleach-spike May 6, 2019
@RatanShreshtha RatanShreshtha deleted the Fix_test_connect_ipv6_addr branch May 6, 2019 05:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants