Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.7k
Comments
✅ Simplify tests for response_model#14062
✅ Simplify tests for response_model#14062tiangolo merged 1 commit intofastapi:masterfromdynamicy:tests-tutorial-tasks
Conversation
svlandeg commentedSep 11, 2025
Hi! Could you look into the failing tests for Python 3.8? |
pytest.mark.parametrizedynamicy commentedSep 12, 2025
Hi@svlandeg, |
pytest.mark.parametrize
svlandeg left a comment
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 for looking into this,@dynamicy!
I'm not quite sure I understand why you're rewritingclient = TestClient(app)
to aget_client function that yields severalTestClientobjects, 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 athttps://github.com/fastapi/fastapi/pull/13197/files for instance. Theget_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 clientI 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 commentedSep 12, 2025
Thanks for the feedback,@svlandeg! Thanks for pointing me to the right direction! |
svlandeg left a comment
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.
This now looks good to me, thanks@dynamicy !
svlandeg commentedSep 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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. |
dynamicy commentedSep 17, 2025
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok cool, thanks for the explanation! Had another look and confirmed all is still good to merge here 😎
tiangolo left a comment
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.
Nice, thanks! 🚀
And thanks@svlandeg for the review and help! ☕
11d424c intofastapi:masterUh oh!
There was an error while loading.Please reload this page.
#13150