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.

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

Conversation

sleepyStick
Copy link
Contributor

Another subtask PR for PYTHON-5262, this one is for comparisons:
self.assertTrue(... > ...) -> self.assertGreater(..., ...)
self.assertTrue(... >= ...) -> self.assertGreaterEqual(..., ...)
self.assertTrue(... < ...) -> self.assertLess(..., ...)
self.assertTrue(... <= ...) -> self.assertLessEqual(..., ...)
self.assertFalse(... >= ...) -> self.assertLess(..., ...)
self.assertFalse(... > ...) -> self.assertLessEqual(..., ...)
self.assertFalse(... <= ...) -> self.assertGreater(..., ...)
self.assertFalse(... < ...) -> self.assertGreaterEqual(..., ...)
self.assertTrue(... == ...) -> self.assertEqual(..., ...)
self.assertTrue(... != ...) -> self.assertNotEqual(..., ...)
self.assertFalse(... == ...) -> self.assertNotEqual(..., ...)

@sleepyStick sleepyStick marked this pull request as ready for review May 23, 2025 17:40
@sleepyStick sleepyStick requested a review from NoahStapp May 23, 2025 17:40
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 ==.

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

Successfully merging this pull request may close these issues.

2 participants