Skip to content

test_builtin.test_import #6546

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.

Sign up for GitHub

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

Jump to bottom
Merged
youknowone merged 2 commits into from
Dec 27, 2025
Merged

test_builtin.test_import #6546

youknowone merged 2 commits into from
Dec 27, 2025

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 26, 2025 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Bug Fixes
    • Improved warning module detection to check existing imports before returning unavailable.
    • Enhanced warning message handling to properly construct warning instances for consistent display.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 26, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

The changes modify warning module behavior in two functions: get_warnings_attr now probes sys.modules for an already-imported warnings module instead of immediately returning None, and call_show_warning now instantiates warning objects by invoking the category constructor with the message before passing them to warnings.WarningMessage.

Changes

Cohort / File(s) Summary
Warning module behavior refinement
crates/vm/src/warn.rs		 Modified get_warnings_attr to check sys.modules for pre-imported warnings module when not importing eagerly. Updated call_show_warning to create warning instances by invoking category constructor with message, then pass instance to warning message constructor.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Warnings now probe the module shelf,
Seeking what's already there itself,
Each category spawns its proper kin,
A CPython-like dance begins,
Where instances flow, not strings so thin! 🌟

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'test_builtin.test_import' is vague and does not clearly convey what the actual changes are about. The PR modifies warning handling logic in warn.rs, but the title suggests it's only about a test file or import test. Consider a more descriptive title like 'Fix warning message handling to match CPython behavior' or 'Improve warnings module detection and instantiation' to better reflect the actual changes made.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 27ab62d and 3ccf37c.

⛔ Files ignored due to path filters (6)
  • Lib/test/test_builtin.py is excluded by !Lib/**
  • Lib/test/test_import/__init__.py is excluded by !Lib/**
  • Lib/test/test_importlib/import_/test___package__.py is excluded by !Lib/**
  • Lib/test/test_importlib/import_/test_helpers.py is excluded by !Lib/**
  • Lib/test/test_importlib/import_/test_meta_path.py is excluded by !Lib/**
  • Lib/test/test_importlib/import_/test_path.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/vm/src/warn.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

  • crates/vm/src/warn.rs
🧠 Learnings (1)
📓 Common learnings 
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : When modifying test files in `Lib/test/`, use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment to mark unsupported features
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Run tests under miri
  • GitHub Check: Check the WASM package and demo
🔇 Additional comments (2)
crates/vm/src/warn.rs (2)

70-78: LGTM - Properly mirrors CPython's PyImport_GetModule behavior.

The change correctly probes sys.modules for an already-imported warnings module when eager import is not desired, rather than immediately returning None. This is appropriate behavior during VM finalization or when try_import is false, as it allows use of an already-loaded warnings module without triggering new imports.

330-336: LGTM - Correct instantiation of warning objects.

This properly creates a warning instance by invoking the category constructor with the message, matching how CPython and the Python warnings module work. WarningMessage expects a warning instance (which is a BaseException subclass), not a raw string message.

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.

@youknowone youknowone marked this pull request as ready for review December 27, 2025 02:03
@youknowone youknowone merged commit 75ecd72 into RustPython:main Dec 27, 2025
13 checks passed
@youknowone youknowone deleted the test_import branch December 27, 2025 12:38
@coderabbitai coderabbitai bot mentioned this pull request Jan 18, 2026
Merged
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

@youknowone