Skip to content

Conversation

@jonemo
Copy link
Contributor

@jonemo jonemo commented Jan 17, 2023

While running ./pants test :: on another branch, I noticed two related problems:

  1. Async tests (async def test_*) aren't actually running even though they should because we have the asyncio_mode = "auto" setting in pyproject.toml.
  2. If any test fails in a file that also contains an async test, the following warning gets output:
=============================== warnings summary ===============================
../../../../../../../Users/jonasneu/.cache/pants/named_caches/pex_root/venvs/s/5cb658c8/venv/lib/python3.11/site-packages/_pytest/config/__init__.py:1249
  /Users/jonasneu/.cache/pants/named_caches/pex_root/venvs/s/5cb658c8/venv/lib/python3.11/site-packages/_pytest/config/__init__.py:1249: PytestConfigWarning: Unknown config option: asyncio_mode
  
    self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")

tests/integration/test_async_stuff.py::test_async
  /Users/jonasneu/.cache/pants/named_caches/pex_root/venvs/s/5cb658c8/venv/lib/python3.11/site-packages/_pytest/python.py:181: PytestUnhandledCoroutineWarning: async def functions are not natively supported and have been skipped.
  You need to install a suitable plugin for your async framework, for example:
    - anyio
    - pytest-asyncio
    - pytest-tornasync
    - pytest-trio
    - pytest-twisted
    warnings.warn(PytestUnhandledCoroutineWarning(msg.format(nodeid)))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
- generated xml file: /private/var/folders/dr/w3lkv8jj0b53yb35jvm79ckc0000gs/T/pants-sandbox-8PioQv/python-packages.smithy-python.tests.integration.test_async_stuff.py@tests.xml -

Simply upgrading pytest and pytest-asyncio to the latest minor versions resolves the issue. I tested this with the following dummy test file:

# test_async_stuff.py

import pytest

async def test_async():
    await sleep(0.1)
    assert False

def test_nonasync():
    assert 1 + 1 == 3

... and seeing two test failures and zero warnings.

Additionally, I removed the repo's one remaining but unnecessary @pytest.mark.asyncio.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jonemo jonemo requested a review from nateprewitt January 17, 2023 20:44
@jonemo
Copy link
Contributor Author

jonemo commented Jan 17, 2023

Oh right. And this is the one existing test that has always been broken but previously didn't run. Fix coming in next commit...

assert response.body.done


@pytest.mark.asyncio
Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity, why wasn't this needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we have asyncio-mode set to auto in our configuration file, the marker can be avoided.

Relevant docs are here: https://pytest-asyncio.readthedocs.io/en/latest/reference.html#markers

In auto mode, the pytest.mark.asyncio marker can be omitted, the marker is added automatically to async test functions.

async def test_endpoint_provider_with_url_string() -> None:
params = StaticEndpointParams(
url="https://foo.example.com/spam:8080?foo=bar&foo=baz"
url="https://foo.example.com:8080/spam?foo=bar&foo=baz"
Copy link
Contributor

Choose a reason for hiding this comment

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

🤦

@jonemo
Copy link
Contributor Author

jonemo commented Jan 18, 2023

What an adventure getting the CI to actually pass on this. Actually had to add an __eq__ to the URI for the formerly silently skipped tests to pass. And then had to deal with a known disagreement between flake8 and mypy. And then I finally got the green check 🎉

Ready for another look.

@jonemo jonemo requested a review from nateprewitt January 18, 2023 00:53
Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

I think this overall looks great. Thanks for the deep dive @jonemo! I have one comment on the URI that we should take a look at but doesn't need to be part of this PR. Let me know your thoughts, but otherwise, this is fine to merge.

Comment on lines +80 to +87
self.scheme == other.scheme
and self.host == other.host
and self.port == other.port
and self.path == other.path
and self.query == other.query
and self.username == other.username
and self.password == other.password
and self.fragment == other.fragment
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this change, but it's pointing to a couple things I would have noted on the original PR.

URI (and all of our method signatures) should start with (self, *, ...). The goal there is to ensure we only operate with keyword arguments. This provides us more flexibility in ordering on function signatures and prevents breakages with accidental ordering issues.

We should also ideally have these ordered in the constructor (and here) as they appear in the URI when formatted.

{scheme}://{username}:{password}@{host}:{port}{path}{query}{fragment}

We can handle that in a separate PR but should probably track it somewhere unless there's a strong argument with the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My other branch is all about implementing the interfaces as stated in the spec. This PR is just the minimal changes to get asyncio tests working at all. I will take note of this comment for my other branch, but I think I've already got most of it covered there.

@jonemo jonemo merged commit f720ad4 into develop Jan 18, 2023
@nateprewitt nateprewitt deleted the pytest-asyncio-update branch January 18, 2023 21:15
dlm6693 pushed a commit that referenced this pull request Jan 19, 2023
* bump pytest and pytest-asyncio to latest versions

* regenerate lockfiles

* remove unnecessary @pytest.mark.asyncio (we have asyncio_mode=auto)

* fix endpoint provider unit test

* equality operator for URI objects

* update pants and ignore W503 which conflicts with black
dlm6693 pushed a commit that referenced this pull request Jan 23, 2023
* bump pytest and pytest-asyncio to latest versions

* regenerate lockfiles

* remove unnecessary @pytest.mark.asyncio (we have asyncio_mode=auto)

* fix endpoint provider unit test

* equality operator for URI objects

* update pants and ignore W503 which conflicts with black
@jonemo jonemo mentioned this pull request Jan 24, 2023
dlm6693 pushed a commit that referenced this pull request Feb 7, 2023
* bump pytest and pytest-asyncio to latest versions

* regenerate lockfiles

* remove unnecessary @pytest.mark.asyncio (we have asyncio_mode=auto)

* fix endpoint provider unit test

* equality operator for URI objects

* update pants and ignore W503 which conflicts with black
dlm6693 pushed a commit that referenced this pull request Feb 7, 2023
* bump pytest and pytest-asyncio to latest versions

* regenerate lockfiles

* remove unnecessary @pytest.mark.asyncio (we have asyncio_mode=auto)

* fix endpoint provider unit test

* equality operator for URI objects

* update pants and ignore W503 which conflicts with black
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.

2 participants