-
Notifications
You must be signed in to change notification settings - Fork 314
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
Switch to native pytest in tests/utils #1387
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left two minor comments. Please feel free to address them but no need for another review round in any case. LGTM
tests/utils/convert_test.py
Outdated
convert.to_bool(value) | ||
self.assertEqual("Cannot convert [%s] to bool." % value, ctx.exception.args[0]) | ||
assert "Cannot convert [%s] to bool." % value == ctx.exception.args[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also switch to f-strings here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention is to this with pyupgrade at some point. Fixed here, though!
tests/utils/io_test.py
Outdated
self.assertEqual(os.path.expanduser("~"), io.normalize_path("~/Documents/..")) | ||
assert io.normalize_path("/already/a/normalized/path") == "/already/a/normalized/path" | ||
assert io.normalize_path("/not/normalized/path/../") == "/not/normalized" | ||
assert os.path.expanduser("~") == io.normalize_path("~/Documents/..") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we reverse the order here so the call to io.normalize_path
comes first? Then this is consistent with the other assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks
There are two different commits, which should help with the review.