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

Add helpers to better handle session in tests #33307

Closed
wants to merge 1 commit into from

Conversation

uranusjr
Copy link
Member

In various places we intentionally create dangling sessions in the worker, but when running a task in tests (which don't use workers) we need to make those not dangling.

@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Aug 11, 2023
@potiuk potiuk closed this Aug 11, 2023
@potiuk potiuk reopened this Aug 11, 2023
In various places we intentionally create dangling sessions in the
worker, but when running a task in tests (which don't use workers) we
need to make those not dangling.
@uranusjr uranusjr force-pushed the no-leak-session-in-tests branch from 5887049 to d4933a2 Compare August 11, 2023 06:19
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 26, 2023
@uranusjr
Copy link
Member Author

@potiuk I forgot the context on this, is this still needed after we have the cleanup context manager thing?

@github-actions github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 27, 2023
@potiuk
Copy link
Member

potiuk commented Oct 8, 2023

No. No need. We are now (for a long time) closing all Sqlalchemy sessions between tests, so this is not needed even without context manager I think

@uranusjr uranusjr closed this Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants