S3 settings functional tests

[osulzhenko]

[SerhiiNahornyi]

@muuki88 We added a few tests for your PR https://github.com/prebid/prebid-server-java/pull/2837

Could you pls review them with us?

[muuki88]

Sure thing 😃 I'll try to review ASAP

[osulzhenko]

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.

[muuki88]

@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]

@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]

@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]

@osulzhenko anything that blocks this?

[osulzhenko]

@osulzhenko anything that blocks this?

@muuki88 Not from my side. You need to get approval on your original PR https://github.com/prebid/prebid-server-java/pull/2733

[Net-burst]

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