Skip to content

Add e2e test for S3 destination processing pipeline#202

Open
PuneetPunamiya wants to merge 2 commits into
redhat-data-and-ai:mainfrom
PuneetPunamiya:adds-s3-e2e-test
Open

Add e2e test for S3 destination processing pipeline#202
PuneetPunamiya wants to merge 2 commits into
redhat-data-and-ai:mainfrom
PuneetPunamiya:adds-s3-e2e-test

Conversation

@PuneetPunamiya

Copy link
Copy Markdown
Contributor
  • Tests the complete flow from source S3 bucket through document processing, chunking, and vector embeddings generation to destination S3 bucket

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new end-to-end test, TestS3Destination, which validates the unstructured data processing pipeline using S3 for both source and destination. The feedback focuses on improving the robustness and cleanliness of the test code. Key suggestions include avoiding hardcoded SQS URLs by capturing them from the creation response, using t.Fatal for setup failures to prevent invalid test states, removing redundant client initializations, and ensuring the teardown logic comprehensively cleans up all created resources, including the data storage bucket and SQS queue.

Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
@gshikhar2021

gshikhar2021 commented May 18, 2026

Copy link
Copy Markdown
Contributor

@PuneetPunamiya , After rebasing you haven't rename the unstructureddataproduct to unstructureddatapipeline at line 942 and 1012 as function name is updated , please take a look of that

  • also the above comment needs to be addressed imo, by directly deleting the bucket instead of cleaning and then deleting
  • just a small improvement as per my suggestion ,can we use the same buckets instead of creating new one , so basically you can move the bucket objects deletion code into the cleanup logic of Test1 and when the Test2 will start it will get fresh buckets and eventually in the cleanup of Test2 you can drop the buckets, wdyt ??

Comment thread test/e2e/unstructured_test.go Outdated
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>
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
Comment thread test/e2e/unstructured_test.go Outdated
}
t.Log("UnstructuredDataPipeline is ready after S3 destination patch")

if err := apimachinerywait.PollUntilContextTimeout(

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.

why this wait is required ?

Comment thread test/e2e/unstructured_test.go Outdated
Verify destination embeddings are not re-uploaded when a new source
file is added, and are re-uploaded when an existing file is overwritten.

Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants