Skip to content

Refactor tests in the light of backend auto-detection - #615

Merged
florimondmanca merged 4 commits into
masterfrom
tests/auto-backend
Dec 7, 2019
Merged

florimondmanca merged 4 commits into
masterfrom
tests/auto-backend

Conversation

@florimondmanca

Copy link
Copy Markdown
Contributor

Following up comments from #614, this PR updates the tests so that:

  • They prefer passing the string-form backend ("auto", "asyncio", "trio") to all pieces of public API.
  • The "auto" option is put under test as well.

Fixes a bunch of NotImplementedErrors in the concurrency test utils as well.

Tests that use the backend are now twice as many (because the backend fixture now has two more params - auto-in-asyncio and auto-in-trio). I figured the test suite was still fast enough that running all these tests for each situation was acceptable, rather than e.g. unit-testing that passing "asyncio" looks up AsyncioBackend. Now that I wrote about it I'm less sure this wouldn't be preferable.

@florimondmanca florimondmanca added tooling Changes to our CI/CD, tests setup, etc. refactor Issues and PRs related to code refactoring labels Dec 7, 2019
@florimondmanca

Copy link
Copy Markdown
Contributor Author

Okay, I've moved things around a bit more so that:

  • We only have two params for backend: auto-in-asyncio, auto-in-trio. (In particular this allows running tests for only one of the async libraries using pytest -m [asyncio|trio], which we'd lose with the previous pass.)
  • We rely on auto-detection, as most users should, i.e. don't pass backend=backend to clients, connection pools or connections.
  • We test passing the backend explicitly in a dedicated test.

@florimondmanca florimondmanca added the concurrency Issues related to concurrency and usage of async libraries label Dec 7, 2019

@lovelydinosaur lovelydinosaur left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks ace, yup!

@florimondmanca
florimondmanca merged commit ab41a5d into master Dec 7, 2019
@florimondmanca
florimondmanca deleted the tests/auto-backend branch December 7, 2019 14:17
@@ -7,7 +7,7 @@

async def test_get(server, backend):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can also remove the backend fixture param from all these test functions since it's not being referenced anywhere.

@florimondmanca florimondmanca Dec 9, 2019 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, but actually we can’t — even though we don’t use the backend fixture in the test it’s responsible for setting up the asyncio/trio environment. I agree the usage as a function parameter is misleading though. We can probably switch to pytest.mark.usefixtures.

@florimondmanca florimondmanca Dec 9, 2019 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can probably switch to pytest.mark.usefixtures.

... and rename the fixture to « async_environment » or something? It’s true that the fixture doesn’t even return a backend class — it’s always « auto ». Hmm. Which means we might then get away with a custom mark, eg pytest.mark.is_async?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I only suggested it since it is supposed to be auto-detected now, and they could be parameterized like https://github.com/encode/httpx/pull/615/files#diff-21bb0534f2645b17f8572db206999d00R165-R171
Which I now see is done here: https://github.com/encode/httpx/pull/615/files#diff-484462fced51d1a06b1d93b4a44dd535R52-R64 so, well done! Sorry I missed that before.
It is a little confusing as a function param, so I would agree with renaming it "async_backend" and then using it as @pytest.mark.usefixtures("async_backend") since all it's doing is adding the mark for asyncio/trio to do their thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

concurrency Issues related to concurrency and usage of async libraries refactor Issues and PRs related to code refactoring tooling Changes to our CI/CD, tests setup, etc.

Development

Successfully merging this pull request may close these issues.

3 participants