Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

More tests for the simple_* methods #16596

Merged
merged 15 commits into from
Nov 7, 2023
Merged

More tests for the simple_* methods #16596

merged 15 commits into from
Nov 7, 2023

Conversation

clokep
Copy link
Member

@clokep clokep commented Nov 3, 2023

After being a bit nervous about #16583, I wanted to write some tests for more of the simple_* methods of the DatabasePool.

Comment on lines -2295 to -2296
if clauses:
sql = "%s WHERE %s" % (sql, " AND ".join(clauses))
Copy link
Member Author

Choose a reason for hiding this comment

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

There's always at least 1 clause, so this is just a minor clean-up.

tests/storage/test_base.py Outdated Show resolved Hide resolved
@clokep clokep marked this pull request as ready for review November 3, 2023 16:59
@clokep clokep requested a review from a team as a code owner November 3, 2023 16:59
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Looks sensible; no objections. Not sure I understand what the difficulty is with postgres though. Shout if you want a rubber duck.

@@ -209,3 +209,63 @@ def test_delete_one(self) -> Generator["defer.Deferred[object]", object, None]:
self.mock_txn.execute.assert_called_with(
"DELETE FROM tablename WHERE keycol = ?", ["Go away"]
)

@defer.inlineCallbacks
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess twisted/trial doesn't let you write test methods using async def ...? :(

Copy link
Member Author

Choose a reason for hiding this comment

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

They do, but I think not on our lowest bound. I started testing it and got into some dependency hell and stopped.

tests/storage/test_base.py Show resolved Hide resolved
tests/storage/test_base.py Outdated Show resolved Hide resolved
tests/storage/test_base.py Outdated Show resolved Hide resolved
tests/storage/test_base.py Outdated Show resolved Hide resolved
@clokep
Copy link
Member Author

clokep commented Nov 6, 2023

@DMRobertson Thanks for the feedback, can you take another look? I've:

  • Handled the minor feedback.
  • Added some tests for emulated upserts.
  • Also run the tests on postgres.

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thanks for tolerating my pedantry :)

@clokep clokep merged commit ec9ff38 into develop Nov 7, 2023
36 of 38 checks passed
@clokep clokep deleted the clokep/sql-tests branch November 7, 2023 14:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants