✅ Simplify tests for response_model #14062
Conversation
dynamicy
force-pushed
the
tests-tutorial-tasks
branch
3 times, most recently
from
31b613b to
4aeae73
Compare
|
Hi! Could you look into the failing tests for Python 3.8? |
svlandeg
changed the title
pytest.mark.parametrize
dynamicy
force-pushed
the
tests-tutorial-tasks
branch
from
4aeae73 to
c257eac
Compare
|
Hi @svlandeg, |
svlandeg
changed the title
pytest.mark.parametrize
There was a problem hiding this comment.
Thanks for looking into this, @dynamicy!
I'm not quite sure I understand why you're rewriting
client = TestClient(app)
to a get_client function that yields several TestClientobjects, even when there's only one. In fact, I don't think we need to change the test files that don't have variants of them (which is the focus of #13150)
Have a look at https://github.com/fastapi/fastapi/pull/13197/files for instance. The get_client method becomes something like this:
@pytest.fixture(
name="client",
params=[
"tutorial001",
pytest.param("tutorial001_py310", marks=needs_py310),
],
)
def get_client(request: pytest.FixtureRequest):
mod = importlib.import_module(f"docs_src.schema_extra_example.{request.param}")
client = TestClient(mod.app)
return client
I suggest to keep to this style for consistency across the code base. Then most of the edits of this PR can be removed, and we can focus on the files that actually have code variants for Python 3.10 and clean those up.
dynamicy
force-pushed
the
tests-tutorial-tasks
branch
5 times, most recently
from
a25d3d3 to
a5bd18c
Compare
|
Thanks for the feedback, @svlandeg! Thanks for pointing me to the right direction! |
dynamicy
force-pushed
the
tests-tutorial-tasks
branch
from
a5bd18c to
8ca56ee
Compare
|
@dynamicy: this was already approved and ready to merge, but now I see that a change has been force-pushed. I generally dislike force pushes because it doesn't allow a reviewer to track the history. What have you changed on this PR since my approval? Just to clarify: there's really no need to obliterate the history of a PR. We squash right before merging anyway. |
|
Thanks for the heads-up! GitHub showed a notice that the branch was out of date with the base, so I used the Rebase and update option in the UI. That resulted in a force-push of the rebased commits. |
3 participants
#13150