[WIP] Adding unit tests for the extend recommended alerts#2213
Open
JianLi-RH wants to merge 1 commit intoopenshift:mainfrom
Open
[WIP] Adding unit tests for the extend recommended alerts#2213JianLi-RH wants to merge 1 commit intoopenshift:mainfrom
JianLi-RH wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughAdded multiple example data files (JSON, YAML, and text outputs) for upgrade recommendation scenarios—both standard and critical alert cases—and a small test guard to skip a "specific version" variant when no ClusterVersion mapping exists. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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 unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JianLi-RH The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🧹 Nitpick comments (1)
pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.show-outdated-releases-output (1)
21-21: Verify whitespace consistency in the runbook annotation.This line has extra leading whitespace (3 spaces) before
<alert does not have a runbook_url annotation>, while the same annotation on lines 8 and 13 has no such leading whitespace.If the actual output includes this extra spacing, the test fixture is correct. However, if this is unintentional, consider normalizing the whitespace for consistency across alerts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.show-outdated-releases-output` at line 21, The test fixture contains an inconsistent leading whitespace for the annotation "<alert does not have a runbook_url annotation>" (it has three leading spaces on the shown line while identical annotations earlier do not); locate that annotation string in the example content and remove the extra leading spaces so all occurrences are normalized to the same no-leading-space format to ensure consistency across alerts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/admin/upgrade/recommend/examples_test.go`:
- Around line 81-85: The test loop incorrectly assumes variant.versions is
non-nil and indexes it unguarded, causing nil-map variants to always hit the
else branch and skip tests; fix by first checking whether variant.versions !=
nil before reading variant.versions[cv], and only apply the continue/setting of
variant.outputSuffix when the map exists: if variant.versions != nil { if
version = variant.versions[cv]; version != "" { variant.outputSuffix =
fmt.Sprintf(variant.outputSuffixPattern, version) } else { continue } } (leave
behavior unchanged for variants with a nil versions map).
---
Nitpick comments:
In
`@pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.show-outdated-releases-output`:
- Line 21: The test fixture contains an inconsistent leading whitespace for the
annotation "<alert does not have a runbook_url annotation>" (it has three
leading spaces on the shown line while identical annotations earlier do not);
locate that annotation string in the example content and remove the extra
leading spaces so all occurrences are normalized to the same no-leading-space
format to ensure consistency across alerts.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert-alerts.jsonpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert-cv.yamlpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.outputpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.show-outdated-releases-outputpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert-alerts.jsonpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert-cv.yamlpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert.outputpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert.show-outdated-releases-outputpkg/cli/admin/upgrade/recommend/examples_test.go
Author
|
/uncc @ardaguclu @atiratree |
JianLi-RH
force-pushed
the
test_for_extend-update-recommend-precheck-alerts
branch
from
0c36357 to
261f317
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert-alerts.json (1)
29-33: Consider sanitizing environment-specific identifiers in fixtures.Line 29 and Line 51 include concrete cluster/node identifiers. Replacing them with clearly synthetic placeholders would make snapshots less brittle and safer to share broadly.
Also applies to: 51-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert-alerts.json` around lines 29 - 33, The fixture contains environment-specific identifiers (e.g., values for the JSON keys "instance" and "pod" in pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert-alerts.json) that should be sanitized; replace concrete cluster/node IDs like "10.130.0.31:8443" and "insights-operator-b8b5f97fc-kl8rr" (and the similar values around lines 51-58) with clear synthetic placeholders such as "INSTANCE_PLACEHOLDER" and "POD_PLACEHOLDER" (or similarly named tokens) so snapshots are stable and shareable, and ensure any other environment-specific fields (namespace, job, info_link query params containing IDs) are likewise replaced or parameterized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.show-outdated-releases-output`:
- Around line 18-21: The wrapped "Message:" block has inconsistent indentation
(an extra leading space on the third wrapped line); edit the golden output so
that the third line of the wrapped message matches the indentation of the
previous wrapped lines (remove the extra leading space before "it might have
issues..." in the Message block) to normalize formatting and ensure all wrapped
lines under "Message:" align consistently.
---
Nitpick comments:
In
`@pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert-alerts.json`:
- Around line 29-33: The fixture contains environment-specific identifiers
(e.g., values for the JSON keys "instance" and "pod" in
pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert-alerts.json)
that should be sanitized; replace concrete cluster/node IDs like
"10.130.0.31:8443" and "insights-operator-b8b5f97fc-kl8rr" (and the similar
values around lines 51-58) with clear synthetic placeholders such as
"INSTANCE_PLACEHOLDER" and "POD_PLACEHOLDER" (or similarly named tokens) so
snapshots are stable and shareable, and ensure any other environment-specific
fields (namespace, job, info_link query params containing IDs) are likewise
replaced or parameterized.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert-alerts.jsonpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert-cv.yamlpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.outputpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.show-outdated-releases-outputpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert-alerts.jsonpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert-cv.yamlpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert.outputpkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert.show-outdated-releases-outputpkg/cli/admin/upgrade/recommend/examples_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.output
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cli/admin/upgrade/recommend/examples_test.go
- pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert-cv.yaml
- pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-critical-alert.output
Comment on lines
+18
to
+21
| Message: warning alert TestAlert firing, suggesting issues worth investigating before updating the cluster. machine ci-ln-plnvytb-76ef8-42h8v-worker-us-east-1a-84pg2 is in phase: . The alert description is: The machine has been without a Running or Deleting phase for more than 60 minutes. | ||
| The machine may not have been provisioned properly from the infrastructure provider, or | ||
| it might have issues with CertificateSigningRequests being approved. | ||
| <alert does not have a runbook_url annotation> |
There was a problem hiding this comment.
Normalize wrapped-message indentation in the golden output.
Line 21 has an extra leading space versus Line 19 and Line 20, making this block inconsistently formatted.
Suggested fix
- <alert does not have a runbook_url annotation>
+ <alert does not have a runbook_url annotation>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Message: warning alert TestAlert firing, suggesting issues worth investigating before updating the cluster. machine ci-ln-plnvytb-76ef8-42h8v-worker-us-east-1a-84pg2 is in phase: . The alert description is: The machine has been without a Running or Deleting phase for more than 60 minutes. | |
| The machine may not have been provisioned properly from the infrastructure provider, or | |
| it might have issues with CertificateSigningRequests being approved. | |
| <alert does not have a runbook_url annotation> | |
| Message: warning alert TestAlert firing, suggesting issues worth investigating before updating the cluster. machine ci-ln-plnvytb-76ef8-42h8v-worker-us-east-1a-84pg2 is in phase: . The alert description is: The machine has been without a Running or Deleting phase for more than 60 minutes. | |
| The machine may not have been provisioned properly from the infrastructure provider, or | |
| it might have issues with CertificateSigningRequests being approved. | |
| <alert does not have a runbook_url annotation> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@pkg/cli/admin/upgrade/recommend/examples/4.22.0-extend-recommended-alert.show-outdated-releases-output`
around lines 18 - 21, The wrapped "Message:" block has inconsistent indentation
(an extra leading space on the third wrapped line); edit the golden output so
that the third line of the wrapped message matches the indentation of the
previous wrapped lines (remove the extra leading space before "it might have
issues..." in the Message block) to normalize formatting and ensure all wrapped
lines under "Message:" align consistently.
Author
|
@wking Could you please help take a look at below tests? Tested it locally: Make test: |
https://issues.redhat.com/browse/OTA-1811 introduces some new alerts, this PR meand to automate the tests to those alerts. In this PR I added two test file: 1. 4.22.0-extend-recommended-alert-cv.yaml This file covers: - VirtHandlerDaemonSetRolloutFailing + warning + precheck + firing - VMCannotBeEvicted + warning + not precheck + firing - Custom alert + warning + precheck + firing 2. 4.22.0-extend-recommended-critical-alert-cv.yaml This file covers: - VirtHandlerDaemonSetRolloutFailing + critical + precheck + firing - VMCannotBeEvicted + critical + precheck + pending
JianLi-RH
force-pushed
the
test_for_extend-update-recommend-precheck-alerts
branch
from
261f317 to
626e1c5
Compare
Author
|
/cc @wking |
Contributor
|
@JianLi-RH: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
This is the follow up to #2210
This is the JIRA card: https://issues.redhat.com/browse/OTA-1817
In this PR I added two test file:
This file covers:
This file covers:
These tests do not need
specific version, so I updatedexamples_test.gothen these cases will be skipped when the test files do not exist in variant versions.Summary by CodeRabbit
Tests
Chores