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

HTTP20Adapter.close tests (supersedes #307) #344

Merged
merged 4 commits into from
Jul 31, 2017

Conversation

KostyaEsmukov
Copy link
Contributor

@KostyaEsmukov KostyaEsmukov commented Jul 31, 2017

Add close() method implementation to the requests adapter and integration tests for it.

Supersedes #307
Related issue: #306

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.

Thanks for this! It would be good if we could add a test that demonstrates that calling close on the adapter both closes the underlying connections and also clears the connection cache. Mind adding those?

@KostyaEsmukov
Copy link
Contributor Author

I've added that check to these two tests instead of creating a new one. Let me know if you think that this should be checked in a separate test.


# check that all connections are actually closed
assert (connections_before_close and
all(conn._sock is None for conn in connections_before_close))
Copy link
Member

Choose a reason for hiding this comment

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

Let's split the first assertion out and call it when we get the connections initially.


# check that all connections are actually closed
assert (connections_before_close and
all(conn._sock is None for conn in connections_before_close))
Copy link
Member

Choose a reason for hiding this comment

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

Same note here.

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.

Very nice, this looks really good. Thanks so much!

@Lukasa Lukasa merged commit 669253f into python-hyper:development Jul 31, 2017
@KostyaEsmukov KostyaEsmukov deleted the pr/requests_close branch September 2, 2017 07:00
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