Skip to content
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

gh-130524 Create WinGetpassTest class for improved coverage #130529

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

floor-licker
Copy link

@floor-licker floor-licker commented Feb 25, 2025

Improved Getpass coverage for Windows

I followed the same pattern as the Unix tests, the new class tests the core functionality of win_getpass using msvcrt.getch for input, verifies proper handling of special characters including keyboard interrupt, ensures stream flushing works correctly, and a test for fallback_getpass() to test the mscvrt fallback mechanism when the primary method fails.

@bedevere-app bedevere-app bot added the tests Tests in the Lib/test dir label Feb 25, 2025
@bedevere-app
Copy link

bedevere-app bot commented Feb 25, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link

bedevere-app bot commented Feb 25, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link

bedevere-app bot commented Feb 25, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@floor-licker
Copy link
Author

the macOS tests are failing which is odd given the @unittest.skipUnless(sys.platform.startswith("win"), "requires Windows") outer-decorator of the class. Let me try something else.

@bedevere-app
Copy link

bedevere-app bot commented Feb 25, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@donBarbos
Copy link
Contributor

donBarbos commented Feb 25, 2025

@floor-licker
before sending changes you can try to run tests using next command (I assume you are using Windows):

./python Lib/test/test_getpass.py

and also please don't forget to add news entry file (using blurb) at least when ci requires it, they should be passed.

you can read the errors in the github actions logs, everything is described there (you just don't define the objects you use)

@bedevere-app
Copy link

bedevere-app bot commented Feb 25, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link

bedevere-app bot commented Feb 25, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@donBarbos
Copy link
Contributor

BTW, please do not force-push; it makes reviewing harder. Moreover, all commits are squashed upon merge anyway, so there's no need for the PR/branch to be cluttered with amendment commits. See also the devguide.

@floor-licker
Copy link
Author

@donBarbos my apologies for the mess, just lost access to my windows machine so I've been trying to sort this out on macOS. I'll create a partition and come up with a fix. Thanks for the info, I'll give it a read.

@picnixz
Copy link
Member

picnixz commented Feb 25, 2025

It seems that the CI is failing because the test hangs. I'm converting it to a draft until the CI is green again.

@picnixz picnixz marked this pull request as draft February 25, 2025 19:08
floor-licker and others added 2 commits February 25, 2025 18:17
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
floor-licker and others added 2 commits February 25, 2025 18:31
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: donBarbos <donbarbos@proton.me>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@picnixz
Copy link
Member

picnixz commented Mar 23, 2025

The tests still don't seem to work. Are you willing to continue working on this @floor-licker or should someone else take over (I can't because I don't have a Windows for that; maybe @donBarbos could as it's blocking #130496).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants