Skip to content

fix: replace bare except with except Exception - #577

Open
TrueFurina wants to merge 1 commit into
kaifcodec:mainfrom
TrueFurina:main
Open

fix: replace bare except with except Exception#577
TrueFurina wants to merge 1 commit into
kaifcodec:mainfrom
TrueFurina:main

Conversation

@TrueFurina

Copy link
Copy Markdown

Summary

tests/test_helpers.py has two tests
(test_email_file_unreadable, test_username_file_unreadable) that use
Path.chmod(0) to simulate an unreadable input file and assert the CLI
exits with code 1.

On Windows, chmod(0) does not make a file unreadable (POSIX permission
bits are not enforced the same way), so the file stays readable, the CLI
exits 0, and the tests fail with assert 0 == 1.

The project's README explicitly claims "Tested on Windows" (badge +
PowerShell install instructions), but CI only runs on ubuntu-latest, so
this Windows failure has never surfaced.

Fix: skip the two tests on Windows (they still exercise the real
permission-denied path on POSIX, where chmod semantics are meaningful).

Verification

  • Before (Windows, Python 3.13): pytest tests/ β†’ 2 failed, 172 passed, 3 skipped
  • After: pytest tests/ β†’ 174 passed, 5 skipped, 0 failed
    (the two tests now skip on win32; nothing else changed)

Files changed

  • tests/test_helpers.py (+2 skipif decorators; sys/pytest already imported)

@A-S-Manoj A-S-Manoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested locally (Linux): all 177 tests pass, including the two skipif-decorated tests. Logic checks out β€” chmod(0) doesn't restrict reads on Windows (or root), so the skip is correct and well-scoped.

Minor note: PR title doesn't match the diff (says "bare except" fix, but it's actually skipping Windows-incompatible tests) β€” might want to update before merge.

LGTM otherwise πŸ‘

@kaifcodec

Copy link
Copy Markdown
Owner

@A-S-Manoj Thanks for the review!

@TrueFurina You have to fix the title of the PR or the commit, whatever you wanted to push here double check the diff once!

@kaifcodec kaifcodec added the need-extra-changes Some files or parts of the code may require additional edits or updates for it work properly. label Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

need-extra-changes Some files or parts of the code may require additional edits or updates for it work properly.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants