Address even more pytest warnings, co-authored with Opus 4.6#16330
Open
AlanCoding wants to merge 3 commits intoansible:develfrom
Open
Address even more pytest warnings, co-authored with Opus 4.6#16330AlanCoding wants to merge 3 commits intoansible:develfrom
AlanCoding wants to merge 3 commits intoansible:develfrom
Conversation
📝 WalkthroughWalkthroughUpdates tests to use timezone-aware UTC datetimes, refactors internal pyparsing usage in a utility to pyparsing v3-style APIs, removes several pytest deprecation filterwarnings, and updates pyparsing and a dependency version in requirements files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/utils/filters.py`:
- Around line 6-8: The module imports and uses pyparsing v3 API names
(infix_notation, OpAssoc, Opt, set_parse_action, parse_string) while the repo
pins pyparsing==2.4.7, causing ImportError; update awx/main/utils/filters.py to
import pyparsing with a compatibility shim: attempt to import the v3 symbols
(infix_notation, OpAssoc, Opt, set_parse_action, parse_string) and on
ImportError fall back to v2 equivalents (infixNotation, opAssoc, Optional,
setParseAction, parseString), then use those local names throughout (e.g., where
infix_notation/OpAssoc/Opt/set_parse_action/parse_string are referenced) so the
rest of the file (parsing logic around the filter parser) works with both
pyparsing v2 and v3.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e9a14c9-b6ae-4481-b332-ddd9d53cb18d
📒 Files selected for processing (3)
awx/main/tests/unit/notifications/test_grafana.pyawx/main/utils/filters.pypytest.ini
💤 Files with no reviewable changes (1)
- pytest.ini
github-actions
bot
added
the
dependencies
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@requirements/requirements.in`:
- Line 53: The requirement "pyparsing>3.0" should be constrained to the v3.x
major range to avoid pulling an unreviewed v4; update the requirement spec from
"pyparsing>3.0" to a bounded spec such as "pyparsing>=3.0,<4.0" (or
"pyparsing~=3.0") in requirements.in so host query parsing code that depends on
pyparsing (awx.main.utils.filters) remains on the supported major version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81b35f59-e211-40a6-b2e9-0e929bff2be6
📒 Files selected for processing (2)
requirements/requirements.inrequirements/requirements.txt
AlanCoding
force-pushed
the
your_third_warning
branch
from
4eb0d07 to
7f324d6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
awx/main/utils/filters.py (2)
317-335: Add a couple of tokenizer-focused regression cases.The coverage in
awx/main/tests/unit/utils/test_filters.py:74-94,150-175, and201-220covers the common quoted/unquoted paths and precedence, but not escaped quotes or parenthesized mixed expressions through this new tokenizer. A few targeted cases there would make the pyparsing v3 swap safer.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/utils/filters.py` around lines 317 - 335, Add targeted regression tests exercising the new tokenizer paths: create test cases in awx/main/tests/unit/utils/test_filters.py that feed filter strings containing escaped quotes inside quoted tokens (e.g. a value with \" inside quotes) and mixed parenthesized expressions combining quoted and unquoted tokens (e.g. "(field1=value1 or field2=\"with \\\"quote\\\"\") and field3=value3") to ensure the parser built from unquoted, quoted, token, operand, BoolOperand and bool_expr (with BoolAnd/BoolOr) handles escapes and parentheses correctly; assert the parsed structure or resulting boolean AST matches expected BoolOperand/BoolAnd/BoolOr nesting and include both positive and edge cases so the pp.QuotedString escape behavior and pp.infix_notation precedence are covered.
268-272: Preserve the real pyparsing error and the original cause.
pp.ParseException('...')treats the string aspstr, notmsg, so Line 271 does not build the error you intend. Line 336 then wraps it without chaining, which drops the underlying lookup failure during validation.♻️ Proposed fix
- except LookupError: - raise pp.ParseException('No related field named %s' % relation) + except LookupError as exc: + raise pp.ParseException("", 0, f"No related field named {relation}") from exc @@ - except (pp.ParseException, FieldDoesNotExist): - raise RuntimeError(u"Invalid query %s" % filter_string_raw) + except (pp.ParseException, FieldDoesNotExist) as exc: + raise RuntimeError(f"Invalid query {filter_string_raw}") from excAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 333-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/utils/filters.py` around lines 268 - 272, The current code raises pp.ParseException('No related field named ...') which passes the text as the pstr parameter instead of the msg, and it also drops the original LookupError cause; update both places where this pattern occurs (the get_model lookup and the later wrap around lines referencing the same ParseException) to construct the ParseException with an explicit msg (e.g. pp.ParseException('', 0, None, msg='No related field named %s' % relation)) and re-raise it using exception chaining (raise pp.ParseException(... ) from e) so the original LookupError is preserved as the __cause__.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@awx/main/utils/filters.py`:
- Around line 317-335: Add targeted regression tests exercising the new
tokenizer paths: create test cases in awx/main/tests/unit/utils/test_filters.py
that feed filter strings containing escaped quotes inside quoted tokens (e.g. a
value with \" inside quotes) and mixed parenthesized expressions combining
quoted and unquoted tokens (e.g. "(field1=value1 or field2=\"with
\\\"quote\\\"\") and field3=value3") to ensure the parser built from unquoted,
quoted, token, operand, BoolOperand and bool_expr (with BoolAnd/BoolOr) handles
escapes and parentheses correctly; assert the parsed structure or resulting
boolean AST matches expected BoolOperand/BoolAnd/BoolOr nesting and include both
positive and edge cases so the pp.QuotedString escape behavior and
pp.infix_notation precedence are covered.
- Around line 268-272: The current code raises pp.ParseException('No related
field named ...') which passes the text as the pstr parameter instead of the
msg, and it also drops the original LookupError cause; update both places where
this pattern occurs (the get_model lookup and the later wrap around lines
referencing the same ParseException) to construct the ParseException with an
explicit msg (e.g. pp.ParseException('', 0, None, msg='No related field named
%s' % relation)) and re-raise it using exception chaining (raise
pp.ParseException(... ) from e) so the original LookupError is preserved as the
__cause__.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 624401cc-d49a-464d-8707-2fb9fe2f1036
📒 Files selected for processing (5)
awx/main/tests/unit/notifications/test_grafana.pyawx/main/utils/filters.pypytest.inirequirements/requirements.inrequirements/requirements.txt
💤 Files with no reviewable changes (1)
- pytest.ini
🚧 Files skipped from review as they are similar to previous changes (2)
- awx/main/tests/unit/notifications/test_grafana.py
- requirements/requirements.in
|
AlanCoding
force-pushed
the
your_third_warning
branch
from
7f324d6 to
50834e6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
awx/main/utils/filters.py (1)
268-271: Consider exception chaining for better traceability.Static analysis flags that re-raising within an
exceptblock withoutfromcan obscure the original exception context.♻️ Suggested fix
try: model = get_model(relation) - except LookupError: - raise pp.ParseException('No related field named %s' % relation) + except LookupError as err: + raise pp.ParseException('No related field named %s' % relation) from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/utils/filters.py` around lines 268 - 271, The except block catching LookupError when calling get_model(relation) should chain the original exception to preserve context: capture the caught exception (e.g., as e) and re-raise pp.ParseException('No related field named %s' % relation) using "from e" so the original LookupError is attached; update the block around get_model(relation) accordingly to use exception chaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@awx/main/utils/filters.py`:
- Around line 268-271: The except block catching LookupError when calling
get_model(relation) should chain the original exception to preserve context:
capture the caught exception (e.g., as e) and re-raise pp.ParseException('No
related field named %s' % relation) using "from e" so the original LookupError
is attached; update the block around get_model(relation) accordingly to use
exception chaining.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1befe84-068f-4b55-bda6-5d50abe48300
📒 Files selected for processing (5)
awx/main/tests/unit/notifications/test_grafana.pyawx/main/utils/filters.pypytest.inirequirements/requirements.inrequirements/requirements.txt
💤 Files with no reviewable changes (1)
- pytest.ini
🚧 Files skipped from review as they are similar to previous changes (1)
- awx/main/tests/unit/notifications/test_grafana.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Fed the same prompt again, as from #16298
One before that was #16269
ISSUE TYPE
COMPONENT NAME
Note
Medium Risk
Upgrades
pyparsingto v3 and rewritesSmartFilter.query_from_stringparsing logic, which can affect host filtering behavior and error handling. Other changes are test/warning hygiene and dependency pin updates with low functional risk.Overview
Updates the smart host filter parser to use
pyparsingv3 APIs, including a new token/operand grammar, updated exception handling, and simplified key/value extraction behavior inSmartFilter.query_from_string.Cleans up test and CI noise by switching Grafana notification tests to timezone-aware UTC datetimes, removing several
pytestdeprecation-warning suppressions, and updating dependency constraints/pins (notablypyparsingand adispatcherdversion formatting fix).Written by Cursor Bugbot for commit 50834e6. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Tests
Refactor
Chores