Skip to content

PYTHON-5392 Better test assertions for comparisons #2350

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

PYTHON-5392 Better test assertions for comparisons #2350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87086ab
better comparison assertions
sleepyStick May 23, 2025
cbc463b
accidentally left this one out!
sleepyStick May 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions test/asynchronous/test_bulk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ async def test_write_concern_failure_ordered(self):

# When talking to legacy servers there will be a
# write concern error for each operation.
self.assertTrue(len(details["writeConcernErrors"]) > 0)
self.assertGreater(len(details["writeConcernErrors"]), 0)

failed = details["writeConcernErrors"][0]
self.assertEqual(64, failed["code"])
Expand Down Expand Up @@ -1035,7 +1035,7 @@ async def test_write_concern_failure_ordered(self):
details,
)

self.assertTrue(len(details["writeConcernErrors"]) > 1)
self.assertGreater(len(details["writeConcernErrors"]), 1)
failed = details["writeErrors"][0]
self.assertIn("duplicate", failed["errmsg"])

Expand Down Expand Up @@ -1073,7 +1073,7 @@ async def test_write_concern_failure_unordered(self):
self.assertEqual(0, len(details["writeErrors"]))
# When talking to legacy servers there will be a
# write concern error for each operation.
self.assertTrue(len(details["writeConcernErrors"]) > 1)
self.assertGreater(len(details["writeConcernErrors"]), 1)

await self.coll.delete_many({})
await self.coll.create_index("a", unique=True)
Expand All @@ -1100,7 +1100,7 @@ async def test_write_concern_failure_unordered(self):
self.assertEqual(1, len(details["writeErrors"]))
# When talking to legacy servers there will be a
# write concern error for each operation.
self.assertTrue(len(details["writeConcernErrors"]) > 1)
self.assertGreater(len(details["writeConcernErrors"]), 1)

failed = details["writeErrors"][0]
self.assertEqual(2, failed["index"])
Expand Down
2 changes: 1 addition & 1 deletion test/asynchronous/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,7 @@ async def test_list_databases(self):
cursor = await self.client.list_databases()
self.assertIsInstance(cursor, AsyncCommandCursor)
helper_docs = await cursor.to_list()
self.assertTrue(len(helper_docs) > 0)
self.assertGreater(len(helper_docs), 0)
self.assertEqual(len(helper_docs), len(cmd_docs))
# PYTHON-3529 Some fields may change between calls, just compare names.
for helper_doc, cmd_doc in zip(helper_docs, cmd_docs):
Expand Down
2 changes: 1 addition & 1 deletion test/asynchronous/test_collection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ async def test_index_text(self):
# Sort by 'score' field.
cursor.sort([("score", {"$meta": "textScore"})])
results = await cursor.to_list()
self.assertTrue(results[0]["score"] >= results[1]["score"])
self.assertGreaterEqual(results[0]["score"], results[1]["score"])

await db.test.drop_indexes()

Expand Down
2 changes: 1 addition & 1 deletion test/asynchronous/test_database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ async def test_check_exists(self):
listener.reset()
await db.drop_collection("unique")
await db.create_collection("unique", check_exists=False)
self.assertTrue(len(listener.started_events) > 0)
self.assertGreater(len(listener.started_events), 0)
self.assertNotIn("listCollections", listener.started_command_names())

async def test_list_collections(self):
Expand Down
9 changes: 5 additions & 4 deletions test/asynchronous/test_pooling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,9 @@ async def test_wait_queue_timeout(self):
pass

duration = time.time() - start
self.assertTrue(
abs(wait_queue_timeout - duration) < 1,
self.assertLess(
abs(wait_queue_timeout - duration),
1,
f"Waited {duration:.2f} seconds for a socket, expected {wait_queue_timeout:f}",
)

Expand Down Expand Up @@ -547,7 +548,7 @@ async def f():

await async_joinall(tasks)
self.assertEqual(ntasks, self.n_passed)
self.assertTrue(len(cx_pool.conns) > 1)
self.assertGreater(len(cx_pool.conns), 1)
self.assertEqual(0, cx_pool.requests)

async def test_max_pool_size_none(self):
Expand Down Expand Up @@ -578,7 +579,7 @@ async def f():

await async_joinall(tasks)
self.assertEqual(ntasks, self.n_passed)
self.assertTrue(len(cx_pool.conns) > 1)
self.assertGreater(len(cx_pool.conns), 1)
self.assertEqual(cx_pool.max_pool_size, float("inf"))

async def test_max_pool_size_zero(self):
Expand Down
102 changes: 51 additions & 51 deletions test/test_bson.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,46 +1045,46 @@ def test_exception_wrapping(self):

def test_minkey_maxkey_comparison(self):
# MinKey's <, <=, >, >=, !=, and ==.
self.assertTrue(MinKey() < None)
self.assertTrue(MinKey() < 1)
self.assertTrue(MinKey() <= 1)
self.assertTrue(MinKey() <= MinKey())
self.assertFalse(MinKey() > None)
self.assertFalse(MinKey() > 1)
self.assertFalse(MinKey() >= 1)
self.assertTrue(MinKey() >= MinKey())
self.assertTrue(MinKey() != 1)
self.assertFalse(MinKey() == 1)
self.assertTrue(MinKey() == MinKey())
self.assertLess(MinKey(), None)
self.assertLess(MinKey(), 1)
self.assertLessEqual(MinKey(), 1)
self.assertLessEqual(MinKey(), MinKey())
self.assertLessEqual(MinKey(), None)
self.assertLessEqual(MinKey(), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to preserve the operation that each test is verifying. This test should be an assertGreater, not an assertLessEqual. The same holds for any other tests that have had their operations changed to preserve the original test case. The value should change so the test accurately reflects what operation is being verified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to clarify, instead of being assertFalse(a > b) it should beassertGreater(b, a), correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, in this case MinKey and MaxKey define their ge and such methods to be different. @ShaneHarvey can you offer the reasoning behind these tests?

self.assertLess(MinKey(), 1)
self.assertGreaterEqual(MinKey(), MinKey())
self.assertNotEqual(MinKey(), 1)
self.assertNotEqual(MinKey(), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need duplicate assertions: the case above checks !=, and the case below checks ==.

self.assertEqual(MinKey(), MinKey())

# MinKey compared to MaxKey.
self.assertTrue(MinKey() < MaxKey())
self.assertTrue(MinKey() <= MaxKey())
self.assertFalse(MinKey() > MaxKey())
self.assertFalse(MinKey() >= MaxKey())
self.assertTrue(MinKey() != MaxKey())
self.assertFalse(MinKey() == MaxKey())
self.assertLess(MinKey(), MaxKey())
self.assertLessEqual(MinKey(), MaxKey())
self.assertLessEqual(MinKey(), MaxKey())
self.assertLess(MinKey(), MaxKey())
self.assertNotEqual(MinKey(), MaxKey())
self.assertNotEqual(MinKey(), MaxKey())

# MaxKey's <, <=, >, >=, !=, and ==.
self.assertFalse(MaxKey() < None)
self.assertFalse(MaxKey() < 1)
self.assertFalse(MaxKey() <= 1)
self.assertTrue(MaxKey() <= MaxKey())
self.assertTrue(MaxKey() > None)
self.assertTrue(MaxKey() > 1)
self.assertTrue(MaxKey() >= 1)
self.assertTrue(MaxKey() >= MaxKey())
self.assertTrue(MaxKey() != 1)
self.assertFalse(MaxKey() == 1)
self.assertTrue(MaxKey() == MaxKey())
self.assertGreaterEqual(MaxKey(), None)
self.assertGreaterEqual(MaxKey(), 1)
self.assertGreater(MaxKey(), 1)
self.assertLessEqual(MaxKey(), MaxKey())
self.assertGreater(MaxKey(), None)
self.assertGreater(MaxKey(), 1)
self.assertGreaterEqual(MaxKey(), 1)
self.assertGreaterEqual(MaxKey(), MaxKey())
self.assertNotEqual(MaxKey(), 1)
self.assertNotEqual(MaxKey(), 1)
self.assertEqual(MaxKey(), MaxKey())

# MaxKey compared to MinKey.
self.assertFalse(MaxKey() < MinKey())
self.assertFalse(MaxKey() <= MinKey())
self.assertTrue(MaxKey() > MinKey())
self.assertTrue(MaxKey() >= MinKey())
self.assertTrue(MaxKey() != MinKey())
self.assertFalse(MaxKey() == MinKey())
self.assertGreaterEqual(MaxKey(), MinKey())
self.assertGreater(MaxKey(), MinKey())
self.assertGreater(MaxKey(), MinKey())
self.assertGreaterEqual(MaxKey(), MinKey())
self.assertNotEqual(MaxKey(), MinKey())
self.assertNotEqual(MaxKey(), MinKey())

def test_minkey_maxkey_hash(self):
self.assertEqual(hash(MaxKey()), hash(MaxKey()))
Expand All @@ -1094,25 +1094,25 @@ def test_minkey_maxkey_hash(self):
def test_timestamp_comparison(self):
# Timestamp is initialized with time, inc. Time is the more
# significant comparand.
self.assertTrue(Timestamp(1, 0) < Timestamp(2, 17))
self.assertTrue(Timestamp(2, 0) > Timestamp(1, 0))
self.assertTrue(Timestamp(1, 7) <= Timestamp(2, 0))
self.assertTrue(Timestamp(2, 0) >= Timestamp(1, 1))
self.assertTrue(Timestamp(2, 0) <= Timestamp(2, 0))
self.assertTrue(Timestamp(2, 0) >= Timestamp(2, 0))
self.assertFalse(Timestamp(1, 0) > Timestamp(2, 0))
self.assertLess(Timestamp(1, 0), Timestamp(2, 17))
self.assertGreater(Timestamp(2, 0), Timestamp(1, 0))
self.assertLessEqual(Timestamp(1, 7), Timestamp(2, 0))
self.assertGreaterEqual(Timestamp(2, 0), Timestamp(1, 1))
self.assertLessEqual(Timestamp(2, 0), Timestamp(2, 0))
self.assertGreaterEqual(Timestamp(2, 0), Timestamp(2, 0))
self.assertLessEqual(Timestamp(1, 0), Timestamp(2, 0))

# Comparison by inc.
self.assertTrue(Timestamp(1, 0) < Timestamp(1, 1))
self.assertTrue(Timestamp(1, 1) > Timestamp(1, 0))
self.assertTrue(Timestamp(1, 0) <= Timestamp(1, 0))
self.assertTrue(Timestamp(1, 0) <= Timestamp(1, 1))
self.assertFalse(Timestamp(1, 0) >= Timestamp(1, 1))
self.assertTrue(Timestamp(1, 0) >= Timestamp(1, 0))
self.assertTrue(Timestamp(1, 1) >= Timestamp(1, 0))
self.assertFalse(Timestamp(1, 1) <= Timestamp(1, 0))
self.assertTrue(Timestamp(1, 0) <= Timestamp(1, 0))
self.assertFalse(Timestamp(1, 0) > Timestamp(1, 0))
self.assertLess(Timestamp(1, 0), Timestamp(1, 1))
self.assertGreater(Timestamp(1, 1), Timestamp(1, 0))
self.assertLessEqual(Timestamp(1, 0), Timestamp(1, 0))
self.assertLessEqual(Timestamp(1, 0), Timestamp(1, 1))
self.assertLess(Timestamp(1, 0), Timestamp(1, 1))
self.assertGreaterEqual(Timestamp(1, 0), Timestamp(1, 0))
self.assertGreaterEqual(Timestamp(1, 1), Timestamp(1, 0))
self.assertGreater(Timestamp(1, 1), Timestamp(1, 0))
self.assertLessEqual(Timestamp(1, 0), Timestamp(1, 0))
self.assertLessEqual(Timestamp(1, 0), Timestamp(1, 0))

def test_timestamp_highorder_bits(self):
doc = {"a": Timestamp(0xFFFFFFFF, 0xFFFFFFFF)}
Expand Down
8 changes: 4 additions & 4 deletions test/test_bulk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,7 +992,7 @@ def test_write_concern_failure_ordered(self):

# When talking to legacy servers there will be a
# write concern error for each operation.
self.assertTrue(len(details["writeConcernErrors"]) > 0)
self.assertGreater(len(details["writeConcernErrors"]), 0)

failed = details["writeConcernErrors"][0]
self.assertEqual(64, failed["code"])
Expand Down Expand Up @@ -1033,7 +1033,7 @@ def test_write_concern_failure_ordered(self):
details,
)

self.assertTrue(len(details["writeConcernErrors"]) > 1)
self.assertGreater(len(details["writeConcernErrors"]), 1)
failed = details["writeErrors"][0]
self.assertIn("duplicate", failed["errmsg"])

Expand Down Expand Up @@ -1069,7 +1069,7 @@ def test_write_concern_failure_unordered(self):
self.assertEqual(0, len(details["writeErrors"]))
# When talking to legacy servers there will be a
# write concern error for each operation.
self.assertTrue(len(details["writeConcernErrors"]) > 1)
self.assertGreater(len(details["writeConcernErrors"]), 1)

self.coll.delete_many({})
self.coll.create_index("a", unique=True)
Expand All @@ -1096,7 +1096,7 @@ def test_write_concern_failure_unordered(self):
self.assertEqual(1, len(details["writeErrors"]))
# When talking to legacy servers there will be a
# write concern error for each operation.
self.assertTrue(len(details["writeConcernErrors"]) > 1)
self.assertGreater(len(details["writeConcernErrors"]), 1)

failed = details["writeErrors"][0]
self.assertEqual(2, failed["index"])
Expand Down
2 changes: 1 addition & 1 deletion test/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,7 +978,7 @@ def test_list_databases(self):
cursor = self.client.list_databases()
self.assertIsInstance(cursor, CommandCursor)
helper_docs = cursor.to_list()
self.assertTrue(len(helper_docs) > 0)
self.assertGreater(len(helper_docs), 0)
self.assertEqual(len(helper_docs), len(cmd_docs))
# PYTHON-3529 Some fields may change between calls, just compare names.
for helper_doc, cmd_doc in zip(helper_docs, cmd_docs):
Expand Down
2 changes: 1 addition & 1 deletion test/test_collection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ def test_index_text(self):
# Sort by 'score' field.
cursor.sort([("score", {"$meta": "textScore"})])
results = cursor.to_list()
self.assertTrue(results[0]["score"] >= results[1]["score"])
self.assertGreaterEqual(results[0]["score"], results[1]["score"])

db.test.drop_indexes()

Expand Down
2 changes: 1 addition & 1 deletion test/test_database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ def test_check_exists(self):
listener.reset()
db.drop_collection("unique")
db.create_collection("unique", check_exists=False)
self.assertTrue(len(listener.started_events) > 0)
self.assertGreater(len(listener.started_events), 0)
self.assertNotIn("listCollections", listener.started_command_names())

def test_list_collections(self):
Expand Down
2 changes: 1 addition & 1 deletion test/test_objectid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ def test_generation_time(self):

self.assertEqual(utc, d2.tzinfo)
d2 = d2.replace(tzinfo=None)
self.assertTrue(d2 - d1 < datetime.timedelta(seconds=2))
self.assertLess(d2 - d1, datetime.timedelta(seconds=2))

def test_from_datetime(self):
d = datetime.datetime.now(tz=datetime.timezone.utc).replace(tzinfo=None)
Expand Down
9 changes: 5 additions & 4 deletions test/test_pooling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,9 @@ def test_wait_queue_timeout(self):
pass

duration = time.time() - start
self.assertTrue(
abs(wait_queue_timeout - duration) < 1,
self.assertLess(
abs(wait_queue_timeout - duration),
1,
f"Waited {duration:.2f} seconds for a socket, expected {wait_queue_timeout:f}",
)

Expand Down Expand Up @@ -545,7 +546,7 @@ def f():

joinall(tasks)
self.assertEqual(ntasks, self.n_passed)
self.assertTrue(len(cx_pool.conns) > 1)
self.assertGreater(len(cx_pool.conns), 1)
self.assertEqual(0, cx_pool.requests)

def test_max_pool_size_none(self):
Expand Down Expand Up @@ -576,7 +577,7 @@ def f():

joinall(tasks)
self.assertEqual(ntasks, self.n_passed)
self.assertTrue(len(cx_pool.conns) > 1)
self.assertGreater(len(cx_pool.conns), 1)
self.assertEqual(cx_pool.max_pool_size, float("inf"))

def test_max_pool_size_zero(self):
Expand Down
Loading