Adds hash change e2e test by PuneetPunamiya · Pull Request #210 · redhat-data-and-ai/unstructured-data-controller

PuneetPunamiya · 2026-05-14T14:09:53Z

No description provided.

Tests the complete flow from source S3 bucket through document processing, chunking, and vector embeddings generation to destination S3 bucket Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>

Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>

gemini-code-assist

Code Review

This pull request introduces a new end-to-end test, TestS3Destination, to verify the processing of unstructured data when using S3 as a destination. It includes setup for AWS resources via Localstack, file upload simulations, and verification of embedding generation and hash-based reconciliation. Feedback focuses on improving test stability by using t.Fatal for setup failures to prevent cascading errors and adding nil pointer checks when dereferencing AWS SDK response fields to avoid panics. Additionally, it is recommended to track the port-forwarding process for proper cleanup to avoid background process leaks.

gemini-code-assist · 2026-05-14T14:11:25Z

+		if err != nil {
+			t.Error(err)
+		}


Using t.Error here allows the test to continue even if the directory cannot be read. This can lead to misleading test results (e.g., comparing empty slices) or panics in subsequent steps. Use t.Fatal to stop the test immediately on setup failures.

Suggested change

if err != nil {

t.Error(err)

}

files, err := os.ReadDir(testFilesDirectory)

if err != nil {

t.Fatalf("Failed to read test files directory: %v", err)

}

gemini-code-assist · 2026-05-14T14:11:25Z

+	pf := exec.Command("kubectl", "port-forward", "-n", namespace, "services/localstack", "4566:4566")
+	pf.Stdout = os.Stdout
+	pf.Stderr = os.Stderr
+	if err := pf.Start(); err != nil {
+		return fmt.Errorf("failed to start port-forward: %w", err)
+	}


The port-forward process started here is not tracked for cleanup. If the test suite finishes or crashes, this process might continue running in the background. Consider returning the exec.Cmd object or adding it to a global registry so it can be killed in the TestMain cleanup phase, similar to how it is handled in testSetup.

gemini-code-assist · 2026-05-14T14:11:25Z

+			if err != nil {
+				t.Fatal(err)
+			}
+			queueURL = *createQueueOutput.QueueUrl


The QueueUrl field in CreateQueueOutput is a pointer. Dereferencing it directly without a nil check can cause a panic if the AWS call returns an unexpected response. It is safer to verify the pointer is not nil before dereferencing.

Suggested change

queueURL = *createQueueOutput.QueueUrl

if createQueueOutput.QueueUrl == nil {

t.Fatal("QueueUrl is nil")

}

queueURL = *createQueueOutput.QueueUrl

gemini-code-assist · 2026-05-14T14:11:25Z

+			if err := operatorUtils.WaitForResourceReady(v1alpha1.UnstructuredDataProductCondition, "unstructureddataproducts.operator.dataverse.redhat.com", dataProductCRName, testNamespace); err != nil {
+				t.Error(err)
+			}


Failures during the setup phase of a test should use t.Fatal or t.Fatalf to stop execution immediately. Continuing after a setup failure often leads to confusing secondary errors or panics in the assessment phase, making debugging more difficult.

Suggested change

if err := operatorUtils.WaitForResourceReady(v1alpha1.UnstructuredDataProductCondition, "unstructureddataproducts.operator.dataverse.redhat.com", dataProductCRName, testNamespace); err != nil {

t.Error(err)

}

if err := operatorUtils.WaitForResourceReady(v1alpha1.UnstructuredDataProductCondition, "unstructureddataproducts.operator.dataverse.redhat.com", dataProductCRName, testNamespace); err != nil {

t.Fatalf("Unstructured data product CR failed to become healthy: %v", err)

}

gemini-code-assist · 2026-05-14T14:11:25Z

+				// look for embeddings files (should have .json extension and contain embeddings)
+				foundCount := 0
+				for _, obj := range output.Contents {
+					key := *obj.Key


The Key field in s3types.Object is a pointer. Use aws.ToString(obj.Key) for a safe conversion to string without risking a nil pointer dereference panic.

Suggested change

key := *obj.Key

key := aws.ToString(obj.Key)

gemini-code-assist · 2026-05-14T14:11:26Z

+		if headBefore.ChecksumSHA256 != nil {
+			hashBefore = *headBefore.ChecksumSHA256
+		}
+		timestampBefore := *headBefore.LastModified


Dereferencing LastModified without a nil check can cause a panic. In AWS SDK v2, these fields are pointers and should be checked before access to ensure test stability.

Suggested change

timestampBefore := *headBefore.LastModified

if headBefore.LastModified == nil {

t.Fatal("LastModified is nil")

}

timestampBefore := *headBefore.LastModified

gshikhar2021 · 2026-05-18T18:46:21Z

+// ensurePortForward ensures a fresh port-forward connection to localstack
+// Kills any existing port-forward and starts a new one
+func ensurePortForward(namespace string) error {
+	// Kill any existing port-forward
+	exec.Command("pkill", "-f", "port-forward.*localstack").Run()
+	time.Sleep(1 * time.Second)
+
+	// Start new port-forward
+	log.Println("Starting fresh port-forward for localstack...")
+	pf := exec.Command("kubectl", "port-forward", "-n", namespace, "services/localstack", "4566:4566")
+	pf.Stdout = os.Stdout
+	pf.Stderr = os.Stderr
+	if err := pf.Start(); err != nil {
+		return fmt.Errorf("failed to start port-forward: %w", err)
+	}
+
+	// Give it time to establish
+	time.Sleep(3 * time.Second)
+	log.Println("Port-forward established")
+	return nil
+}
+


I dont think so it is required because user will make sure that while doing port forwarding it should not have any process running , this is making code unnecessarily complex

gshikhar2021 · 2026-05-18T18:52:50Z

+		// upload files to source S3 bucket
+		for _, file := range files {
+			if file.IsDir() {
+				t.Errorf("subdirectories are not allowed in the test files directory: %s", testFilesDirectory)
+			}
+
+			fileContent, err := os.ReadFile(filepath.Join(testFilesDirectory, file.Name()))
+			if err != nil {
+				t.Error(err)
+			}
+
+			key := fmt.Sprintf("%s/%s", sourcePrefix, file.Name())
+			_, err = s3Client.PutObject(ctx, &s3.PutObjectInput{
+				Bucket: aws.String(sourceBucketName),
+				Key:    aws.String(key),
+				Body:   bytes.NewReader(fileContent),
+			})
+			if err != nil {
+				t.Error(err)
+			}
+			t.Logf("Uploaded test file: %s", key)
+		}


so after listing all the files in testFilesDirectory , you are uploading all the files to the source bucket -----> files get processed -----------> uploaded to destinationPrefix in destination bucket , right ??.
Just hold your thoughts!!!!!!!!! and read the next comment

gshikhar2021 · 2026-05-18T19:06:18Z

+		t.Log("Waiting for reconciliation to complete...")
+		err = apimachinerywait.PollUntilContextTimeout(ctx, 2*time.Second, 3*time.Minute, false, func(ctx context.Context) (bool, error) {
+			currentCR := &v1alpha1.UnstructuredDataProduct{}
+			if err := kubeClient.Resources().Get(ctx, dataProductCRName, testNamespace, currentCR); err != nil {
+				t.Logf("Failed to get CR: %v", err)
+				return false, nil
+			}
+
+			// Check if the CR has been reconciled (observed generation matches current generation)
+			if currentCR.Status.LastAppliedGeneration != currentCR.Generation {
+				t.Logf("Waiting for reconciliation, generation: %d, observed: %d",
+					currentCR.Generation, currentCR.Status.LastAppliedGeneration)
+				return false, nil
+			}
+
+			// Check if the UnstructuredDataProductReady condition is True
+			for _, condition := range currentCR.Status.Conditions {
+				if condition.Type == v1alpha1.UnstructuredDataProductCondition {
+					if condition.Status == metav1.ConditionTrue {
+						t.Log("Reconciliation completed successfully")
+						return true, nil
+					}
+					t.Logf("Waiting for condition to be ready, current status: %s, reason: %s",
+						condition.Status, condition.Reason)
+					return false, nil
+				}
+			}
+
+			t.Log("UnstructuredDataProductReady condition not found yet")
+			return false, nil
+		})


we already have WaitForResourceReady function inside the utils_function.go file which automatically waits until CR status true for 10 min , can't we avoid re-writing same logic again ?

gshikhar2021 · 2026-05-18T19:07:04Z

+		err = apimachinerywait.PollUntilContextTimeout(ctx, 2*time.Second, 5*time.Minute, false, func(ctx context.Context) (bool, error) {
+			currentCR := &v1alpha1.UnstructuredDataProduct{}
+			if err := kubeClient.Resources().Get(ctx, dataProductCRName, testNamespace, currentCR); err != nil {
+				t.Logf("Failed to get CR: %v", err)
+				return false, nil
+			}
+
+			// Check if the CR has been reconciled
+			if currentCR.Status.LastAppliedGeneration != currentCR.Generation {
+				t.Logf("Waiting for reconciliation, generation: %d, observed: %d",
+					currentCR.Generation, currentCR.Status.LastAppliedGeneration)
+				return false, nil
+			}
+
+			// Check if the UnstructuredDataProductReady condition is True
+			for _, condition := range currentCR.Status.Conditions {
+				if condition.Type == v1alpha1.UnstructuredDataProductCondition {
+					if condition.Status == metav1.ConditionTrue {
+						t.Log("Reconciliation completed successfully")
+						return true, nil
+					}
+					t.Logf("Waiting for condition to be ready, current status: %s, reason: %s",
+						condition.Status, condition.Reason)
+					return false, nil
+				}
+			}
+
+			t.Log("UnstructuredDataProductReady condition not found yet")
+			return false, nil
+		})
+		if err != nil {
+			t.Fatalf("Timeout waiting for reconciliation to complete: %v", err)
+		}


same................logic is getting repeated

gshikhar2021 · 2026-05-18T19:13:45Z

+		})
+		if err != nil {
+			t.Fatalf("Failed to upload new file: %v", err)
+		}


Contd... listing all the files from the destination getting the details of first file, then reading the testFilesDirectory again and finding the files that hasn't been uploaded yet at line 1139, how is it possible because we have uploaded all the files previously.

gshikhar2021 · 2026-05-18T19:23:41Z

@PuneetPunamiya , what I think , instead of writing this much complex logic like iterating through the directory , ensuring that there are atleast 2 files, finding the non uploaded file and then uploading....verifying

can we do something like

// Modify the file content (append some text to change the hash)
		modifiedContent := append(originalContent, []byte("\n\n--- MODIFIED CONTENT FOR TESTING HASH CHANGE ---\n")...)
		t.Log("Modified file content to trigger hash change")

instead of iterating , uploading , simply create file1 , file2 content variable with []byte data

upload file1 get the hash and timestamp, uplaod file2 verify [expected - should not re-upload]
modify file1 content as you did above and verify (expected - should re-upload the file)

what are your thoughts on this ?

piyush-garg · 2026-05-26T04:00:25Z

+	feature.Setup(
+		func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context {
+			// Ensure port-forward is alive before setup
+			if err := ensurePortForward(testNamespace); err != nil {


why we need this ?

piyush-garg · 2026-05-26T04:01:16Z

+			}
+
+			// create AWS clients
+			err = awsclienthandler.NewSourceS3ClientFromConfig(ctx, &awsclienthandler.AWSConfig{


are not all these created in main_test.go or these should be part of config i required for anything apart from file upload

piyush-garg · 2026-06-01T03:37:56Z

closing this as both tests are merged in single MR #202

PuneetPunamiya added 3 commits May 14, 2026 15:45

Add e2e test for S3 destination processing pipeline

ae77fb9

Tests the complete flow from source S3 bucket through document processing, chunking, and vector embeddings generation to destination S3 bucket Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>

adds e2e test for hash

8599676

Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>

test: add e2e test for file modification scenario

1f3f970

Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>

PuneetPunamiya had a problem deploying to e2e-tests May 14, 2026 14:09 — with GitHub Actions Failure

gemini-code-assist Bot reviewed May 14, 2026

View reviewed changes

piyush-garg added this to the v0.7.0 milestone May 15, 2026

piyush-garg added the needs-review label May 15, 2026

gshikhar2021 reviewed May 18, 2026

View reviewed changes

piyush-garg reviewed May 26, 2026

View reviewed changes

piyush-garg added needs-fix and removed needs-review labels May 26, 2026

piyush-garg closed this Jun 1, 2026

Uh oh!

Conversation

PuneetPunamiya commented May 14, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

gshikhar2021 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

gshikhar2021 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

gshikhar2021 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

gshikhar2021 May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gshikhar2021 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

gshikhar2021 commented May 18, 2026

Uh oh!

piyush-garg May 26, 2026

Choose a reason for hiding this comment

Uh oh!

piyush-garg May 26, 2026

Choose a reason for hiding this comment

Uh oh!

piyush-garg commented Jun 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gshikhar2021 May 18, 2026 •

edited

Loading