[RHOAIENG-36758] - Investigate and Optimize KServe Unit Test Executio… #992
Conversation
WalkthroughAdds a new test utilities file providing constants and multiple factory/helper functions to build v1beta1 InferenceService test objects and expected Kubernetes/Knative resources, and introduces a Bash script that runs Go tests, captures verbose output, and identifies slow tests by duration thresholds. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/CI
participant Script as identify_slow_tests.sh
participant GoTest as go test
participant Log as detailed_test_output.log
participant Analyzer as log parser
Dev->>Script: invoke script (package target)
Script->>GoTest: run `go test -v` (captures stdout/stderr)
GoTest-->>Script: verbose test output stream
Script->>Log: write full output to file
Script->>Analyzer: scan output for timing markers (>=30s,40s,50s,70s)
Analyzer-->>Script: list of slow tests and thresholds
Script-->>Dev: print summary and path to `detailed_test_output.log`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
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 |
![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
🧹 Nitpick comments (2)
pkg/controller/v1beta1/inferenceservice/tests_common.go (2)
40-44: Fix typos in constant and helper names for readabilityThere are a couple of small but visible typos that are worth fixing before these helpers spread:
- Constant:
PROGRESSION_DEADLINE_SECODS→PROGRESSION_DEADLINE_SECONDS.- Function:
getExectedService→getExpectedService.Suggested change (plus updating call sites accordingly):
-const ( - REPLICAS int32 = 1 - REVISION_HISTORY int32 = 10 - PROGRESSION_DEADLINE_SECODS int32 = 600 - GRACE_PERIOD int64 = 30 -) +const ( + REPLICAS int32 = 1 + REVISION_HISTORY int32 = 10 + PROGRESSION_DEADLINE_SECONDS int32 = 600 + GRACE_PERIOD int64 = 30 +) -func getExectedService(predictorServiceKey types.NamespacedName, serviceName string) corev1.Service { +func getExpectedService(predictorServiceKey types.NamespacedName, serviceName string) corev1.Service { - ProgressDeadlineSeconds: ptr.To(PROGRESSION_DEADLINE_SECODS), + ProgressDeadlineSeconds: ptr.To(PROGRESSION_DEADLINE_SECONDS),This keeps the helpers self‑explanatory for future readers and avoids typo propagation.
Also applies to: 126-148, 265-331, 404-407
335-402: Document or guard against nilInferenceServicemodel fields
getDeploymentWithKServiceLabeldereferences several nested pointers:
*isvc.Spec.Predictor.Model.StorageURI*isvc.Spec.Predictor.Model.RuntimeVersionIf any of
isvc,isvc.Spec.Predictor.Model,StorageURI, orRuntimeVersionis nil, tests will panic.If the intended contract is “only call this with a fully‑initialized
InferenceService”, it’d help to either:
- Add a brief comment stating that precondition, or
- Add explicit checks with a clear panic message so failures are easier to diagnose, e.g.:
func getDeploymentWithKServiceLabel(predictorDeploymentKey types.NamespacedName, serviceName string, isvc *v1beta1.InferenceService) appsv1.Deployment { + if isvc == nil || isvc.Spec.Predictor.Model == nil || + isvc.Spec.Predictor.Model.StorageURI == nil || + isvc.Spec.Predictor.Model.RuntimeVersion == nil { + panic("getDeploymentWithKServiceLabel: isvc.Spec.Predictor.Model.{StorageURI,RuntimeVersion} must be non-nil") + }This keeps the helper safer to reuse in new tests that might construct partial specs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/controller/v1beta1/inferenceservice/tests_common.go(1 hunks)test/scripts/identify_slow_tests.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/scripts/identify_slow_tests.sh (1)
pkg/controller/v1beta1/inferenceservice/suite_test.go (1)
TestV1beta1APIs(62-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build (3.10)
- GitHub Check: test
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.9)
- GitHub Check: Test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: precommit-check
|
/lgtm |
…n Time cherry-picked from community: clean-up raw-controller tests (kserve#4781) Signed-off-by: Spolti <[email protected]>
spolti
force-pushed
the
RHOAIENG-36758
branch
from
aab8f83 to
6a6e91e
Compare
|
/lgtm |
Jooho
left a comment
Jooho
left a comment
There was a problem hiding this comment.
2 CI fails were expected because there is an issue.
So I will merge this manually.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jooho, spolti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@spolti: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions 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. |
github-project-automation
bot
moved this from New/Backlog
to Done
in ODH Model Serving Planning
4 participants
…n Time
cherry-picked from community: clean-up raw-controller tests (kserve#4781)
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Type of changes
Please delete options that are not relevant.
Feature/Issue validation/testing:
Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.
Test A
Test B
Logs
Special notes for your reviewer:
Checklist:
Release note:
Re-running failed tests
/rerun-all- rerun all failed workflows./rerun-workflow <workflow name>- rerun a specific failed workflow. Only one workflow name can be specified. Multiple /rerun-workflow commands are allowed per comment.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.