S3 settings functional tests #2837

osulzhenko · 2023-12-13T17:53:24Z

No description provided.

SerhiiNahornyi · 2023-12-14T10:19:51Z

@muuki88 We added a few tests for your PR
#2837

Could you pls review them with us?

muuki88 · 2023-12-14T11:17:03Z

Sure thing 😃 I'll try to review ASAP

muuki88

First of all: kudos to the structure of your functional tests. Super comfortable to read and understand. Even though most of them look like they could be unit tests 😄

3 of the 4 failing tests make totally sense. The one with .secure property I don't get, because I'm not familiar with this feature and what the intention is.

My question is: Do you have the capacity to add the missing checks to make the tests green?

muuki88 · 2023-12-20T07:41:05Z

src/test/groovy/org/prebid/server/functional/tests/storage/AccountS3Spec.groovy

+        assert exception.responseBody == "Account $accountId is inactive"
+    }
+
+    def "PBS should throw exception when account have different id inside json in S3 storage"() {


This test makes sense and the implementation is missing this check

muuki88 · 2023-12-20T07:42:49Z

src/test/groovy/org/prebid/server/functional/tests/storage/AmpS3Spec.groovy

+        assert bidderRequest.regs?.gdpr == ampStoredRequest.regs.ext.gdpr
+    }
+
+    def "PBS should throw exception when trying to take parameters from the stored request on S3 service with invalid id in file"() {


This test makes sense and currently fails in the pipeline.

muuki88 · 2023-12-20T07:45:42Z

src/test/groovy/org/prebid/server/functional/tests/storage/AuctionS3Spec.groovy

+        assert bidderRequest.imp[0].secure == secureStoredRequest
+    }
+
+    def "PBS should throw exception when trying populate imp[0].secure from imp stored request on S3 service with invalid impId in file"() {


Suggested change

def "PBS should throw exception when trying populate imp[0].secure from imp stored request on S3 service with invalid impId in file"() {

def "PBS should throw exception when trying to populate imp[0].secure from imp stored request on S3 service with invalid impId in file"() {

This test I don't understand. What is in imp[0].secure that this fails the test? Is this another layer of checksum/validation to ensure the integrity of the configuration?

Okay, got it: https://docs.prebid.org/prebid-server/endpoints/openrtb2/pbs-endpoint-auction.html#determining-bid-security-httphttps

it's a simple flag, forcing https or not

muuki88 · 2023-12-20T07:49:14Z

src/test/groovy/org/prebid/server/functional/tests/storage/StoredResponseS3Spec.groovy

+        assert !bidder.getRequestCount(bidRequest.id)
+    }
+
+    def "PBS should throw request format exception when stored auction response have different id inside S3 file"() {


This is probably sane to do that filename and ID match.

marki1an · 2023-12-20T10:20:55Z

src/test/groovy/org/prebid/server/functional/service/S3Service.groovy

+        this.localStackContainer = new LocalStackContainer(DockerImageName.parse("localstack/localstack:s3-latest"))
+                .withNetwork(Dependencies.network)
+                .withServices(LocalStackContainer.Service.S3)
+        localStackContainer.start()


Creation of LSContainet should be in Dependencies.groovy

marki1an · 2023-12-20T10:30:13Z

src/test/groovy/org/prebid/server/functional/service/S3Service.groovy

+                        localStackContainer.getAccessKey(), localStackContainer.getSecretKey())))
+                .region(Region.of(localStackContainer.getRegion()))
+                .build()
+        createBucket(bucketName)


Would be great to remove call of createBucket(...) from constuructor

marki1an · 2023-12-20T10:59:44Z

src/test/groovy/org/prebid/server/functional/service/S3Service.groovy

+    S3Service(String bucketName) {
+        this.localStackContainer = new LocalStackContainer(DockerImageName.parse("localstack/localstack:s3-latest"))
+                .withNetwork(Dependencies.network)
+                .withServices(LocalStackContainer.Service.S3)
+        localStackContainer.start()
+        s3PbsService = S3Client.builder()
+                .endpointOverride(localStackContainer.getEndpointOverride(LocalStackContainer.Service.S3))
+                .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create(
+                        localStackContainer.getAccessKey(), localStackContainer.getSecretKey())))
+                .region(Region.of(localStackContainer.getRegion()))
+                .build()
+        createBucket(bucketName)


Better formating

Suggested change

S3Service(String bucketName) {

this.localStackContainer = new LocalStackContainer(DockerImageName.parse("localstack/localstack:s3-latest"))

.withNetwork(Dependencies.network)

.withServices(LocalStackContainer.Service.S3)

localStackContainer.start()

s3PbsService = S3Client.builder()

.endpointOverride(localStackContainer.getEndpointOverride(LocalStackContainer.Service.S3))

.credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create(

localStackContainer.getAccessKey(), localStackContainer.getSecretKey())))

.region(Region.of(localStackContainer.getRegion()))

.build()

createBucket(bucketName)

s3PbsService = S3Client.builder()

.endpointOverride(localStackContainer.getEndpointOverride(LocalStackContainer.Service.S3))

.credentialsProvider(

StaticCredentialsProvider.create(

AwsBasicCredentials.create(

localStackContainer.getAccessKey(),

localStackContainer.getSecretKey())))

.region(Region.of(localStackContainer.getRegion()))

.build()

marki1an · 2023-12-20T11:08:20Z

src/test/groovy/org/prebid/server/functional/tests/storage/StorageBaseSpec.groovy

+    protected static final String INVALID_FILE_BODY = 'INVALID'
+    protected static final def DEFAULT_BUCKET = PBSUtils.randomString.toLowerCase()
+
+    protected static final S3Service S3_SERVICE = new S3Service(DEFAULT_BUCKET)


Looks weard when on constant we call method like here S3_SERVICE.uploadStoredResponse(...)
As for me would be great move to lower case s3Service

marki1an · 2023-12-20T11:09:31Z

src/test/groovy/org/prebid/server/functional/tests/storage/StorageBaseSpec.groovy

+class StorageBaseSpec extends BaseSpec {
+
+    protected static final String INVALID_FILE_BODY = 'INVALID'
+    protected static final def DEFAULT_BUCKET = PBSUtils.randomString.toLowerCase()


Pretty minor, for consistency move def to String

marki1an · 2023-12-20T11:11:35Z

src/test/groovy/org/prebid/server/functional/tests/storage/StorageBaseSpec.groovy

+    protected static Map<String, String> s3StorageConfig = [
+            'settings.s3.accessKeyId'         : "${S3_SERVICE.accessKeyId}".toString(),
+            'settings.s3.secretAccessKey'     : "${S3_SERVICE.secretKeyId}".toString(),
+            'settings.s3.endpoint'            : "${S3_SERVICE.endpoint}".toString(),
+            'settings.s3.bucket'              : "${DEFAULT_BUCKET}".toString(),
+            'settings.s3.accounts-dir'        : "${S3Service.DEFAULT_ACCOUNT_DIR}".toString(),
+            'settings.s3.stored-imps-dir'     : "${S3Service.DEFAULT_IMPS_DIR}".toString(),
+            'settings.s3.stored-requests-dir' : "${S3Service.DEFAULT_REQUEST_DIR}".toString(),
+            'settings.s3.stored-responses-dir': "${S3Service.DEFAULT_RESPONSE_DIR}".toString(),
+    ]


Please remove redundant .toString()

Suggested change

protected static Map<String, String> s3StorageConfig = [

'settings.s3.accessKeyId' : "${S3_SERVICE.accessKeyId}".toString(),

'settings.s3.secretAccessKey' : "${S3_SERVICE.secretKeyId}".toString(),

'settings.s3.endpoint' : "${S3_SERVICE.endpoint}".toString(),

'settings.s3.bucket' : "${DEFAULT_BUCKET}".toString(),

'settings.s3.accounts-dir' : "${S3Service.DEFAULT_ACCOUNT_DIR}".toString(),

'settings.s3.stored-imps-dir' : "${S3Service.DEFAULT_IMPS_DIR}".toString(),

'settings.s3.stored-requests-dir' : "${S3Service.DEFAULT_REQUEST_DIR}".toString(),

'settings.s3.stored-responses-dir': "${S3Service.DEFAULT_RESPONSE_DIR}".toString(),

]

protected static Map<String, String> s3StorageConfig = [

'settings.s3.accessKeyId' : s3Service.accessKeyId,

'settings.s3.secretAccessKey' : s3Service.secretKeyId,

'settings.s3.endpoint' : s3Service.endpoint,

'settings.s3.bucket' : DEFAULT_BUCKET,

'settings.s3.accounts-dir' : S3Service.DEFAULT_ACCOUNT_DIR,

'settings.s3.stored-imps-dir' : S3Service.DEFAULT_IMPS_DIR,

'settings.s3.stored-requests-dir' : S3Service.DEFAULT_IMPS_DIR,

'settings.s3.stored-responses-dir': S3Service.DEFAULT_IMPS_DIR,

]

marki1an · 2023-12-20T11:40:11Z

src/test/groovy/org/prebid/server/functional/tests/storage/AccountS3Spec.groovy

+        assert exception.responseBody == "Account $accountId is inactive"
+    }
+
+    def "PBS should throw exception when account have different id inside json in S3 storage"() {


PBS should throw exception when account id is't match wiht bid request account id
Please check other similar case

osulzhenko · 2023-12-20T18:03:36Z

First of all: kudos to the structure of your functional tests. Super comfortable to read and understand. Even though most of them look like they could be unit tests 😄

3 of the 4 failing tests make totally sense. The one with .secure property I don't get, because I'm not familiar with this feature and what the intention is.

My question is: Do you have the capacity to add the missing checks to make the tests green?

@muuki88 It’s possible to add the missing checks here to make the tests pass. However, for more consistency, I suggest adding these checks in your Pull Request (PR). Once done, I can merge them with the current PR.

…to s3-settings-functional-tests

muuki88 · 2024-01-22T19:40:59Z

@SerhiiNahornyi , @marki1an I need to push back on some of the failing tests, as they are currently very hard to implement.

The StoredDataResult model contains the results untyped. So this validation is currently not available in any ApplicationSettings implementation (FilesystemSettings, JdbcSettings).

I would argue that these kind of validations should be done in a generic way a level higher. Could you remove the failing tests?

marki1an · 2024-01-24T09:26:47Z

@SerhiiNahornyi , @marki1an I need to push back on some of the failing tests, as they are currently very hard to implement.

The StoredDataResult model contains the results untyped. So this validation is currently not available in any ApplicationSettings implementation (FilesystemSettings, JdbcSettings).

I would argue that these kind of validations should be done in a generic way a level higher. Could you remove the failing tests?

@muuki88 Hi there.
Sure, we'll mark those tests with @PedningFeature annotation, and those tests will skip while annotation present.
@osulzhenko Can you mark those tests?

osulzhenko · 2024-01-24T11:21:29Z

@SerhiiNahornyi , @marki1an I need to push back on some of the failing tests, as they are currently very hard to implement.
The StoredDataResult model contains the results untyped. So this validation is currently not available in any ApplicationSettings implementation (FilesystemSettings, JdbcSettings).
I would argue that these kind of validations should be done in a generic way a level higher. Could you remove the failing tests?

@muuki88 Hi there. Sure, we'll mark those tests with @PedningFeature annotation, and those tests will skip while annotation present. @osulzhenko Can you mark those tests?

@muuki88 sure, marked

muuki88 · 2024-01-25T09:12:42Z

@osulzhenko anything that blocks this?

osulzhenko · 2024-01-25T15:01:55Z

@osulzhenko anything that blocks this?

@muuki88 Not from my side. You need to get approval on your original PR #2733

Net-burst · 2024-03-12T15:18:47Z

The workflow still fails on S3 unit tests:

Error:  Errors: 
Error:    S3ApplicationSettingsTest.getStoredDataReturnsErrorsForNotFoundImpressions:359 » Sdk
Error:    S3ApplicationSettingsTest.getStoredDataReturnsErrorsForNotFoundRequests:329 » Sdk
Error:    S3ApplicationSettingsTest.getStoredDataShouldReturnFetchedStoredImpression » ClassCast
Error:    S3ApplicationSettingsTest.getStoredDataShouldReturnFetchedStoredImpressionAndStoredRequest » ClassCast
Error:    S3ApplicationSettingsTest.getStoredDataShouldReturnFetchedStoredImpressionWithAdUnitPathStoredId » ClassCast
Error:    S3ApplicationSettingsTest.getStoredDataShouldReturnFetchedStoredRequest » ClassCast
[INFO] 
Error:  Tests run: 7271, Failures: 0, Errors: 6, Skipped: 0

…nctional-tests # Conflicts: # pom.xml # src/test/groovy/org/prebid/server/functional/testcontainers/Dependencies.groovy

…nctional-tests

…ings/copy' into s3-settings-functional-tests

osulzhenko requested review from Net-burst, SerhiiNahornyi and marki1an December 13, 2023 17:53

Add functional tests for S3 storage support

371673a

osulzhenko force-pushed the s3-settings-functional-tests branch from 8abb1a6 to 371673a Compare December 14, 2023 08:16

osulzhenko added the tests Functional or other tests label Dec 15, 2023

muuki88 approved these changes Dec 20, 2023

View reviewed changes

marki1an requested changes Dec 20, 2023

View reviewed changes

update after review

18696f2

osulzhenko added 5 commits December 21, 2023 10:28

update after review

7c17435

update after review

2a9836d

Add clear bucket method

4f7deb0

Add clear bucket method

5594ecf

Merge remote-tracking branch 'origin/s3-settings-functional-tests' in…

2896dbe

…to s3-settings-functional-tests

marki1an previously approved these changes Dec 21, 2023

View reviewed changes

osulzhenko added 2 commits January 24, 2024 13:07

Merge branch 'highfivve-github-master' into s3-settings-functional-tests

a3d1fdb

pending validation for matching records in S3

a0dd561

osulzhenko dismissed marki1an’s stale review via a0dd561 January 24, 2024 11:19

osulzhenko added 3 commits August 21, 2024 09:48

Merge branch 'refs/heads/highfivve-github-master' into s3-settings-fu…

1a92ff0

…nctional-tests # Conflicts: # pom.xml # src/test/groovy/org/prebid/server/functional/testcontainers/Dependencies.groovy

update functional tests

1880058

Merge branch 'refs/heads/highfivve-github-master' into s3-settings-fu…

b06b0c7

…nctional-tests

add force-path-style setting

c1bcd04

osulzhenko requested a review from marki1an August 23, 2024 07:11

Compile-Ninja changed the base branch from master to s3-application-settings/copy September 3, 2024 09:41

marki1an added 3 commits September 3, 2024 12:55

Merge remote-tracking branch 'refs/remotes/origin/s3-application-sett…

479e2d5

…ings/copy' into s3-settings-functional-tests

Update gdpr

e59aedb

Update secure field

ab0d4c1

marki1an approved these changes Sep 3, 2024

View reviewed changes

Compile-Ninja merged commit 65d28dc into s3-application-settings/copy Sep 3, 2024

Compile-Ninja deleted the s3-settings-functional-tests branch September 3, 2024 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 settings functional tests #2837

S3 settings functional tests #2837

osulzhenko commented Dec 13, 2023

SerhiiNahornyi commented Dec 14, 2023

muuki88 commented Dec 14, 2023

muuki88 left a comment

muuki88 Dec 20, 2023

muuki88 Jan 3, 2024

muuki88 Dec 20, 2023

muuki88 Dec 20, 2023

muuki88 Dec 20, 2023

muuki88 Dec 20, 2023

marki1an Dec 20, 2023

osulzhenko Dec 20, 2023

marki1an Dec 20, 2023

marki1an Dec 20, 2023

osulzhenko Dec 20, 2023

marki1an Dec 20, 2023

osulzhenko Dec 20, 2023

marki1an Dec 20, 2023

osulzhenko Dec 20, 2023

marki1an Dec 20, 2023

osulzhenko Dec 20, 2023

marki1an Dec 20, 2023

osulzhenko Dec 20, 2023

osulzhenko commented Dec 20, 2023

muuki88 commented Jan 22, 2024

marki1an commented Jan 24, 2024

osulzhenko commented Jan 24, 2024 •

edited

Loading

muuki88 commented Jan 25, 2024

osulzhenko commented Jan 25, 2024

Net-burst commented Mar 12, 2024

	def "PBS should throw exception when trying populate imp[0].secure from imp stored request on S3 service with invalid impId in file"() {
	def "PBS should throw exception when trying to populate imp[0].secure from imp stored request on S3 service with invalid impId in file"() {

S3 settings functional tests #2837

S3 settings functional tests #2837

Conversation

osulzhenko commented Dec 13, 2023

SerhiiNahornyi commented Dec 14, 2023

muuki88 commented Dec 14, 2023

muuki88 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osulzhenko commented Dec 20, 2023

muuki88 commented Jan 22, 2024

marki1an commented Jan 24, 2024

osulzhenko commented Jan 24, 2024 • edited Loading

muuki88 commented Jan 25, 2024

osulzhenko commented Jan 25, 2024

Net-burst commented Mar 12, 2024

osulzhenko commented Jan 24, 2024 •

edited

Loading