Improvement/cldsrv 724 kms tests migration #5951

benzekrimaha · 2025-09-24T15:44:17Z

codecov · 2025-09-24T15:46:50Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.04%. Comparing base (adb7ac5) to head (55803c6).

Additional details and impacted files

see 74 files with indirect coverage changes

@@                            Coverage Diff                             @@
##           improvement/CLDSRV-724-sur-utapi-tests    #5951      +/-   ##
==========================================================================
- Coverage                                   79.61%   72.04%   -7.57%     
==========================================================================
  Files                                         191      191              
  Lines                                       12233    12233              
==========================================================================
- Hits                                         9739     8813     -926     
- Misses                                       2494     3420     +926

Flag	Coverage Δ
ceph-backend-test	`19.78% <ø> (-44.82%)`	⬇️
file-ft-tests	`22.27% <ø> (ø)`
kmip-ft-tests	`27.19% <ø> (ø)`
mongo-v0-ft-tests	`28.14% <ø> (ø)`
mongo-v1-ft-tests	`28.14% <ø> (ø)`
multiple-backend	`19.81% <ø> (ø)`
sur-tests	`34.76% <ø> (-0.86%)`	⬇️
sur-tests-inflights	`36.70% <ø> (ø)`
unit	`68.32% <ø> (ø)`
utapi-v2-tests	`33.55% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DarkIsDude · 2025-10-02T08:54:24Z

tests/functional/sse-kms-migration/beforeMigration.js

                // ensure versioned bucket is empty
                await helpers.bucketUtil.empty(bkt.vname);
-                let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }).promise();
+                let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || [];


Versions can then be undefined ?

Should probably be like this to ensure we do not have an error trying to access Versions from an array that has no fields

Suggested change

let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || [];

let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || { Versions: [] };

DarkIsDude · 2025-10-02T08:55:02Z

tests/functional/sse-kms-migration/cleanup.js

        console.log('Run cleanup',
            { profile: helpers.credsProfile, accessKeyId: helpers.s3.config.credentials.accessKeyId });
-        const allBuckets = (await helpers.s3.listBuckets().promise()).Buckets.map(b => b.Name);
+        const allBuckets = ((await helpers.s3.listBuckets()).Buckets || []).map(b => b.Name);


We have same stuff for other tests (for example in beforeMigration). Seems like a debug ? Should we keep it ? Check the length ?

what do you mean by the same stuff , are you referring to the console logs ? as they have been there before they're being kept

DarkIsDude · 2025-10-02T08:56:50Z

tests/functional/sse-kms-migration/scenarios.js

 }

 async function getBucketSSEError(Bucket) {
-    await assert.rejects(helpers.s3.getBucketEncryption({ Bucket }).promise(), err => {


Why we don't keep the reject assertion ?

the try-catch approach provides clearer information in this case thus the replacement

DarkIsDude · 2025-10-02T08:56:57Z

tests/functional/sse-kms-migration/scenarios.js

 // check MPU headers against the MPU overview MD
 // because there is no migration for ongoing MPU
 function assertMPUSSEHeaders(actual, expected, algo) {
+    // eslint-disable-next-line no-console


To be deleted 🙏

DarkIsDude · 2025-10-02T08:58:37Z

tests/functional/sse-kms-migration/scenarios.js


 // before has no headers to assert
 async function mpuComplete({ UploadId, Bucket, Key }, { existingParts, newParts }, mpuOverviewMDSSE, algo, testCase) {
+    const extractETag = part => {


As we are in a test, we should prefer assert instead of throw ? Also return part.CopyPartResult?.ETag || part.ETag || undefined is better with a check that the result is not undefined ?

DarkIsDude · 2025-10-02T09:00:00Z

tests/functional/sse-kms-migration/helpers.js

-const s3config = getConfig(credsProfile, { signatureVersion: 'v4' });
-const s3 = new S3(s3config);
+
+// Create custom agents with specific pooling settings


Suggested change

// Create custom agents with specific pooling settings

if you want to make it clear, you can change the name of the var httpAgent with httpAgentWithSpecificPooling if this is important when we read tests

DarkIsDude · 2025-10-02T09:01:41Z

tests/functional/sse-kms-migration/helpers.js

+
 const bucketUtil = new BucketUtility(credsProfile);

+// Wrapper for SDK v3 commands to return promises directly


Suggested change

// Wrapper for SDK v3 commands to return promises directly

Your code is clear enough

DarkIsDude · 2025-10-02T09:06:11Z

tests/functional/sse-kms-migration/helpers.js

+
+const s3Client = new S3Client(s3config);
+
+// Remove logger middleware if present


Suggested change

// Remove logger middleware if present

DarkIsDude · 2025-10-02T09:06:53Z

tests/functional/sse-kms-migration/helpers.js

-        .Rules[0]
-        .ApplyServerSideEncryptionByDefault;
+    try {
+        const sse = await s3Client.send(new GetBucketEncryptionCommand({ Bucket }));


Should you use your wrapper instead of s3Client ?

DarkIsDude · 2025-10-02T09:08:00Z

tests/functional/sse-kms-migration/arnPrefix.js

            action: 'copyObject', detail: ' when getting from source', kmsAction: 'Decrypt',
            fct: async () =>
-                helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }).promise(),
+                helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }) ,


Suggested change

helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }) ,

helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }),

You have some other iterations of that in your file

williamlardier · 2025-10-02T09:54:34Z

tests/functional/sse-kms-migration/beforeMigration.js

                // ensure versioned bucket is empty
                await helpers.bucketUtil.empty(bkt.vname);
-                let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }).promise();
+                let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || [];


Should probably be like this to ensure we do not have an error trying to access Versions from an array that has no fields

Suggested change

let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || [];

let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || { Versions: [] };

williamlardier · 2025-10-02T10:00:38Z

tests/functional/sse-kms-migration/helpers.js

+        httpAgent,
+        httpsAgent,
+    }),
+    maxAttempts: 8,


Usually we can keep 3, this should be enough (and one could argue it should be 1 for a test suite, as if we hide transient errors, we might miss real bugs; and in such case we could just remove the retry logic)

Issue: CLDSRV-724

SylvainSenechal · 2025-10-15T16:09:57Z

tests/functional/sse-kms-migration/helpers.js

+
+const s3Client = new S3Client(s3config);
+
+if (s3Client.middlewareStack.identify().includes('loggerMiddleware')) {


I think it deserves a comment, why are we doing this ? Is this removing some logs to make the test less verbose ?

SylvainSenechal · 2025-10-15T16:22:12Z

package.json

  },
  "homepage": "https://github.com/scality/S3#readme",
  "dependencies": {
    "@aws-sdk/client-s3": "^3.705.0",


nit: 705 is ~11 months old, you can use 910 if you want

SylvainSenechal

Nothing much to change here, but tests are failing

benzekrimaha force-pushed the improvement/CLDSRV-724-kms-tests-migration branch from d6238a4 to 0bd90df Compare September 25, 2025 06:28

DarkIsDude requested changes Oct 2, 2025

View reviewed changes

williamlardier reviewed Oct 2, 2025

View reviewed changes

benzekrimaha force-pushed the improvement/CLDSRV-724-sur-utapi-tests branch 4 times, most recently from 1f24c59 to adb7ac5 Compare October 10, 2025 15:10

benzekrimaha added 3 commits October 13, 2025 09:06

kms tests related code update after sdk migration

eb79537

Issue: CLDSRV-724

dependencies update

f1e5bd8

Issue: CLDSRV-724

post reviews fixups

55803c6

benzekrimaha force-pushed the improvement/CLDSRV-724-kms-tests-migration branch from a8406fa to 55803c6 Compare October 13, 2025 07:07

SylvainSenechal reviewed Oct 15, 2025

View reviewed changes

	let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) \|\| [];
	let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) \|\| { Versions: [] };


		const bucketUtil = new BucketUtility(credsProfile);

		// Wrapper for SDK v3 commands to return promises directly


		const s3Client = new S3Client(s3config);

		// Remove logger middleware if present

	helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }) ,
	helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }),


		const s3Client = new S3Client(s3config);

		if (s3Client.middlewareStack.identify().includes('loggerMiddleware')) {

Improvement/cldsrv 724 kms tests migration #5951

Are you sure you want to change the base?

Improvement/cldsrv 724 kms tests migration #5951

Uh oh!

Conversation

benzekrimaha commented Sep 24, 2025

Uh oh!

codecov bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 24, 2025 •

edited

Loading