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

Remove erroneous select_for_update and update tests to use transactions #1066

Merged
merged 3 commits into from
May 2, 2022

Conversation

jjnesbitt
Copy link
Member

Closes #1064

To verify that the updated tests do catch this kind of error, you can roll back 98c1158 and see that the bug is now caught by tests.

@jjnesbitt
Copy link
Member Author

Tests will fail due to #1065

Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

LGTM
It's unfortunate that you have to manually specify transaction=True if a transaction is used in the code being tested.... I could see this being a problem again in the future.

@jjnesbitt
Copy link
Member Author

LGTM It's unfortunate that you have to manually specify transaction=True if a transaction is used in the code being tested.... I could see this being a problem again in the future.

I agree, but I also don't think it's a good idea to try to default to using transactions, since they'd slow down our already not that fast tests. We'll probably have to keep our eye on this.

@jjnesbitt jjnesbitt merged commit ba147be into master May 2, 2022
@jjnesbitt jjnesbitt deleted the 1064-select-for-update branch May 2, 2022 16:51
@dandibot
Copy link
Member

dandibot commented May 5, 2022

🚀 PR was released in v0.2.20 🚀

@dandibot dandibot added the released This issue/pull request has been released. label May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TransactionManagementError: select_for_update cannot be used outside of a transaction.
3 participants