-
-
Notifications
You must be signed in to change notification settings - Fork 532
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
Refactor nox to separate integration tests #2950
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2950 +/- ##
==========================================
+ Coverage 96.11% 96.54% +0.42%
==========================================
Files 211 464 +253
Lines 9244 28775 +19531
Branches 1492 3533 +2041
==========================================
+ Hits 8885 27780 +18895
- Misses 229 815 +586
- Partials 130 180 +50 |
CodSpeed Performance ReportMerging #2950 will not alter performanceComparing Summary
|
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature. Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) Here's the tweet text:
|
Apollo Federation Subgraph Compatibility Results
Learn more: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is massive but the changes are really obvious :)
LGTM 👍🏼
Btw, don't know if it is codecov that is going crazy or something wrong in this change, but codecov is reporting a |
this is because of the changes, I'm still not done (I thought I was) :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
|
||
|
||
def _get_http_client_classes() -> Generator[Any, None, None]: | ||
for client, module, marks in [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a nitpick (Open Closed Principle) I'd move the loop outside of this function and iteratively call the body of the loop to within a generator comprehension in the params argument to the fixture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you show me how you'd do it? 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm begin lazy :P)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey sorry Patrick I didn't have access to my PC when writing the comment, I just saw your message now. I'll post how to do it here when I get home 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! I forgot to ping you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something like this:
import importlib
from typing import Any, Sequence, Type
import pytest
from .clients.base import HttpClient
def _get_http_client_class(
client: str,
module: str,
marks: Sequence[pytest.MarkDecorator],
) -> Any:
try:
client_class = getattr(
importlib.import_module(f".{module}", package="tests.http.clients"),
client,
)
except ImportError:
client_class = None
return pytest.param(
client_class,
marks=[
*marks,
pytest.mark.skipif(
client_class is None, reason=f"Client {client} not found"
),
],
)
@pytest.fixture(
params=(
_get_http_client_class(client, module, marks)
for client, module, marks in [
("AioHttpClient", "aiohttp", [pytest.mark.aiohttp]),
("AsgiHttpClient", "asgi", [pytest.mark.asgi]),
("AsyncDjangoHttpClient", "async_django", [pytest.mark.django]),
("AsyncFlaskHttpClient", "async_flask", [pytest.mark.flask]),
("ChannelsHttpClient", "channels", [pytest.mark.channels]),
("ChaliceHttpClient", "chalice", [pytest.mark.chalice]),
("DjangoHttpClient", "django", [pytest.mark.django]),
("FastAPIHttpClient", "fastapi", [pytest.mark.fastapi]),
("FlaskHttpClient", "flask", [pytest.mark.flask]),
("SanicHttpClient", "sanic", [pytest.mark.sanic]),
("StarliteHttpClient", "starlite", [pytest.mark.starlite]),
(
"SyncChannelsHttpClient",
"channels",
[pytest.mark.channels, pytest.mark.django_db],
),
]
)
)
def http_client_class(request: Any) -> Type[HttpClient]:
return request.param
@pytest.fixture()
def http_client(http_client_class: Type[HttpClient]) -> HttpClient:
return http_client_class()
Or alternatively:
import importlib
from typing import Any, Generator, Sequence, Tuple, Type
import pytest
from .clients.base import HttpClient
def parametrize_clients(
specifications: Sequence[Tuple[str, str, Sequence[pytest.MarkDecorator]]],
) -> Generator[Any, None, None]:
for client, module, marks in specifications:
try:
client_class = getattr(
importlib.import_module(f".{module}", package="tests.http.clients"),
client,
)
except ImportError:
client_class = None
yield pytest.param(
client_class,
marks=[
*marks,
pytest.mark.skipif(
client_class is None, reason=f"Client {client} not found"
),
],
)
@pytest.fixture(
params=parametrize_clients(
[
("AioHttpClient", "aiohttp", [pytest.mark.aiohttp]),
("AsgiHttpClient", "asgi", [pytest.mark.asgi]),
("AsyncDjangoHttpClient", "async_django", [pytest.mark.django]),
("AsyncFlaskHttpClient", "async_flask", [pytest.mark.flask]),
("ChannelsHttpClient", "channels", [pytest.mark.channels]),
("ChaliceHttpClient", "chalice", [pytest.mark.chalice]),
("DjangoHttpClient", "django", [pytest.mark.django]),
("FastAPIHttpClient", "fastapi", [pytest.mark.fastapi]),
("FlaskHttpClient", "flask", [pytest.mark.flask]),
("SanicHttpClient", "sanic", [pytest.mark.sanic]),
("StarliteHttpClient", "starlite", [pytest.mark.starlite]),
(
"SyncChannelsHttpClient",
"channels",
[pytest.mark.channels, pytest.mark.django_db],
),
]
)
)
def http_client_class(request: Any) -> Type[HttpClient]:
return request.param
@pytest.fixture()
def http_client(http_client_class: Type[HttpClient]) -> HttpClient:
return http_client_class()
Could then re-use the same parametrize_clients
in the websockets conftest
|
||
def _get_http_client_classes() -> Generator[Any, None, None]: | ||
for client, module, marks in [ | ||
("AioHttpClient", "aiohttp", [pytest.mark.aiohttp]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nitpick here if you choose to accept the other one
* Split tests on nox * Fix marker * Improve how we skip tests * Make dependencies not required * More ignores and import fixes * More import skips * Remove aiohttp dep in tests * Fix * Remove more imports * More imports * One more import * More skips * Older pydantic when using starlite * Missing dep * Missing dep * Missing deps * cli tests * Fix federation * TMP disable cache * Reskip * Reenable ws tests * Fix more tests * Add coverage for tests * Fix import path * Fix test * Fix markers * Test fix * Fix pydantic tests * Remove unused fixtures * Run windows with poetry * Remove use * Fix command
This PR removes all integrations deps from dev dependencies
and instead adds them to a new group in poetry, you can install
all like this:
Meanwhile on CI will be able to run tests based on the individual
dependency. This should make it easier to tests with newer python versions