Skip to content

Fix test_tty.py tests#8065

Open
ShaharNaveh wants to merge 5 commits into
RustPython:mainfrom
ShaharNaveh:align-test-tty
Open

Fix test_tty.py tests#8065
ShaharNaveh wants to merge 5 commits into
RustPython:mainfrom
ShaharNaveh:align-test-tty

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Jun 9, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

Summary by CodeRabbit

  • Refactor
    • Enhanced file descriptor handling in the termios module's core functions (tcgetattr, tcsetattr, tcsendbreak, tcdrain, tcflush, tcflow) for improved robustness.

@coderabbitai

coderabbitai Bot commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 007dec0a-6c72-4167-b7a9-6069ffdd06cb

📥 Commits

Reviewing files that changed from the base of the PR and between fb422da and d450567.

⛔ Files ignored due to path filters (2)
  • Lib/test/test_termios.py is excluded by !Lib/**
  • Lib/test/test_tty.py is excluded by !Lib/**
📒 Files selected for processing (2)
  • crates/stdlib/src/termios.rs
  • crates/vm/src/stdlib/_io.rs
📝 Walkthrough

Walkthrough

The PR updates RustPython's termios module to accept file descriptor objects from Python (PyObjectRef) instead of raw integers, converting them via the Fildes wrapper at function entry points. File descriptor trait support in _io.rs is refined by adding From<Fildes> for i32 and removing the unix-specific AsFd trait.

Changes

File descriptor abstraction and termios integration

Layer / File(s) Summary
File descriptor conversion traits
crates/vm/src/stdlib/_io.rs		 Adds From<Fildes> for i32 conversion and removes unix-specific AsFd trait implementation while preserving AsRawFd for raw file descriptor usage.
Termios module Python object file descriptor conversion
crates/stdlib/src/termios.rs		 Imports Fildes and updates all six ioctl functions (tcgetattr, tcsetattr, tcsendbreak, tcdrain, tcflush, tcflow) to accept PyObjectRef instead of raw integers, converting via Fildes::try_from_object at entry.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • fanninpm
  • youknowone

Poem

🐰 A rabbit hops through file descriptors,
converting Python's abstract pointers
to native integers with grace—
Fildes finds its place!
From trait and try_from dance,
termios gets a Python-y chance. 🎪

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The PR title 'Fix test_tty.py tests' describes fixing test files, but the actual changes implement functionality updates to termios ioctl wrappers to accept file descriptors as Python objects, which is foundational work enabling those tests to pass. Clarify whether the title should emphasize the core change (termios fd conversion to PyObjectRef) or the test-fixing outcome; current title suggests only test fixes without revealing the underlying implementation changes.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] test: cpython/Lib/test/test_termios.py (TODO: 5)

dependencies:

dependent tests: (7 tests)

  • termios: test_getpass
    • getpass:
      • imaplib: test_imaplib
    • tty: test_asyncio test_pty test_sundry
      • pty: test_builtin test_ioctl

[x] lib: cpython/Lib/tty.py
[x] test: cpython/Lib/test/test_tty.py

dependencies:

  • tty

dependent tests: (5 tests)

  • tty: test_asyncio test_pty test_sundry
    • pty: test_builtin test_ioctl

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@ShaharNaveh ShaharNaveh marked this pull request as ready for review June 9, 2026 12:58
@ShaharNaveh

ShaharNaveh commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

related to #7611

@fanninpm fanninpm mentioned this pull request Jun 10, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ShaharNaveh