Skip to content

Python Unittests: Get more tests running#7569

Open
feliwir wants to merge 2 commits intoRustPython:mainfrom
feliwir:unittests
Open

Python Unittests: Get more tests running#7569
feliwir wants to merge 2 commits intoRustPython:mainfrom
feliwir:unittests

Conversation

@feliwir
Copy link
Copy Markdown

@feliwir feliwir commented Apr 8, 2026 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Bug Fixes
    • Standardized error messages across iterable and container operations. Error reporting is now more consistent and descriptive, providing clearer feedback when operations fail on non-iterable objects or containers.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 8, 2026 •
edited
Loading

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: 1fd2b6f8-f444-4bf3-8e2f-654290765f17

📥 Commits

Reviewing files that changed from the base of the PR and between d5a90e5 and c697f27.

⛔ Files ignored due to path filters (2)
  • Lib/test/datetimetester.py is excluded by !Lib/**
  • Lib/test/test_contains.py is excluded by !Lib/**
📒 Files selected for processing (3)
  • crates/stdlib/src/csv.rs
  • crates/vm/src/function/protocol.rs
  • crates/vm/src/protocol/iter.rs
📝 Walkthrough

Walkthrough

Three files updated to standardize error messages for iterator and container conversion failures. The error message format changed from "'<class>' object is not iterable" to "argument of type '<class>' is not a container or iterable" across CSV, protocol, and iteration modules, maintaining identical exception handling logic.

Changes

Cohort / File(s) Summary
Iterator Error Message Standardization
crates/stdlib/src/csv.rs, crates/vm/src/function/protocol.rs, crates/vm/src/protocol/iter.rs		 Unified error messages for container/iterable type conversion failures from "'<class>' object is not iterable" to "argument of type '<class>' is not a container or iterable" across CSV writer, function protocol, and iteration protocol modules.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

Poem

🐰 Error messages now align with grace,
Once scattered, now they share one space,
"Not a container!" the rabbit does cry,
With harmonized strings across modules so high,
Consistency hops through the code—hooray!

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% 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 title 'Python Unittests: Get more tests running' is vague and generic, using non-descriptive terms that don't convey the actual changes made to error messages in three files. Consider revising the title to reflect the actual changes, such as 'Improve error messages for non-iterable type checks' or 'Update iterator protocol error messages for clarity'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026 •
edited
Loading

📦 Library Dependencies

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

[x] test: cpython/Lib/test/test_contains.py

dependencies:

dependent tests: (no tests depend on contains)

Legend:

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

ShaharNaveh
ShaharNaveh approved these changes Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

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

tysm!

Please run cargo fmt --all, this should resolve CI lint failures

ShaharNaveh
ShaharNaveh reviewed Apr 8, 2026
View reviewed changes
Lib/test/datetimetester.py
Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh Apr 8, 2026

Choose a reason for hiding this comment

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

I'd extract this changes to a separate PR as the problematic change seems to be with the modifications of the error message

Copy link
Copy Markdown
Author

@feliwir feliwir Apr 8, 2026

Choose a reason for hiding this comment

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

I'll clean this up later today / tomorrow.

@feliwir feliwir marked this pull request as ready for review April 8, 2026 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShaharNaveh ShaharNaveh ShaharNaveh approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@feliwir @ShaharNaveh