Skip to content
This repository was archived by the owner on Jan 13, 2021. It is now read-only.

Implement HTTP20Adapter.close to close connections #307

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Implement HTTP20Adapter.close to close connections #307

wants to merge 1 commit into from

Conversation

@allanlei
Copy link

@allanlei allanlei commented Feb 11, 2017 •
edited
Loading

Fixes #306

@allanlei allanlei force-pushed the issue/306 branch 2 times, most recently from ed08296 to cc905d5 Compare February 11, 2017 06:54
@allanlei
Copy link
Author

The http2bin.org endpoint seems to be down, is there another HTTP2 endpoint or a way to dummy the connection?

Lukasa
Lukasa suggested changes Feb 11, 2017
View reviewed changes
Copy link
Member

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Cool, so this looks reasonable! I have made some suggestions for alternative testing approaches.

test/test_hyper.py Outdated
with requests.Session() as s:
a = HTTP20Adapter()
s.mount('https://http2bin.org', a)
s.get('https://http2bin.org/')
Copy link
Member

Choose a reason for hiding this comment

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

I strongly recommend avoiding making real web requests in this test module (even if the rest of them do).

I recommend mocking out some of the lower-level functions. It's probably enough to mock out the send method of the adapter and the HTTPConnection object.

Copy link
Author

Choose a reason for hiding this comment

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

I've changed the test to just mount the adapter without making a request. It raises the same error.

Copy link
Member

@nateprewitt nateprewitt Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @allanlei, it looks like pytest doesn't think this test case is actually hitting line 163 (connection._conn.close()) in contrib.py. Until that's properly triggered, Travis will continue returning a failure.

Without initiating a request, I'm not sure if self.connections.values() will be populated. If that's the case, the for loop will never execute.

@Lukasa
Copy link
Member

Lukasa commented Feb 27, 2017

So I think @nateprewitt's deleted comment is right: I think we need to initiate a HTTP/2 request here. Rather than use http2bin you may need to write a socket-level test as is done elsewhere in the code.

@nateprewitt
Copy link
Member

Not deleted, just hiding on the last "requested changes" comment thread 😉

@KostyaEsmukov KostyaEsmukov mentioned this pull request Jul 31, 2017
Merged
Lukasa pushed a commit that referenced this pull request Jul 31, 2017
@KostyaEsmukov @Lukasa
HTTP20Adapter.close tests (supersedes #307) (#344)
669253f 
* Implement HTTP20Adapter.close to close connections

* Make requests adapter close() tests integrational

* Ensure that connections are actually closed on closing requests adapter

* Split an assertion in requests adapter close() tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@nateprewitt nateprewitt nateprewitt left review comments

@Lukasa Lukasa Lukasa requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@allanlei @Lukasa @nateprewitt