Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First draft of manual #47

Merged
merged 14 commits into from
Aug 27, 2018
Merged

First draft of manual #47

merged 14 commits into from
Aug 27, 2018

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Jul 23, 2018

I think this is pretty comprehensive and complete. It probably needs
some editing.

Also, we shouldn't merge it yet because it's inaccurate: it describes
the way things should work, rather than the way they actually work
right now :-). So before merging, we should:

Fixes: #5

@codecov
Copy link

codecov bot commented Jul 23, 2018

Codecov Report

Merging #47 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #47   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files          19       19           
  Lines         457      457           
  Branches       44       44           
=======================================
  Hits          454      454           
  Misses          2        2           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd27d70...6d10627. Read the comment docs.

njsmith added a commit to njsmith/pytest-trio that referenced this pull request Jul 23, 2018
@njsmith njsmith mentioned this pull request Jul 23, 2018
njsmith added a commit to njsmith/pytest-trio that referenced this pull request Jul 24, 2018
@njsmith
Copy link
Member Author

njsmith commented Jul 24, 2018

Okay, I think this manual now correctly describes master + #49 + #50.

Still need to update cookiecutter-trio.


* Use `Hypothesis <https://hypothesis.works/>`__? This plugin
integrates with Hypothesis, so your async tests can use
property-based testing.
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be useful here to have a word about hypothesis-trio and the fact you need it to run stateful tests ? (we could put it in the Integration with the Hypothesis library chapter for instance)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we should totally point to it! I've added some text to reference.rst. I don't think I understand well enough what hypothesis-trio actually does yet to figure out if we should put something in the little blurb here in index.rst, so for now I didn't – but of course we can always add it later.


Probably our first attempt will look something like::

# DON'T DO THIS, IT DOESN'T WORK
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why you say this doesn't work, this code seems to work perfectly:

import trio
import pytest
from functools import partial

async def echo_server_handler(stream):
    while True:
        data = await stream.receive_some(1000)
        if not data:
            break
        await stream.send_all(data)

@pytest.fixture
async def echo_server_connection():
    async with trio.open_nursery() as nursery:
        listeners = await nursery.start(
            partial(trio.serve_tcp, echo_server_handler, port=0)
        )
        yield listeners[0]
        nursery.cancel_scope.cancel()

@pytest.mark.trio
async def test_connection(echo_server_connection):
    client = await trio.testing.open_stream_to_socket_listener(echo_server_connection)
    async with client:
        for test_byte in [b"a", b"c", b"c"]:
            await client.send_all(test_byte)
            assert await client.receive_some(1) == test_byte

Beside that's a pattern I find extremely useful in my own tests, for instance:

https://github.com/Scille/parsec-cloud/blob/e2569f2eacd93943975b3aefcdca03734c7eebd5/tests/conftest.py#L190-L212

with the backend_factory scope creating it own nursery to guarantee the backend in fully destroyed once we left the scope:

https://github.com/Scille/parsec-cloud/blob/e2569f2eacd93943975b3aefcdca03734c7eebd5/tests/common.py#L90-L101

The great thing I find about this is you don't end up with a root nursery containing all the coroutines which make make it much harder to tell which one is responsible when your test hangs with a deadlock.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, we're both right – it doesn't work, and it's extremely useful. Since the text is correct I'm going to leave it for now, and let's discuss how we can fix it over here: #55


* Async tests have to be decorated with ``@pytest.mark.trio``
* Async fixtures have to be decorated with
``@pytest_trio.trio_fixture`` (instead of ``@pytest.fixture``).
Copy link
Member

Choose a reason for hiding this comment

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

But tests decorated with pytest.mark.trio can still detect and run async fixture marked with pytest.fixture, right ?
So I would say pytest_trio.trio_fixture is only useful if you want to have a synchronous fixture (with no asynchronous dependencies) running within the trio loop...

I seems you explain this lower, so I guess it's fine to have it not 100% accurate here ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, right, this text got confused – I originally wrote this thinking that async fixtures would always require either trio-mode or trio_fixture, then I switched to automatically coercing async fixtures used by pytest.mark.trio-tests, but I didn't update this text.

@trio_fixture is also useful to explicitly declare that a fixture is a trio fixture, so e.g. if you accidentally try to use it with a synchronous or asyncio test then you'll get a nice error message instead of some weird crash.

After sleeping on #51 and investigating some more, I think I actually want to switch the behavior again, to the union of the two things we've talked about: if trio-mode is enabled then all async fixtures are automatically converted to trio fixtures, and even when trio-mode is disabled then all async fixtures that are used by a trio test are automatically converted to trio fixtures. That keeps the current convenience for when we're not in non-trio-mode (and is backwards compatible), and then for projects that use trio-mode it also lets us catch mistakes like bad scopes or using async fixtures with a non-trio test.

@njsmith
Copy link
Member Author

njsmith commented Aug 15, 2018

This is currently a little out-of-date with #50, so don't merge it yet :-)

  • magic fixture name is nursery again (but its semantics are different)
  • nurseries inside fixtures are OK now! (but you might prefer the nursery fixture in common cases)
  • fixture setup/teardown is concurrent now

The fixture setup code doesn't have convenient access to config file
settings, and also keying the automagic-async-fixture-handling off of
the trio-mode setting would be a compatibility break. Not the end of
the world, but kind of annoying.

Instead, let's continue to key the automagic-async-fixture-handling
off of the requesting test being a Trio test. It's just as convenient,
and while technically could be a little more error-prone –
specifically, if you accidentally use an async fixture with a
non-async test, then we won't be able to catch that as an error – I
doubt it matters much in practice. And if people turn out to make that
mistake a lot, we can always add some check for it later.
Also wrote some draft release notes
@njsmith
Copy link
Member Author

njsmith commented Aug 17, 2018

Updated to match the latest changes in #50, + added some draft release notes, + rebased.

@njsmith
Copy link
Member Author

njsmith commented Aug 18, 2018

Test failures are because of the pytest bug with 3.7.1. #50 has a workaround, so once #50 is merged the tests here should start passing again.

@njsmith
Copy link
Member Author

njsmith commented Aug 24, 2018

Once this goes in, I think we're ready to release v0.5.0. (@touilleMan, do you want to do the honors, or shall I?)

This release cycle is weird: there's no newsfragments, but in this PR I manually added some news to the history.rst file. This file needs to be updated to include the final release date.

open a nursery, pass it in to your function, and then cancel it for
you afterwards:

.. code-block:: python3
Copy link
Member

Choose a reason for hiding this comment

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

This and other codeblocks aren't showing up in github's rst preview. Doc issue or github issue??

Copy link
Member

@belm0 belm0 left a comment

Choose a reason for hiding this comment

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

read through the manual-- LGTM
(I've used about half of the pytest-trio features)

trio fixture
* a number of easy-to-make mistakes are now caught and raise
informative errors
* the :data:`nursery` fixture is now 87% more magical
Copy link
Member

Choose a reason for hiding this comment

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

Did you round the magical percentage here ? :trollface:

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it needs more decimal points?

@touilleMan
Copy link
Member

@njsmith I'll let you merge this PR and do the release

@njsmith njsmith merged commit 2c5f476 into python-trio:master Aug 27, 2018
@njsmith njsmith deleted the docs branch August 27, 2018 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write documentation
3 participants