Skip to content

This PR is to add comments to TestGenerateDataLoadValueFile in fluid/pkg/ddc/alluxio/load_data_test.go #6046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AritxOnly wants to merge 1 commit into
base: master
Choose a base branch
from
Open

This PR is to add comments to TestGenerateDataLoadValueFile in fluid/pkg/ddc/alluxio/load_data_test.go #6046

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/ddc/alluxio/load_data_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ import (
"k8s.io/apimachinery/pkg/runtime"
)

// TestGenerateDataLoadValueFile verifies that generateDataLoadValueFile creates
// a temporary values file for a DataLoad and returns the generated file path
// for both empty-target and explicit-target configurations.
func TestGenerateDataLoadValueFile(t *testing.T) {
// Prepare a dataset in the fake client so the dataload can resolve its target dataset.
datasetInputs := []datav1alpha1.Dataset{
{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -54,6 +58,8 @@ func TestGenerateDataLoadValueFile(t *testing.T) {
context := cruntime.ReconcileRequestContext{
Client: client,
}

// Cover both branches where the dataload has no explicit targets and where it does.
dataLoadNoTarget := datav1alpha1.DataLoad{
ObjectMeta: metav1.ObjectMeta{
Name: "test-dataload",
Expand Down Expand Up @@ -98,6 +104,8 @@ func TestGenerateDataLoadValueFile(t *testing.T) {
expectFileName: filepath.Join(os.TempDir(), "fluid-test-dataload-loader-values.yaml"),
},
}

// Generate the values file and assert that the returned temp file path matches the expected pattern.

@gemini-code-assist gemini-code-assist Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There are two issues in the test loop below this comment:

  1. Resource Leak: The temporary files created by generateDataLoadValueFile are never deleted, which leaves leftover files in the temporary directory after running tests.
  2. Misleading Error Message: If generateDataLoadValueFile succeeds but the returned filename does not match the expected pattern, err is nil, resulting in a confusing error message: fail to generate the dataload value file: <nil>.

Consider refactoring the loop to handle errors and clean up the temporary files properly:

for _, test := range testCases {
	engine := AlluxioEngine{}
	fileName, err := engine.generateDataLoadValueFile(context, &test.dataLoad)
	if err != nil {
		t.Errorf("failed to generate the dataload value file: %v", err)
		continue
	}
	if !strings.Contains(fileName, test.expectFileName) {
		t.Errorf("expected file name to contain %q, got %q", test.expectFileName, fileName)
	}
	if fileName != "" {
		_ = os.Remove(fileName)
	}
}

for _, test := range testCases {
engine := AlluxioEngine{}
if fileName, err := engine.generateDataLoadValueFile(context, &test.dataLoad); !strings.Contains(fileName, test.expectFileName) {
Expand Down
Loading