Integration Tests for s3-sink related to Github-issue#1048 #2401

deepaksahu562 · 2023-03-24T16:40:03Z

Description

Integration tests for #1048

Issues Resolved

Resolves: Integration tests for #1048

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

kkondaka · 2023-03-27T18:12:35Z

...-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/S3ObjectWorkerIT.java

+        Collection<Record<Event>> records = setEventQueue();
+        s3SinkService.processRecods(records);
+        s3SinkService.accumulateBufferEvents(s3SinkWorker);
+        assertNotNull(recordsGenerator);


Why is this being done here? You should check the impact of the previous statement here.

Modified asserting "not null" for the records instead of recordsGenerator.

March'2023. Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

kkondaka · 2023-04-05T00:21:52Z

...-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/S3ObjectWorkerIT.java

+        Collection<Record<Event>> records = setEventQueue();
+        s3SinkService.processRecords(records);
+        s3SinkService.accumulateBufferEvents(s3SinkWorker);
+        verify(s3SinkService, atLeastOnce()).processRecords(records);


Please validate if accumulateBufferEvents() worked correctly by checking its functionality. This verification is not that useful

dlvenable

I made a few comments. At a high-level, the integration test should work this way.

Create the object under test. This is S3Sink itself. Use real objects only. (You can mock the input config classes, that is acceptable).
Generate a bunch of Events, and wrap them in Records.
Call objectUnderTest.output(events)
Perform an S3 list request to get the actual object created. Hint: In step 1, You can configure the key_prefix to use a unique value in each test. Then you can list using that prefix and the work is quite easy.
Verify you found the S3 object
Read the S3 object
Verify the actual data inside the S3 object.

That is the basic overview of any test. You may have some different variations as well. One example might be calling output without enough records to write. Or calling output twice when two calls are needed to write.

dlvenable · 2023-04-05T02:17:21Z

...-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/S3ObjectWorkerIT.java

+
+        RecordsGenerator recordsGenerator = new JsonRecordsGenerator();
+        assertNotNull(recordsGenerator);
+        S3SinkWorker s3SinkWorker = new S3SinkWorker(s3Client, s3SinkConfig, recordsGenerator.getCodec(),


We should move this up to testing the S3Sink class itself.

In the S3 source, you will see that we have two sets of tests. This is in part because there are two very significant components involved in the source - S3 and SQS. This is not the case here. So testing only the sink is ideal.

dlvenable · 2023-04-05T02:18:10Z

...-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/S3ObjectWorkerIT.java

+    @Test
+    void copy_s3_object_correctly_into_s3_bucket() {
+
+        RecordsGenerator recordsGenerator = new JsonRecordsGenerator();


These two lines of code are not providing any value. You can remove them.

Removed two unused files related to RecordsGenerator and JsonRecordsGenerator.

dlvenable · 2023-04-05T02:19:36Z

...k/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/JsonRecordsGenerator.java

+import com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+
+class JsonRecordsGenerator implements RecordsGenerator {


You don't need this class. The S3 sink is responsible for generating the JSON - not the test.

You may want to have a JsonRecordsReader which can read an actual object from S3.

Removed two unused files related to RecordsGenerator and JsonRecordsGenerator.

dlvenable · 2023-04-05T02:20:01Z

...-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/S3ObjectWorkerIT.java

+        s3Client = S3Client.builder().region(Region.of(System.getProperty("tests.s3source.region"))).build();
+        String bucket = System.getProperty("tests.s3source.bucket");
+
+        s3SinkConfig = mock(S3SinkConfig.class);


This is an integration test and should not be using mocks. Please create real objects.

Created real objects except for s3SinkConfig class.

codecov-commenter · 2023-04-05T13:09:32Z

Codecov Report

Merging #2401 (c9ca322) into main (8e0fd01) will decrease coverage by 0.14%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #2401      +/-   ##
============================================
- Coverage     93.39%   93.26%   -0.14%     
+ Complexity     2093     2090       -3     
============================================
  Files           247      247              
  Lines          5861     5861              
  Branches        478      478              
============================================
- Hits           5474     5466       -8     
- Misses          262      271       +9     
+ Partials        125      124       -1

see 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

kkondaka · 2023-04-12T04:25:48Z

...s3-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/S3SinkWorkerIT.java

+        int s3ObjectCountAfterIngest = gets3ObjectCount();
+
+        Assert.assertNotSame(s3ObjectCountBeforeIngest, s3ObjectCountAfterIngest);
+        assertThat(s3ObjectCountAfterIngest, greaterThan(s3ObjectCountBeforeIngest));


Why greaterThan? Is it not possible to know the exact count?

Integration Tests for s3-sink related to Github-issue#1048

83d2319

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

deepaksahu562 requested a review from a team as a code owner March 24, 2023 16:40

kkondaka requested changes Mar 27, 2023

View reviewed changes

deepaksahu562 and others added 4 commits March 28, 2023 17:50

Merge branch 'opensearch-project:main' into s3_sink_integration_tests

ff229dc

Addressed the code review comments provided by AWS Team on 27th

0ea4860

March'2023. Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

Merge branch 'opensearch-project:main' into s3_sink_integration_tests

9ce8721

Incorporated review comments. git-issue#1048

22cb4f1

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

kkondaka requested changes Apr 5, 2023

View reviewed changes

dlvenable requested changes Apr 5, 2023

View reviewed changes

deepaksahu562 and others added 2 commits April 11, 2023 19:51

Merge branch 'opensearch-project:main' into s3_sink_integration_tests

6bdcccb

Addressed review comments. Github-issue#1048

c9ca322

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

dlvenable self-assigned this Apr 12, 2023

kkondaka reviewed Apr 12, 2023

View reviewed changes

deepaksahu562 closed this May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration Tests for s3-sink related to Github-issue#1048 #2401

Integration Tests for s3-sink related to Github-issue#1048 #2401

deepaksahu562 commented Mar 24, 2023

kkondaka Mar 27, 2023

deepaksahu562 Mar 28, 2023

kkondaka Apr 5, 2023

dlvenable left a comment

dlvenable Apr 5, 2023

deepaksahu562 Apr 11, 2023

dlvenable Apr 5, 2023

deepaksahu562 Apr 11, 2023

dlvenable Apr 5, 2023

deepaksahu562 Apr 11, 2023

dlvenable Apr 5, 2023

deepaksahu562 Apr 11, 2023

codecov-commenter commented Apr 5, 2023 •

edited

kkondaka Apr 12, 2023

Integration Tests for s3-sink related to Github-issue#1048 #2401

Integration Tests for s3-sink related to Github-issue#1048 #2401

Conversation

deepaksahu562 commented Mar 24, 2023

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 5, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

codecov-commenter commented Apr 5, 2023 •

edited