Improvement/cldsrv 724-backbeat-related-functional-tests #5985

benzekrimaha · 2025-10-23T16:14:03Z

Please note that this PR also takles the rest of the unmigrated code , better to review it by commit to have the context

Issue: CLDSRV-724

codecov · 2025-11-05T08:18:31Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
6785	1	6784	0

View the full list of 1 ❄️ flaky test(s)

"after each" hook for "should create a bunch of objects and their versions"::put and head object with versioning With default signature "after each" hook for "should create a bunch of objects and their versions"

Flake rate in main: 100.00% (Passed 0 times, Failed 3 times)

Stack Traces | 0.972s run time

The bucket you tried to delete is not empty.

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

SylvainSenechal · 2025-12-08T15:26:15Z

lib/routes/routeBackbeat.js

                return metadata.putObjectMD(bucketName, objectKey, omVal, options, log,
                    (err, md) => {
                        if (err) {
+                            // Handle duplicate key error during repair operation


How come we have this in sdkv3 migration ? Is it because you found a bug/flaky test and needed this to fix it ?

yes , this was found in tests flakiness and was added to fix it

SylvainSenechal · 2025-12-08T15:27:32Z

lib/data/wrapper.js

    client = new DataFileInterface(config);
    implName = 'file';
 } else if (config.backends.data === 'multiple') {
+    Object.keys(config.locationConstraints).filter(k => 


(writing this without having read the whole pr) : Seeing this feels a bit weird, we don't really know why it's done, maybe add a comment to explain where this is used ? Also, the result of the map is not assigned to anything 🤔

Edit: Not too sure, just leaving this here so that another reviewer can double check

tests/functional/aws-node-sdk/test/bucket/getLocation.js

SylvainSenechal · 2025-12-08T16:17:22Z

tests/functional/aws-node-sdk/test/bucket/getLocation.js

                const data = await s3.send(new GetBucketLocationCommand({ Bucket: bucketName }));
-                assert.strictEqual(data.LocationConstraint, endpoint);
+
+                // S3C backend has 'dc-1' as default location constraint


Why didn't we have to do such thing for the previous test though 🤔

because this is the branch we bumped arsenal in with the v3 migration so additional adaptations needed to be made

SylvainSenechal · 2025-12-08T16:19:38Z

tests/functional/aws-node-sdk/test/bucket/putBucketLifecycle.js

-
-        describe('with NoncurrentVersionTransitions', () => {
+        // NoncurrentVersionTransitions not implemented
+        describe.skip('with NoncurrentVersionTransitions', () => {


The test isn't skipped on the left, (but it's already sdk v3 code).
Was the test skipped before updating it to v3 ?

yes it was already skipped before the migration all together

SylvainSenechal · 2025-12-08T16:27:24Z

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js

General comment as im still early in the pr : It feels weird to review because the code being replaced is already migrated to v3, so it looks more like a refactoring ?
For example here, beforeEach is modified to async/await
But also, afterEach is logically modified, as the putBucketLogging didn't exist before 🤔

SylvainSenechal · 2025-12-08T16:28:13Z

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js

-                        if (err && err.name !== 'NoSuchBucket') {
-                            return done(err);
+                try {
+                    await s3.send(new PutBucketLoggingCommand({


why doing this in an afterEach (asking because usually, afterEach is cleanup delete commands, not post/put) ? Also it wasn't there in the old code

SylvainSenechal · 2025-12-08T17:07:30Z

tests/functional/backbeat/bucketIndexing.js

+let credentials = null;
+let backbeatAuthCredentials = null;
+
+async function getCredentials() {


i feel like you could've kept the credentials initialization at the top level ?
If not, then I think it's better to move the declaration of backbeatAuthCredentials in the beforeAll too

it is still at the same level ? I'm only declaring the vars used in the function before

SylvainSenechal · 2025-12-09T11:09:37Z

tests/functional/aws-node-sdk/test/multipleBackend/acl/aclAwsVersioning.js

-                assert.strictEqual(versionId, undefined);
-                getObjectAndAssertAcl(s3, { bucket, key, body: someBody,
-                    expectedResult: testAcp }, done);
+            waitForVersioningBeforePut(s3, bucket, err => {


How come we need this now, but didn't need it before (i saw it added to other tests in other files too)

these tests are ceph tests , and we need more time for operation on ceph that's why

SylvainSenechal · 2025-12-09T11:25:13Z

tests/functional/aws-node-sdk/test/multipleBackend/get/get.js

+                        ContentLength: partBody.length,
+                    };
+                    const uploadPartCommand = new UploadPartCommand(uploadPartInput);
+                    uploadPartCommand.middlewareStack.add(next => async args => {


I'm surprised this is needed, as you added content length in the command already. Can probably leave as it's a test, but with more time it would be nice to retest it by retrying the test without the middleware

I tried without the middleware and it didn't work as ceph is less permissive than aws is ( this is what I was talking about in the daily )

SylvainSenechal · 2025-12-09T13:51:31Z

tests/functional/aws-node-sdk/test/multipleBackend/mpuComplete/completeMPUGcp.js

        const params = { Bucket: gcpBucket, Key: gcpKey };
        gcpClient.getObject(params, (err, gcpRes) => {
-            assert.equal(err, null, `Err getting object from GCP: ${err}`);
+            if (err) {


why did you replace assert error nil with cb(err) ? I'm guessing there is not much impact as the error is catched later ?

to return directly instead of the timeout waiting for the assertion , tried to do that whenever I could , because it's painful to debug in case of failure with the assertions

SylvainSenechal · 2025-12-09T13:55:29Z

tests/functional/aws-node-sdk/test/multipleBackend/mpuComplete/completeMPUGcp.js

+                                .catch(err => {
+                                    assert.equal(err, null,
+                                        `Err completing MPU: ${err}`);
+                                    done(err);


I guess this will never be reach since the error is always non nil in the catch

indeed this was fixed

SylvainSenechal · 2025-12-09T13:57:20Z

tests/functional/aws-node-sdk/test/multipleBackend/mpuComplete/completeMPUGcp.js

+                                setTimeout(() => {
+                                    this.test.awsClient.getObject({ Bucket: awsBucket,
+                                    Key: this.test.key }, err => {
+                                        assert.strictEqual(err.code, 'NoSuchKey');


I'm surprised this test isn't problematic, as I guess here it should be err.name 🤔

this is gcp returning not aws

SylvainSenechal · 2025-12-09T14:07:18Z

tests/functional/aws-node-sdk/test/multipleBackend/objectCopy/azureObjectCopy.js

 let bucketUtil;
 let s3;

+function normalizeMetadata(metadata) {


I think it's worth it to explain why this is needed now
Also it looks this is used to modify the result to compare it with an expected value, maybe it would've been better to not modify the result, and modify the expected value without normalization instead (edit: especially since it looks like the function is only called twice and not on every test) ?

SylvainSenechal · 2025-12-09T14:38:49Z

tests/functional/aws-node-sdk/test/multipleBackend/utils.js

+                if (attempt < MAX_VERSIONING_CHECKS) {
+                    await new Promise(resolve => setTimeout(resolve, VERSIONING_CHECK_INTERVAL));
+                } else {
+                    if (callback) {


if we've reached max attempt, and get bucket versioning doesnt't fail but also doesn't return status enabled, should'nt this return an error ?

SylvainSenechal · 2025-12-09T14:40:03Z

tests/functional/aws-node-sdk/test/multipleBackend/utils.js


-utils.putToAwsBackend = async (s3, bucket, key, body) => {
-    const result = await s3.send(new PutObjectCommand({ 
+utils.waitForVersioningBeforePut = async (s3, bucket, callback) => {


In what context would we need to wait a bit for the versioning to take effect, and why this is needed now but not before ?
I guess we have some test that setup bucket versioning, but it should take effect directly no ?

Reading the end of the file, if I understand correctly this is only needed for the Ceph tests that will eventually be removed ?

indeed it's for ceph as precised before in the comment regarding the wait

SylvainSenechal · 2025-12-09T14:54:06Z

tests/functional/raw-node/test/GCP/bucket/get.js

        it('should return 404 and NoSuchBucket', done => {
            const badBucketName = `nonexistingbucket-${genUniqID()}`;
-            gcpClient.getBucket({
+            gcpClient.listObjects({


Why changing from getBucket to listObject ? I guess the test is still valid because listObject will also return noSuchBucket but still, here we want to test the getBucket api I think

SylvainSenechal · 2025-12-09T15:26:34Z

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

 * round robin is confined to each nodejs cluster processes
 */
-const APPROX = Math.floor(0.1 * TOTAL_OBJECTS_PER_NODE);
+const APPROX = Math.ceil(0.2 * TOTAL_OBJECTS_PER_NODE);


do you remember what happened here that you need to do this change ?

SylvainSenechal

bigbig pr again. this time I left a bit more comments but actually not many changes requested, its mostly questions to double check the changes, and make sure we understand why the changes are done because I found 2/3 things were curious

benzekrimaha force-pushed the improvement/CLDSRV-724-service-get-related-functional-tests branch 3 times, most recently from 72dd6ed to cd35cca Compare October 28, 2025 15:59

benzekrimaha force-pushed the improvement/CLDSRV-724-backbeat-related-functional-tests branch 2 times, most recently from 425731c to 2b9959d Compare October 28, 2025 16:03

scality deleted a comment from codecov bot Nov 5, 2025

benzekrimaha force-pushed the improvement/CLDSRV-724-backbeat-related-functional-tests branch 5 times, most recently from 8c9eb10 to 3ab4eae Compare November 13, 2025 10:44

benzekrimaha force-pushed the improvement/CLDSRV-724-backbeat-related-functional-tests branch 2 times, most recently from 17d2fcb to 198582c Compare November 20, 2025 17:19

benzekrimaha force-pushed the improvement/CLDSRV-724-backbeat-related-functional-tests branch 3 times, most recently from 2c806e4 to 836c556 Compare November 28, 2025 15:18

benzekrimaha force-pushed the improvement/CLDSRV-724-backbeat-related-functional-tests branch 2 times, most recently from 26ce909 to 8383cf9 Compare December 2, 2025 17:24

benzekrimaha force-pushed the improvement/CLDSRV-724-service-get-related-functional-tests branch from cd35cca to 0fec989 Compare December 4, 2025 06:39

benzekrimaha changed the title ~~Improvement/cldsrv 724 backbeat related functional tests~~ Improvement/cldsrv 724-backbeat-related-functional-tests Dec 4, 2025

benzekrimaha added 9 commits December 4, 2025 12:35

backbeat functional tests migration

a1fff68

aws-node-sdk bucket functional tests migration

91fe932

aws-node-sdk multiple backend tests migration

7408990

raw-node gcp object and bucket tests migration

0ff38c1

routBackbeat migration adaptation

1c371b1

aws-node-sdk test utils migration

03dc295

raw-node rest of code migration

ac83417

rest of code migration

b07e438

deps install

c2f4c3a

benzekrimaha added 5 commits December 4, 2025 13:00

arsenal rebump

536ef7f

new bucket logging and rate limit tests migration

1af0814

new bucket logging and rate limit tests migration

1ce0df8

new bucket logging and rate limit tests migration

cede1b9

new bucket logging and rate limit tests migration

553c156

benzekrimaha marked this pull request as ready for review December 4, 2025 13:38

latest arsenal bump

768b263

SylvainSenechal reviewed Dec 8, 2025

View reviewed changes

tests/functional/aws-node-sdk/test/bucket/getLocation.js Show resolved Hide resolved

SylvainSenechal reviewed Dec 8, 2025

View reviewed changes

SylvainSenechal reviewed Dec 9, 2025

View reviewed changes

SylvainSenechal requested changes Dec 9, 2025

View reviewed changes

fixups post reviews

5313153

benzekrimaha requested review from a team, SylvainSenechal and delthas December 11, 2025 18:40

Improvement/cldsrv 724-backbeat-related-functional-tests #5985

Are you sure you want to change the base?

Improvement/cldsrv 724-backbeat-related-functional-tests #5985

Conversation

benzekrimaha commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

❌ 1 Tests Failed:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

benzekrimaha Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

benzekrimaha commented Oct 23, 2025 •

edited

Loading

codecov bot commented Nov 5, 2025 •

edited

Loading

SylvainSenechal Dec 8, 2025 •

edited

Loading

SylvainSenechal Dec 8, 2025 •

edited

Loading

benzekrimaha Dec 11, 2025 •

edited

Loading

SylvainSenechal Dec 8, 2025 •

edited

Loading

SylvainSenechal Dec 9, 2025 •

edited

Loading

benzekrimaha Dec 11, 2025 •

edited

Loading