refactor: move validate_metrics_value to utilities/monitoring#5546
refactor: move validate_metrics_value to utilities/monitoring#5546hmeir wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthrough
ChangesRelocate validate_metrics_value helper
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
📌 Additional InformationCustom Commands:
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot-4
requested review from
RoniKishner,
albarker-rh and
dshchedr
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@utilities/monitoring.py`:
- Around line 184-186: Add a Google-format docstring to validate_metrics_value
describing that it polls Prometheus for the metric, logs comparison snapshots
during validation, and may raise TimeoutExpiredError on timeout. Keep the
docstring aligned with the function signature and include Args and Raises
sections so the public side effects are clear.
- Around line 184-209: The `validate_metrics_value` helper is skipping
legitimate zero values because it only compares when `if sample:` is truthy, so
the `get_metrics_value()` empty-result path can never match `0`. Update
`validate_metrics_value` to compare every sample (or at least use an explicit
`is not None` check) and allow `expected_value` to accept `0` by widening its
type if needed, while keeping the existing `TimeoutSampler` and
`get_metrics_value` flow intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26757548-9156-4b90-bcfb-4d7193d391a7
📒 Files selected for processing (10)
tests/observability/metrics/conftest.pytests/observability/metrics/test_aaq_metrics.pytests/observability/metrics/test_general_metrics.pytests/observability/metrics/test_metrics.pytests/observability/metrics/test_migration_metrics.pytests/observability/metrics/test_ssp_metrics.pytests/observability/metrics/test_vms_metrics.pytests/observability/upgrade/test_upgrade_observability.pytests/observability/utils.pyutilities/monitoring.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
RedHatQE/openshift-virtualization-tests-design-docs(manual)
💤 Files with no reviewable changes (1)
- tests/observability/utils.py
Move `validate_metrics_value` from `tests/observability/utils.py` to `utilities/monitoring.py` for cross-directory reuse. Now accepts `str | int` expected_value (int 0 for absent metrics). Add unit tests. Update all observability callers. Signed-off-by: Harel Meir <hmeir@redhat.com>
hmeir
force-pushed
the
move-validate-metrics-value
branch
from
c1c9232 to
a779ee8
Compare
openshift-virtualization-qe-bot-3
added
needs-rebase
size/L
and removed
changes-requested-coderabbitai[bot]
size/M
labels
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5546 +/- ##
==========================================
+ Coverage 98.67% 98.68% +0.01%
==========================================
Files 25 42 +17
Lines 2487 2519 +32
==========================================
+ Hits 2454 2486 +32
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5546 published |
|
/lgtm |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5546 published |
|
/verified All tests passed for PR #5546. Execution details |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== callers of validate_metrics_value ==="
rg -n 'validate_metrics_value' --type py
echo "=== markers in affected files ==="
for f in tests/observability/metrics/conftest.py tests/observability/metrics/test_aaq_metrics.py tests/observability/metrics/test_general_metrics.py tests/observability/metrics/test_metrics.py tests/observability/metrics/test_migration_metrics.py tests/observability/metrics/test_ssp_metrics.py tests/observability/metrics/test_vms_metrics.py tests/observability/upgrade/test_upgrade_observability.py; do
echo "--- $f ---"
rg -n '`@pytest.mark`.(smoke|gating)' "$f" || echo "no smoke/gating marker"
done
echo "=== conftest hierarchy autouse fixtures ==="
rg -n 'autouse=True' tests/observability/conftest.py tests/observability/metrics/conftest.py tests/conftest.py 2>/dev/null
echo "=== smoke/gating files anywhere referencing these test files' fixtures ==="
rg -l '`@pytest.mark.smoke`' tests/observability/ 2>/dev/null
rg -l '`@pytest.mark.gating`' tests/observability/ 2>/dev/nullLength of output: 1430 🏁 Script executed: #!/bin/bash
set +e
echo "=== callers of validate_metrics_value ==="
rg -n 'validate_metrics_value'
echo "=== markers in affected files ==="
for f in tests/observability/metrics/conftest.py tests/observability/metrics/test_aaq_metrics.py tests/observability/metrics/test_general_metrics.py tests/observability/metrics/test_metrics.py tests/observability/metrics/test_migration_metrics.py tests/observability/metrics/test_ssp_metrics.py tests/observability/metrics/test_vms_metrics.py tests/observability/upgrade/test_upgrade_observability.py; do
echo "--- $f ---"
rg -n '`@pytest.mark`.(smoke|gating)' "$f"
echo "(end $f)"
done
echo "=== conftest hierarchy autouse fixtures ==="
rg -n 'autouse=True' tests/observability/conftest.py tests/observability/metrics/conftest.py tests/conftest.py
echo "=== smoke/gating files anywhere in tests/observability ==="
rg -l '`@pytest.mark.smoke`' tests/observability/
echo "---gating---"
rg -l '`@pytest.mark.gating`' tests/observability/
echo "=== other utilities/monitoring.py usages of validate_metrics_value/get_metrics_value ==="
rg -n 'get_metrics_value|validate_metrics_value' utilities/monitoring.pyLength of output: 1556 🏁 Script executed: #!/bin/bash
set +e
echo "pwd: $(pwd)"
git status
git log --oneline -5
echo "=== fd search ==="
fd 'monitoring.py' utilities
fd 'test_metrics.py' tests
echo "=== cat utilities/monitoring.py grep ==="
grep -n 'validate_metrics_value\|get_metrics_value' utilities/monitoring.py
echo "=== grep in tests/observability ==="
grep -rn 'validate_metrics_value' tests/observability/Length of output: 3934 🏁 Script executed: #!/bin/bash
set +e
echo "=== markers in affected test files ==="
for f in tests/observability/metrics/conftest.py tests/observability/metrics/test_aaq_metrics.py tests/observability/metrics/test_general_metrics.py tests/observability/metrics/test_metrics.py tests/observability/metrics/test_migration_metrics.py tests/observability/metrics/test_ssp_metrics.py tests/observability/metrics/test_vms_metrics.py tests/observability/upgrade/test_upgrade_observability.py; do
echo "--- $f ---"
grep -n '`@pytest.mark.smoke`\|`@pytest.mark.gating`\|`@pytest.mark.tier`' "$f"
done
echo "=== conftest.py fixture context around validate_metrics_value calls ==="
sed -n '180,230p' tests/observability/metrics/conftest.py
echo "=== autouse fixtures in observability conftest hierarchy ==="
grep -n 'autouse=True' tests/observability/conftest.py tests/observability/metrics/conftest.py tests/observability/upgrade/conftest.py tests/conftest.py 2>/dev/null
echo "=== any smoke/gating markers anywhere in tests/observability ==="
grep -rl '`@pytest.mark.smoke`' tests/observability/
echo "---gating---"
grep -rl '`@pytest.mark.gating`' tests/observability/Length of output: 3931 🏁 Script executed: #!/bin/bash
set +e
sed -n '1965,1990p' tests/conftest.py
echo "=== check if monitoring.py imported in tests/conftest.py ==="
grep -n 'monitoring' tests/conftest.pyLength of output: 1345
Test Execution Plan
No separate "Real tests (cluster required)" section is needed beyond the list above — this PR only relocates/generalizes existing test logic (plus adds |
8 participants
What this PR does / why we need it:
Move
validate_metrics_valuefromtests/observability/utils.pytoutilities/monitoring.pyfor cross-directory reuse.Now accepts
str | intexpected_value (int 0 for absent metrics).Add unit tests. Update all observability callers.
Which issue(s) this PR fixes:
Allows another packages to verify metrics tests.
Special notes for reviewer:
jira-ticket:
NONE
Summary by CodeRabbit