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

Codecov Report

❌ Patch coverage is 50.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.31%. Comparing base (0fec989) to head (768b263).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
lib/routes/routeBackbeat.js	33.33%	6 Missing ⚠️

❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/data/wrapper.js	`78.37% <100.00%> (+1.90%)`	⬆️
lib/routes/routeBackbeat.js	`75.79% <33.33%> (+2.40%)`	⬆️

... and 27 files with indirect coverage changes

@@                                       Coverage Diff                                       @@
##           improvement/CLDSRV-724-service-get-related-functional-tests    #5985      +/-   ##
===============================================================================================
+ Coverage                                                        81.00%   84.31%   +3.30%     
===============================================================================================
  Files                                                              204      204              
  Lines                                                            12890    12902      +12     
===============================================================================================
+ Hits                                                             10442    10878     +436     
+ Misses                                                            2448     2024     -424

Flag	Coverage Δ
file-ft-tests	`67.52% <0.00%> (+7.39%)`	⬆️
kmip-ft-tests	`28.08% <0.00%> (-0.03%)`	⬇️
mongo-v0-ft-tests	`68.75% <0.00%> (+7.28%)`	⬆️
mongo-v1-ft-tests	`68.87% <0.00%> (+7.40%)`	⬆️
multiple-backend	`35.25% <25.00%> (-0.01%)`	⬇️
sur-tests	`35.53% <0.00%> (-0.04%)`	⬇️
sur-tests-inflights	`37.38% <0.00%> (-0.04%)`	⬇️
unit	`69.88% <25.00%> (-0.05%)`	⬇️
utapi-v2-tests	`34.21% <0.00%> (-0.04%)`	⬇️

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.

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 ?

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 🤔

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 ?

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

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)

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

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 ?

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

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 🤔

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 ?

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 3 commits December 4, 2025 12:41

commenting out ceph tests from CI

b534e28

getlocation functional aws-node-sdk test bucket lint

3699de7

multiple backend aws-node-sdk lint fixup

b3e16fb

benzekrimaha force-pushed the improvement/CLDSRV-724-backbeat-related-functional-tests branch from 8383cf9 to b3e16fb Compare December 4, 2025 11:41

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

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

Uh oh!

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!

Codecov Report

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!

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!

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!

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!

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!

3 participants

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

SylvainSenechal Dec 8, 2025 •

edited

Loading

SylvainSenechal Dec 9, 2025 •

edited

Loading