add test for cache runtime dataload#6116
Conversation
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new test suite dataload_test.go to verify the genDataLoadValue logic of the CacheEngine, covering scenarios such as correct volume/mount wiring, missing DataLoad specs, and empty commands/args. Additionally, it cleans up an unused worker_pod variable in the GHA E2E test script test.sh. The review feedback suggests initializing the test context ctx with a fake recorder in the main BeforeEach block to prevent potential nil pointer panics and simplify individual test cases.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| baseClient = fake.NewFakeClientWithScheme(scheme, runtimeObj, runtimeClass, dataset) | ||
| engine = &CacheEngine{Client: baseClient, name: "demo", namespace: "default"} | ||
| ctx = cruntime.ReconcileRequestContext{Context: context.Background()} |
There was a problem hiding this comment.
Initializing ctx with a fake recorder in the main BeforeEach block ensures that any unexpected warning or event logged during other test cases won't trigger a nil pointer panic (since ctx.Recorder would otherwise be nil). It also simplifies individual test cases by removing the need to manually construct a testCtx with a fake recorder.
| ctx = cruntime.ReconcileRequestContext{Context: context.Background()} | |
| ctx = cruntime.ReconcileRequestContext{ | |
| Context: context.Background(), | |
| Recorder: &record.FakeRecorder{}, | |
| } |
| recorder := &record.FakeRecorder{} | ||
| testCtx := cruntime.ReconcileRequestContext{Context: context.Background(), Recorder: recorder} | ||
| _, err := engine.genDataLoadValue(testCtx, dataset, runtimeObj, runtimeClass, dataload) |
There was a problem hiding this comment.
Since ctx is now initialized with a fake recorder in the main BeforeEach block, we can reuse ctx directly here instead of creating a local testCtx and recorder.
| recorder := &record.FakeRecorder{} | |
| testCtx := cruntime.ReconcileRequestContext{Context: context.Background(), Recorder: recorder} | |
| _, err := engine.genDataLoadValue(testCtx, dataset, runtimeObj, runtimeClass, dataload) | |
| _, err := engine.genDataLoadValue(ctx, dataset, runtimeObj, runtimeClass, dataload) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6116 +/- ##
=========================================
Coverage ? 65.08%
=========================================
Files ? 485
Lines ? 33989
Branches ? 0
=========================================
Hits ? 22122
Misses ? 10126
Partials ? 1741 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request adds a new unit test suite for genDataLoadValue in pkg/ddc/cache/engine/dataload_test.go and removes an unused local variable worker_pod in test/gha-e2e/curvine/test.sh. The review feedback points out critical issues in the test setup: using an uninitialized &record.FakeRecorder{} directly can cause tests to hang indefinitely, and initializing the engine and context without a recorder risks nil pointer dereferences during event recording.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR adds unit test coverage around CacheEngine.genDataLoadValue to validate DataLoad job wiring (config volumes/mounts/env), and removes unused local variables in a Curvine GHA e2e helper script.
Changes:
- Add a new Ginkgo test suite for
genDataLoadValue, asserting configConfigMapvolume/mount/env correctness and basic error cases. - Remove an unused
worker_podvariable and its assignment from the Curvine e2e test script.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/gha-e2e/curvine/test.sh |
Removes unused worker_pod variable/lookup in wait_cache_worker_ready. |
pkg/ddc/cache/engine/dataload_test.go |
Adds Ginkgo tests validating DataLoad value generation (volumes, mounts, env, and error paths). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
Ⅰ. Describe what this PR does
add test for cache runtime dataload
Ⅱ. Does this pull request fix one issue?
fixes #XXXX
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews