Fix TestSpec serialization silently dropping test ids that contain quotes - #9
Fix TestSpec serialization silently dropping test ids that contain quotes#9eeshsaxena wants to merge 1 commit into
Conversation
to_fail_to_pass_str and to_pass_to_pass_str built the list string by hand with single quotes around each test id. A test id containing a quote (for example a parametrized pytest id like test_x[can't]) produced invalid Python, so when load_task_from_directory parses instance_info.txt and evaluates the string it raises, falls into the except branch, and silently replaces the whole list with []. The task then loses its fail_to_pass/pass_to_pass spec. Use repr(list(...)) so any test id is emitted as a valid literal that round trips. Output is unchanged for ids without quotes. Adds tests for the round trip (common, quoted, and empty cases) plus a pytest dev dependency and config, since the repo had no test setup yet.
📝 WalkthroughWalkthroughThe project adds pytest development configuration. ChangesTestSpec serialization
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_wizard_models.py (1)
27-28: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the production
instance_info.txtparser.The assertions at Line 27 and Line 28 call
ast.literal_evaldirectly. The reported failure occurs in the parser read path, where invalid literals can become empty lists. Add a test through that parser for both quoted fields to verify the complete round trip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_wizard_models.py` around lines 27 - 28, Replace the direct ast.literal_eval assertions in the test with coverage through the production instance_info.txt parser, exercising both quoted fields and verifying their complete round-trip values. Ensure the test captures the parser read path, including preservation of expected values rather than silently converting invalid literals to empty lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_wizard_models.py`:
- Around line 27-28: Replace the direct ast.literal_eval assertions in the test
with coverage through the production instance_info.txt parser, exercising both
quoted fields and verifying their complete round-trip values. Ensure the test
captures the parser read path, including preservation of expected values rather
than silently converting invalid literals to empty lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ce76f4e-54dc-4bea-8ec4-9c410a7d585a
📒 Files selected for processing (3)
pyproject.tomlsrc/anvil/wizard/models.pytests/test_wizard_models.py
|
Hi! Gentle nudge on this one whenever you have some bandwidth. It's a small, self-contained fix ( |
TestSpec.to_fail_to_pass_str/to_pass_to_pass_strbuild the list string by hand, wrapping each test id in single quotes:If a test id contains a quote, the result is invalid Python. Parametrized pytest ids do this in practice, e.g.
tests/test_x.py::test_quote[can't]becomes['tests/test_x.py::test_quote[can't]'].load_task_from_directorywrites these intoinstance_info.txtand later parses them back witheval(). When the string is malformed theexcept Exceptionbranch runs, prints a warning, and replaces the whole list with[], so the task silently loses itsfail_to_pass/pass_to_passspec and is evaluated against no tests.Using
repr(list(...))emits a valid literal for any test id, so it round-trips. Output is byte-for-byte identical for ids without quotes, so existing datasets are unaffected.Added
tests/test_wizard_models.pycovering the common, quoted, and empty cases; the quoted case fails onmain(the parser raisesSyntaxError) and passes here. The repo had no test setup, so this also adds pytest as a dev dependency with a small[tool.pytest.ini_options]block.Summary by CodeRabbit
Bug Fixes
Tests