Skip to content
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

Bump pytest and pytest-asyncio #344

Merged
merged 8 commits into from
Nov 23, 2024
Merged

Bump pytest and pytest-asyncio #344

merged 8 commits into from
Nov 23, 2024

Conversation

PGijsbers
Copy link
Collaborator

@PGijsbers PGijsbers commented Jul 25, 2024

SubRequest was removed, but it was really used to denote a FixtureRequest, so I updated that part and now everything works on the latest versions again.

edit: with pytest 8+, test ordering changed which exposed abuse of global state in tests, so I had to remove that too.

@PGijsbers PGijsbers added the pip dependencies update dependency version label Jul 25, 2024
@PGijsbers PGijsbers requested review from jsmatias and removed request for jsmatias July 25, 2024 14:53
@PGijsbers
Copy link
Collaborator Author

Whoops, I accidentally branched off the wrong branch.. rebasing.

@@ -11,6 +11,7 @@


class TestUserType(enum.Enum):
__test__ = False
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Otherwise Pytest will complain it cannot collect from this class since it has an (implicit) __init__ method.

@PGijsbers
Copy link
Collaborator Author

I am a bit confused why some tests failed.. they pass locally. Trying to look into it. Have you experienced this before? Are there flaky tests (on CI)?

@PGijsbers
Copy link
Collaborator Author

Closing and reopening to see if that triggers CI.

@PGijsbers PGijsbers closed this Nov 19, 2024
@PGijsbers PGijsbers reopened this Nov 19, 2024
@PGijsbers
Copy link
Collaborator Author

PGijsbers commented Nov 19, 2024

The test fails when run in the entire suite, not when run in isolation. Have narrowed down to the specific version change of pytest 7.4.4->8.0.0 with pytest-asyncio fixed at 2.3.8.

@PGijsbers
Copy link
Collaborator Author

I'm still not entirely sure what's happening, it looks like a different responses mock is used than intended in the authentication tests... and the tests results depend on execution order. I started down this path since it was a breaking change of pytest 8.0.0, verified by renaming test_authorization.py to auth_test.py so it is executed earlier (alphabetical order..), we verify the tests do work then. So the tests somehow violate independence, but I am not sure yet how since the auth tests seem OK (all work is within the scope of a context manager).

@PGijsbers
Copy link
Collaborator Author

The problem was practically every other test modifying global state.
I patched it for now to where they at least revert the change after the test is ran, but perhaps we should redesign the use of global variables. That would be a separate issue.

@PGijsbers PGijsbers requested review from Taniya-Das and removed request for jsmatias and Taniya-Das November 20, 2024 07:59
Copy link
Collaborator

@Taniya-Das Taniya-Das left a comment

Choose a reason for hiding this comment

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

Looks correct to me.

@PGijsbers PGijsbers merged commit 7c4a80e into develop Nov 23, 2024
4 checks passed
@PGijsbers PGijsbers deleted the bump-pytest branch November 23, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pip dependencies update dependency version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants