✅ Simplify tests for response_model#14062
✅ Simplify tests for response_model#14062tiangolo merged 1 commit intofastapi:masterfrom dynamicy:tests-tutorial-tasks
Conversation
|
Hi! Could you look into the failing tests for Python 3.8? |
pytest.mark.parametrize
|
Hi @svlandeg, |
pytest.mark.parametrize
svlandeg
left a comment
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.
|
Thanks for the feedback, @svlandeg! Thanks for pointing me to the right direction! |
|
@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. |
svlandeg
left a comment
There was a problem hiding this comment.
Ok cool, thanks for the explanation! Had another look and confirmed all is still good to merge here 😎
#13150