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

EDU-3510: Python: Added paragraph that warns about pytest.fixture(scope="session") with test server #3113

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

edmondop
Copy link

What does this PR do?

Addresses #3111 . When running tests in a project I noticed:

  • Time skipping wasn't working even if it was enabled
  • samples-python test never reuse the same workflow environment with timeskipping

This additional paragraph should warn developers familiar with pytest.fixtures not to use the scope session for caching the workflow environment

@edmondop edmondop requested a review from a team as a code owner September 28, 2024 16:20
@fairlydurable
Copy link
Contributor

fairlydurable commented Oct 10, 2024

Created SDK-2941 to review

@fairlydurable fairlydurable changed the title Python: Added paragraph that warns about pytest.fixture(scope="session") with test server EDU-3510: Python: Added paragraph that warns about pytest.fixture(scope="session") with test server Nov 18, 2024
docs/develop/python/testing-suite.mdx Outdated Show resolved Hide resolved


# This might break because it will reuse the workflow environment
pytest.fixture(scope="session")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pytest.fixture(scope="session")
@pytest.fixture(scope="session")

If you do keep this, need @

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching this!

docs/develop/python/testing-suite.mdx Outdated Show resolved Hide resolved
@fairlydurable
Copy link
Contributor

fairlydurable commented Dec 2, 2024

Thanks, @cretz. Back to you @edmondop

@edmondop
Copy link
Author

edmondop commented Dec 2, 2024

I have accepted all @cretz suggestion, thank you @cretz . I see two workflows are not passing, do I need to do something else?

docs/develop/python/testing-suite.mdx Outdated Show resolved Hide resolved
# environment will be created for each test.
@pytest.fixture
def workflow_environment():
return WorkflowEnvironment.start_time_skipping()
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this works without @pytest_asyncio.fixture and async and await. Have you tested this code? And for many, they may prefer a "session" scoped start_local instead of time skipping even though this only demonstrates time skipping.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good point, was using the auto-mode in my tests https://pytest-asyncio.readthedocs.io/en/latest/reference/configuration.html but I agree that being explicit works better

@fairlydurable
Copy link
Contributor

@edmondop Thanks for all your work on this. Please ping me and let me know when it is ready to move forward. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants