Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-89083: Add CLI tests for UUIDv{6,7,8}#136548
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
LamentXU123 commented Jul 11, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
picnixz left a comment
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.
Can you:
- follow PEP-8 (1 blank line between functions)
- make a separate class that is only testing the CLI functionality please?
LamentXU123 commented Jul 11, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Done. Thanks! Edited: btw, we've got tested like this: classTestUUIDWithoutExtModule(BaseTestUUID, unittest.TestCase): uuid=py_uuidIn this current PR code, this test in redundant. I suggest maybe we could combine them together (?) |
Lib/test/test_uuid.py Outdated
| self.assertIs(strong, weak()) | ||
| class TestUUIDCli(BaseTestUUID, unittest.TestCase): |
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.
Do we need all CAPS on term CLI?
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.
Just check how other classes testing CLIs are named.
picnixz left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
There is a lot of duplicated code. I would sugfgest having a single a single function that tests the expected outputs accordingly:
defdo_test_standalone_uuid(self, version): stdout=io.StringIO() withcontextlib.redirect_stdout(stdout): self.uuid.main() output=stdout.getvalue().strip() u=self.uuid.UUID(output) self.assertEqual(output, str(u)) self.assertEqual(u.version, version) @mock.patch.object(sys, "argv", ["", "-u", "uuid1"])deftest_uuid1(self): self.do_test_standalone_uuid(1)and similar stuff for v6, v7 and v8.
Uh oh!
There was an error while loading. Please reload this page.
LamentXU123 commented Jul 11, 2025
In the test case of cli we've actually already tested UUID without ext moudle (?, I'm not quiet sure) So maybe combining them together will be better? |
Lib/test/test_uuid.py Outdated
| self.assertIs(strong, weak()) | ||
| class TestUUIDCli(BaseTestUUID, unittest.TestCase): |
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.
Just check how other classes testing CLIs are named.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
picnixz commented Jul 11, 2025
It's not entirely redundant. However I see the issue. Two possibilities:
Or:
|
picnixz commented Jul 11, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Ok, actually |
LamentXU123 commented Jul 11, 2025
The test classes before are named in testUUID* (which I think would be better if we add this prefix in this func). But yes |
LamentXU123 commented Jul 11, 2025
This is better IMO. I've changed it. |
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Bénédikt Tran <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
UUIDv{6,7,8}c564847 into python:mainUh oh!
There was an error while loading. Please reload this page.
LamentXU123 commented Jul 12, 2025
Thanks! Should we backport this to 3.14? The new UUIDs are added in that version |
Thanks @LamentXU123 for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
(cherry picked from commit c564847) Co-authored-by: Weilin Du <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]>
GH-136576 is a backport of this pull request to the 3.14 branch. |
gh-89083: Add CLI tests for `UUIDv{6,7,8}` (GH-136548) (cherry picked from commit c564847) Co-authored-by: Weilin Du <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
) (python#136576) pythongh-89083: Add CLI tests for `UUIDv{6,7,8}` (pythonGH-136548) (cherry picked from commit c564847) Co-authored-by: Weilin Du <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]>
This PR add simple cli test cases to UUIDv6, v7, v8 added in #89083 , as well as a test case to UUID v1.
There are only one test cases for each, since they can't be customized through CLI.
Skipping news ;)