adding tests for externalclients and gcpservice #2579

benzekrimaha · 2025-12-05T16:45:07Z

DarkIsDude · 2025-12-05T20:13:16Z

tests/unit/storage/data/DummyService.js

+    async deleteIfExists() {
+        if (this.key === 'externalBackendTestBucket/externalBackendMissingKey') {
+            return { succeeded: false };
+        }
+        return { succeeded: true };
+    }


I don't know how you imagine your futur but you can do :

return { succeeded: this.key !== 'externalBackendTestBucket/externalBackendMissingKey' }

DarkIsDude · 2025-12-05T20:13:38Z

tests/unit/storage/data/external/GcpService.spec.js

 function invalidDnsBucketNameHandler() {
    return (req, res) => {
-        assert(req.headers.host, host);
+         assert(req.headers.host, host);


Indent issue ?

tests/unit/storage/data/external/GcpService.spec.js

codecov · 2025-12-09T17:44:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.72%. Comparing base (00033b5) to head (aed443f).
⚠️ Report is 9 commits behind head on development/8.3.

Additional details and impacted files

@@                 Coverage Diff                 @@
##           development/8.3    #2579      +/-   ##
===================================================
+ Coverage            71.01%   72.72%   +1.71%     
===================================================
  Files                  221      221              
  Lines                18176    18070     -106     
  Branches              3773     3737      -36     
===================================================
+ Hits                 12908    13142     +234     
+ Misses                5263     4923     -340     
  Partials                 5        5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bert-e · 2026-01-07T18:32:13Z

Hello benzekrimaha,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2026-01-12T15:23:04Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

This commit aims to improve code clarity by renaming methods and files related to GCP APIs to more descriptive names. The changes include renaming multipart upload methods and object tagging methods to better reflect their functionality. Enhancing code readability and maintainability. Issue: ARSN-524

This commit remove unused methods from the GcpClient class as they are not needed anymore. Issue: ARSN-524

This commit tackles both the removal of the command wrapper functions in the GcpService as they add no additional functonality, calling the send method directly is a better approach. the class name was also updated to GcpService to better reflect its purpose and avoid confusion with the client. Issue: ARSN-524

Issue: ARSN-524

Some multipart upload tests were missing coverage for ExternalClients. This commit adds those missing tests. Issue: ARSN-524

With the refactor of GCP service within the migration from aws-sdk v2 to v3, we noticed that some code paths were not covered by unit tests. this commit adds tests to cover helper functions and mpu/object tagging related functions. Issue: ARSN-524

lib/storage/data/external/GCP/GcpService.js

francoisferrand · 2026-01-13T15:44:55Z

lib/storage/data/external/GCP/GcpService.js

 }

-Object.assign(GcpClient.prototype, gcpApis);
+Object.assign(GcpService.prototype, gcpApis);


can we move this to the constructor of GcpService, to make it easier to notice?

(or even rewrite this without Object.assign : it was done to "extend" the generated class, but now the class is manually written: so we may more cleanly add the methods?)

Keeping GcpApis separate avoids a noisy/huge diff and makes it easier to iterate on these highly customized APIs in isolation. I kept the Object.assign(GcpService.prototype, gcpApis) at module scope (not in the constructor) because putting it in the constructor would re-run for every new GcpService(), repeatedly overwriting prototype methods.

Yeah not much we can do here. Maybe replace this line with something explicit

Suggested change

Object.assign(GcpService.prototype, gcpApis);

GcpClient.prototype.abortMultipartUpload = gcpApis.abortMultipartUpload;

GcpClient.prototype.completeMultipartUpload = gcpApis.completeMultipartUpload;

GcpClient.prototype.createMultipartUpload = gcpApis.createMultipartUpload;

...

...

...

This way it won't be 'hidden' into a single line, but not sure its super nice either

putting it in the constructor would re-run for every new GcpService(), repeatedly overwriting prototype methods.

constructor() { Object.assign(this, gcpApis); }

putting it in the constructor would not overwrite prototype method: only the "current" object's methods.
Would that be an issue?

lib/storage/data/external/GCP/GcpService.js

tests/unit/storage/data/external/GcpService.spec.js

SylvainSenechal · 2026-01-15T10:04:08Z

tests/unit/storage/data/DummyService.js

        };
    }
+
+    async deleteIfExists() {


I don't see this function being used anywhere 🤔 except once in code that was already existing in azureClient test. How come the test didn't fail without this function

SylvainSenechal · 2026-01-15T10:14:40Z

tests/unit/storage/data/external/ExternalClients.spec.js

        });

+        it(`${backend.name} delete() should delete the requested key without error`, async () => {
+            const key = 'externalBackendTestKey';


nit: seems these key/bucket names are important as they are also hardcoded in the dummy service, but we could consider using constants dummyKey/dummyBucketName defined in dummy service and importing them

francoisferrand

with this, are we ready for 8.3.0 release?

if we don't have anything, should bump in this PR so we can immediately release
or align with @DarkIsDude to release once #2581 is merged (on 8.2, with 8.3 waterflow), merge this one "as is", and do the bump #2581.

…command Issue: ARSN-524

Issue: ARSN-524

benzekrimaha · 2026-01-16T14:15:17Z

/approve

bert-e · 2026-01-16T14:22:56Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/8.3

The following branches have NOT changed:

development/6.4
development/7.10
development/7.4
development/7.70
development/8.1
development/8.2

Please check the status of the associated issue ARSN-524.

Goodbye benzekrimaha.

The following options are set: approve

benzekrimaha force-pushed the improvement/ARSN-524 branch from 146c259 to 6166515 Compare December 5, 2025 17:11

benzekrimaha marked this pull request as ready for review December 5, 2025 17:11

benzekrimaha requested review from a team, DarkIsDude and francoisferrand December 5, 2025 17:11

DarkIsDude approved these changes Dec 5, 2025

View reviewed changes

benzekrimaha force-pushed the improvement/ARSN-524 branch from 82141f3 to 6166515 Compare December 9, 2025 17:44

benzekrimaha force-pushed the improvement/ARSN-514 branch 2 times, most recently from ed14c60 to b978085 Compare December 16, 2025 16:17

benzekrimaha force-pushed the improvement/ARSN-514 branch 2 times, most recently from e36f845 to e66bc75 Compare December 19, 2025 10:31

SylvainSenechal force-pushed the improvement/ARSN-514 branch 2 times, most recently from 6e00a70 to 00033b5 Compare January 7, 2026 18:17

Base automatically changed from improvement/ARSN-514 to development/8.3 January 7, 2026 18:32

benzekrimaha force-pushed the improvement/ARSN-524 branch 2 times, most recently from 86bcf63 to bf05ffe Compare January 12, 2026 15:22

scality deleted a comment from bert-e Jan 12, 2026

francoisferrand marked this pull request as draft January 13, 2026 12:27

benzekrimaha force-pushed the improvement/ARSN-524 branch from 23c12d3 to 5ca7b1d Compare January 13, 2026 14:48

benzekrimaha added 5 commits January 13, 2026 15:49

remove unused gcp client methods

aa561e4

This commit remove unused methods from the GcpClient class as they are not needed anymore. Issue: ARSN-524

use aws commands instead of gcpService wrapper functions

c756840

Issue: ARSN-524

coverage amelioration tests for externalclients

695920f

Some multipart upload tests were missing coverage for ExternalClients. This commit adds those missing tests. Issue: ARSN-524

benzekrimaha force-pushed the improvement/ARSN-524 branch from 5ca7b1d to e862723 Compare January 13, 2026 14:56

benzekrimaha force-pushed the improvement/ARSN-524 branch from cf26f38 to dbde6ff Compare January 13, 2026 14:58

benzekrimaha marked this pull request as ready for review January 13, 2026 14:58

francoisferrand reviewed Jan 13, 2026

View reviewed changes

benzekrimaha requested review from SylvainSenechal and francoisferrand January 14, 2026 16:15

SylvainSenechal reviewed Jan 15, 2026

View reviewed changes

SylvainSenechal approved these changes Jan 15, 2026

View reviewed changes

francoisferrand approved these changes Jan 15, 2026

View reviewed changes

benzekrimaha added 3 commits January 16, 2026 15:13

removing unused methods in gcpService and updating tests to use send …

590aac4

…command Issue: ARSN-524

gcpApis tests introduction

9c751cf

Issue: ARSN-524

Remove gcpApis related tests and update existing GcpService tests

aed443f

Issue: ARSN-524

benzekrimaha force-pushed the improvement/ARSN-524 branch from df3d2aa to aed443f Compare January 16, 2026 14:15

bert-e merged commit aed443f into development/8.3 Jan 16, 2026
7 checks passed

bert-e deleted the improvement/ARSN-524 branch January 16, 2026 14:22

-Object.assign(GcpService.prototype, gcpApis);
+GcpClient.prototype.abortMultipartUpload = gcpApis.abortMultipartUpload;
+GcpClient.prototype.completeMultipartUpload = gcpApis.completeMultipartUpload;
+GcpClient.prototype.createMultipartUpload = gcpApis.createMultipartUpload;
+...
+...
+...

adding tests for externalclients and gcpservice #2579

adding tests for externalclients and gcpservice #2579

Conversation

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

Uh oh!

Uh oh!

DarkIsDude Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

DarkIsDude Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

bert-e commented Jan 7, 2026

Hello benzekrimaha,

Uh oh!

bert-e commented Jan 12, 2026

Waiting for approval

Uh oh!

Uh oh!

francoisferrand Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

francoisferrand Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SylvainSenechal Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal Jan 15, 2026 • 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!

francoisferrand left a comment

Choose a reason for hiding this comment

Uh oh!

benzekrimaha commented Jan 16, 2026

Uh oh!

bert-e commented Jan 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

benzekrimaha commented Dec 5, 2025 •

edited

Loading

codecov bot commented Dec 9, 2025 •

edited

Loading

SylvainSenechal Jan 15, 2026 •

edited

Loading