Repository navigation
🧪 Add KeyError test for _resolve_field - #3
Conversation
Add direct unit tests for `_resolve_field` in `policyforge/engine.py`, covering the success path and both `KeyError` exception branches. This addresses the testing gap where missing nested fields in a dictionary context were not explicitly tested at the unit level. - Adds `test_resolve_field_success` - Adds `test_resolve_field_missing_key` (covers segment not in current) - Adds `test_resolve_field_non_dict_traversal` (covers traversal into non-dict) Co-authored-by: tblakex01 <17657984+tblakex01@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideAdds focused unit tests for the internal _resolve_field helper and cleans up existing YAML test fixture construction for brevity and consistency across engine, loader, and decorator tests. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthrough
ChangesField Resolution Test Coverage
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
_resolve_fieldtests assert on the exactKeyErrormessage, which makes them brittle to minor wording changes; consider matching on a shorter, more stable substring or using a custom exception type if the message format is expected to change. - There is extensive repetition of
write_text(textwrap.dedent("""..."""))blocks across tests; introducing a small helper to construct policy files from templates could reduce duplication and make the tests easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_resolve_field` tests assert on the exact `KeyError` message, which makes them brittle to minor wording changes; consider matching on a shorter, more stable substring or using a custom exception type if the message format is expected to change.
- There is extensive repetition of `write_text(textwrap.dedent("""..."""))` blocks across tests; introducing a small helper to construct policy files from templates could reduce duplication and make the tests easier to maintain.
## Individual Comments
### Comment 1
<location path="tests/test_engine.py" line_range="773-776" />
<code_context>
+ context = {"args": {"url": "https://example.com"}}
+ assert _resolve_field(context, "args.url") == "https://example.com"
+
+ def test_resolve_field_missing_key(self):
+ context = {"a": {}}
+ with pytest.raises(KeyError, match="Field 'a.b' not found at segment 'b'"):
+ _resolve_field(context, "a.b")
+
+ def test_resolve_field_non_dict_traversal(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for a missing top-level key to complement the missing nested key case
Consider adding a case where the root segment is missing (e.g., `context = {}` with path `"a.b"` or `context = {"x": 1}` with path `"a"`) to verify the error and message when resolution fails at the top level.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_resolve_field_missing_key(self): | ||
| context = {"a": {}} | ||
| with pytest.raises(KeyError, match="Field 'a.b' not found at segment 'b'"): | ||
| _resolve_field(context, "a.b") |
There was a problem hiding this comment.
suggestion (testing): Add a test for a missing top-level key to complement the missing nested key case
Consider adding a case where the root segment is missing (e.g., context = {} with path "a.b" or context = {"x": 1} with path "a") to verify the error and message when resolution fails at the top level.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/test_decorators.py (1)
1-265:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRun Black to fix formatting before merge.
The CI pipeline reports that Black would reformat this file. This is a critical blocker that must be resolved.
Run the following command to apply Black formatting:
#!/bin/bash # Apply Black formatting to the test file black tests/test_decorators.py🤖 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_decorators.py` around lines 1 - 265, The file tests/test_decorators.py is failing CI due to Black formatting; fix by running Black on that file (or repository) to apply the canonical formatter, re-stage the modified file, and push a new commit so CI can pass; targets to check include the decorator/tests referencing policy_gate, PolicyGateWrapper, and _bind_positional_args to ensure no behavioral edits were made when formatting.tests/test_loader.py (1)
1-448:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRun Black to fix formatting before merge.
The CI pipeline reports that Black would reformat this file. This is a critical blocker that must be resolved.
Run the following command to apply Black formatting:
#!/bin/bash # Apply Black formatting to the test file black tests/test_loader.py🤖 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_loader.py` around lines 1 - 448, This test module (contains classes like TestLoadFile, TestValidationEdgeCases, TestLoadTrustConfig and tests such as test_loads_single_policy and test_parses_full_block) needs to be reformatted with Black to satisfy CI; run Black on the test file (or reformat the module) so the file meets the project's Black configuration before merging.tests/test_engine.py (1)
1-784:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the Black formatting failure before merge.
CI is currently failing on
black --checkfor this file. Please run Black and commit the formatting-only changes so the lint job passes.🤖 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_engine.py` around lines 1 - 784, The file tests/test_engine.py is failing CI due to Black formatting; run the Black formatter (e.g., black tests/test_engine.py or black .) and commit the resulting formatting-only changes so the lint job passes. Locate this file and reformat the tests (including classes like TestBasicEvaluation, TestResolveField and functions referencing _resolve_field and _hash_args) then add a commit with only formatting changes.
🧹 Nitpick comments (1)
tests/test_engine.py (1)
1-1: 🏗️ Heavy liftAlign this test file with the test-path mirroring guideline.
This module tests
policyforge/engine.py, so placing it under a mirrored path (for example,tests/policyforge/test_engine.py) would satisfy the repository rule and keep test discovery structure consistent.As per coding guidelines,
tests/**/*.py: Test directory structure must mirror the package structure with corresponding test files.🤖 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_engine.py` at line 1, The test file currently located as tests/test_engine.py should be moved to mirror the package path: create tests/policyforge/test_engine.py (matching the tested module policyforge/engine.py) and remove the top-level tests/test_engine.py; after moving, ensure any imports in the test refer to policyforge.engine (e.g., from policyforge import engine or from policyforge.engine import <symbols>) and update any relative import paths or test discovery configuration accordingly.
🤖 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.
Outside diff comments:
In `@tests/test_decorators.py`:
- Around line 1-265: The file tests/test_decorators.py is failing CI due to
Black formatting; fix by running Black on that file (or repository) to apply the
canonical formatter, re-stage the modified file, and push a new commit so CI can
pass; targets to check include the decorator/tests referencing policy_gate,
PolicyGateWrapper, and _bind_positional_args to ensure no behavioral edits were
made when formatting.
In `@tests/test_engine.py`:
- Around line 1-784: The file tests/test_engine.py is failing CI due to Black
formatting; run the Black formatter (e.g., black tests/test_engine.py or black
.) and commit the resulting formatting-only changes so the lint job passes.
Locate this file and reformat the tests (including classes like
TestBasicEvaluation, TestResolveField and functions referencing _resolve_field
and _hash_args) then add a commit with only formatting changes.
In `@tests/test_loader.py`:
- Around line 1-448: This test module (contains classes like TestLoadFile,
TestValidationEdgeCases, TestLoadTrustConfig and tests such as
test_loads_single_policy and test_parses_full_block) needs to be reformatted
with Black to satisfy CI; run Black on the test file (or reformat the module) so
the file meets the project's Black configuration before merging.
---
Nitpick comments:
In `@tests/test_engine.py`:
- Line 1: The test file currently located as tests/test_engine.py should be
moved to mirror the package path: create tests/policyforge/test_engine.py
(matching the tested module policyforge/engine.py) and remove the top-level
tests/test_engine.py; after moving, ensure any imports in the test refer to
policyforge.engine (e.g., from policyforge import engine or from
policyforge.engine import <symbols>) and update any relative import paths or
test discovery configuration accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11eda8ca-1fde-4c5d-add3-9502e99110ef
📒 Files selected for processing (3)
tests/test_decorators.pytests/test_engine.pytests/test_loader.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_engine.py (1)
915-922: ⚡ Quick winEscape literal KeyError messages in
match=assertions.
pytest.raises(..., match=...)uses regex. The.ina.bis currently wildcard, so these tests can pass on non-exact messages.Proposed tightening
+import re ... - with pytest.raises(KeyError, match="Field 'a.b' not found at segment 'b'"): + with pytest.raises( + KeyError, match=re.escape("Field 'a.b' not found at segment 'b'") + ): _resolve_field(context, "a.b") ... - with pytest.raises( - KeyError, match="Cannot traverse into non-dict at segment 'b' in path 'a.b'" - ): + with pytest.raises( + KeyError, + match=re.escape("Cannot traverse into non-dict at segment 'b' in path 'a.b'"), + ): _resolve_field(context, "a.b")🤖 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_engine.py` around lines 915 - 922, The tests calling pytest.raises in test_resolve_field and test_resolve_field_non_dict_traversal use regex match strings containing literal dots (e.g. "a.b") which act as wildcards; update the match patterns for the _resolve_field KeyError assertions to escape regex metacharacters (e.g. use "a\\.b" or apply re.escape to the expected message) so the assertions match the exact literal message "a.b" and the segment names like "b".
🤖 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_engine.py`:
- Around line 915-922: The tests calling pytest.raises in test_resolve_field and
test_resolve_field_non_dict_traversal use regex match strings containing literal
dots (e.g. "a.b") which act as wildcards; update the match patterns for the
_resolve_field KeyError assertions to escape regex metacharacters (e.g. use
"a\\.b" or apply re.escape to the expected message) so the assertions match the
exact literal message "a.b" and the segment names like "b".
This PR improves the test coverage of the core policy engine by adding unit tests for the internal
_resolve_fieldutility.🎯 What: The testing gap addressed was the lack of direct unit tests for
_resolve_field, specifically its error handling when traversing nested dictionaries.📊 Coverage:
KeyErroris raised with the correct message when a path segment is missing.KeyErroris raised when trying to traverse into a non-dictionary value (e.g., trying to accessa.bwhenais an integer).✨ Result: Improved reliability and confidence in the field resolution logic used throughout the policy evaluation engine.
PR created automatically by Jules for task 13155194893270359330 started by @tblakex01
Summary by Sourcery
Add direct tests for the internal field resolution helper and tidy YAML fixture construction in existing tests.
Tests:
Summary by CodeRabbit