Update functional tests to use cloudserverclient extended commands by maeldonn · Pull Request #6103 · scality/cloudserver

maeldonn · 2026-03-12T10:57:46Z

bert-e · 2026-03-12T10:57:49Z

Hello maeldonn,

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-03-12T10:57:54Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-868 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.3.6
9.4.0

Please check the Fix Version/s of CLDSRV-868, or the target
branch of this pull request.

maeldonn · 2026-03-12T10:58:54Z

package.json

  },
  "devDependencies": {
    "@eslint/compat": "^1.2.2",
+    "@scality/cloudserverclient": "github:scality/cloudserverclient#improvement/CLDSRVCLT-10/user-metadata",


Suggested change

"@scality/cloudserverclient": "github:scality/cloudserverclient#improvement/CLDSRVCLT-10/user-metadata",

"@scality/cloudserverclient": "^1.0.5",

package.json

claude · 2026-03-12T10:59:20Z

cloudserverclient is pinned to a branch instead of a tag. Git-based deps should pin to a tag for reproducible builds.
- Pin to a release tag once available, or pin to the specific commit hash.

Review by Claude Code

codecov · 2026-03-12T11:00:40Z

❌ 8 Tests Failed:

Tests completed	Failed	Passed	Skipped
7946	8	7938	0

View the top 3 failed test(s) by shortest run time

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

Stack Traces | 0.067s run time

Entity expansion limit exceeded: 1002 > 1000
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.

"after each" hook for "should send back VersionId and DeleteMarkerVersionId both equal to deleteVersionId"::Multi-Object Versioning Delete - deleting delete marker With v4 signature "after each" hook for "should send back VersionId and DeleteMarkerVersionId both equal to deleteVersionId"

Stack Traces | 0.072s run time

Entity expansion limit exceeded: 1002 > 1000
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.

"after each" hook for "should send back VersionId and DeleteMarkerVersionId both equal to deleteVersionId"::Multi-Object Versioning Delete - deleting delete marker With default signature "after each" hook for "should send back VersionId and DeleteMarkerVersionId both equal to deleteVersionId"

Stack Traces | 0.079s run time

Entity expansion limit exceeded: 1002 > 1000
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.

"after each" hook for "should batch delete 1000 objects quietly"::Multi-Object Versioning Delete Success With v4 signature "after each" hook for "should batch delete 1000 objects quietly"

Stack Traces | 0.09s run time

Entity expansion limit exceeded: 1002 > 1000
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.

"after each" hook for "should not crash when deleting a null versionId that does not exist"::Multi-Object Versioning Delete Success With default signature "after each" hook for "should not crash when deleting a null versionId that does not exist"

Stack Traces | 0.094s run time

Entity expansion limit exceeded: 1002 > 1000
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.

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

"after each" hook for "should create a bunch of objects and their versions"::put and get 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.064s run time

Entity expansion limit exceeded: 1002 > 1000
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.

"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 41 times)

Stack Traces | 0.072s run time

Entity expansion limit exceeded: 1002 > 1000
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.

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

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

Stack Traces | 0.066s run time

Entity expansion limit exceeded: 1002 > 1000
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.

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

DarkIsDude

Your CI is fully broken

claude · 2026-03-13T13:44:24Z

LGTM

Clean refactor replacing manual middleware hacks (custom request handlers, XML parsing, header injection) with the @scality/cloudserverclient extended commands. The new dependency is properly pinned to an exact version (1.0.5) in devDependencies, the @aws-sdk/client-s3 bump is consistent with the transitive dependency from cloudserverclient, and the new test case for non-existing metadata keys is a good addition.

Review by Claude Code

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

claude · 2026-03-13T16:02:53Z

head.js:24 — err.name === "Unknown" is a weak assertion that will match any unrecognized error, not specifically a missing-bucket-name error. Consider also asserting on the HTTP status code or another specific property.

Review by Claude Code

DarkIsDude

LGTM, congrats

claude · 2026-03-13T16:39:29Z

LGTM

Clean refactor replacing manual middleware/XML-parsing workarounds with the @scality/cloudserverclient extended commands. The .toDate() fixes for moment objects and the updated empty-bucket-name error assertions align with the AWS SDK v3 upgrade. New test case for non-existing-only metadata keys is a nice addition.

Review by Claude Code

claude · 2026-03-13T19:04:23Z

LGTM

Clean refactor replacing manual middleware/XML-parsing workarounds with proper @scality/cloudserverclient extended commands (ListObjectsV2ExtendedCommand, GetObjectAttributesExtendedCommand). The .toDate() additions correctly fix moment-to-Date compatibility with AWS SDK v3, and the test adjustments for empty bucket name validation reflect the updated SDK behavior (server-side 405 instead of client-side validation error). New test coverage for non-existing metadata keys is a good addition.

Review by Claude Code

claude · 2026-03-16T14:21:31Z

package.json

  "dependencies": {
    "@aws-sdk/client-iam": "^3.930.0",
-    "@aws-sdk/client-s3": "^3.908.0",
+    "@aws-sdk/client-s3": "^3.931.0",


The @scality/cloudserverclient dependency pulls in @aws-sdk/client-s3@^3.1009.0 as a transitive dependency. Since package.json only bumps the direct @aws-sdk/client-s3 to ^3.931.0, yarn resolves a single 3.1009.0 instance for both. This means the production @aws-sdk/client-s3 is now forced to 3.1009.0 — a much larger jump than the ^3.931.0 in package.json suggests. Consider aligning the direct dependency version to ^3.1009.0 to make this explicit, or pinning @scality/cloudserverclient peer dependency range to avoid silently pulling the production SDK forward.

— Claude Code

claude · 2026-03-16T14:21:39Z

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

@@ -224,18 +224,6 @@ describe('GET object', () => {
                await s3.send(new DeleteBucketCommand({ Bucket: bucketName }));
        });



The test for empty bucket name was removed, but the equivalent test in head.js was kept and changed to assert a 405 HTTP status. Was this removal intentional? If the newer SDK now sends the request to the server (instead of rejecting client-side), this test should be updated like head.js rather than deleted, to retain coverage of the server's behavior on empty bucket names.

— Claude Code

claude · 2026-03-16T14:22:26Z

The @scality/cloudserverclient devDep transitively requires @aws-sdk/client-s3 ^3.1009.0, forcing production SDK from ^3.931.0 to 3.1009.0. The declared version in package.json is misleading. Align to ^3.1009.0 or isolate the transitive pull.
The empty-bucket-name test in get.js was removed, but the analogous test in head.js was updated to assert 405 status. Update get.js test similarly instead of deleting it to preserve coverage.

Review by Claude Code

Issue: CLDSRV-868

claude · 2026-03-18T15:22:58Z

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

-                });
-            });
-
        it('should return NoSuchKey error when no such object',


The test for GET object with empty bucket name was removed entirely. Unlike the HEAD bucket test (head.js) which was updated to check the new server-side error behavior, this test was just deleted. If the newer SDK now sends the request to the server instead of rejecting client-side, consider adding a replacement test that asserts the expected server response (as was done for HEAD).

— Claude Code

claude · 2026-03-18T15:23:07Z

package.json

  },
  "devDependencies": {
    "@eslint/compat": "^1.2.2",
+    "@scality/cloudserverclient": "1.0.7",


Adding @scality/cloudserverclient (which depends on @aws-sdk/client-s3@^3.1009.0) as a devDependency causes yarn to deduplicate and resolve the production @aws-sdk/client-s3 from 3.917.0 to 3.1009.0. This is a much larger jump than the ^3.908.0 to ^3.931.0 bump in package.json suggests. Ensure this is intentional and that the 3.1009.0 SDK has been validated against the production server code (not just functional tests).

— Claude Code

claude · 2026-03-18T15:24:48Z

Unintended production dependency bump: Adding @scality/cloudserverclient (requires @aws-sdk/client-s3 ^3.1009.0) causes yarn deduplication to bump the production @aws-sdk/client-s3 from 3.917.0 to 3.1009.0, far beyond the ^3.931.0 range declared in package.json. Validate this jump against production server code. Consider whether the package.json range should be updated to ^3.1009.0 to make the actual resolved version explicit.

Removed test without replacement (tests/functional/aws-node-sdk/test/object/get.js): The GET object empty-bucket-name test was deleted rather than updated. The analogous HEAD test was updated to assert the new server-side error. Consider adding a similar replacement for GET.

Review by Claude Code

claude · 2026-03-18T16:04:49Z

package.json

    "jsonwebtoken": "^9.0.0",
-    "nan": "v2.22.0"
+    "nan": "v2.22.0",
+    "fast-xml-parser": ">=5.5.6"


The resolution >=5.5.6 has no upper bound — a future major version (e.g. 6.x) could introduce breaking changes. Pin it more tightly.

```suggestion
"fast-xml-parser": "^5.5.6"

claude · 2026-03-18T16:05:01Z

fast-xml-parser resolution uses >=5.5.6 which has no upper bound and could pull in breaking major versions. Use ^5.5.6 instead to cap at the current major.

Review by Claude Code

claude · 2026-03-18T17:18:17Z

package.json

    "jsonwebtoken": "^9.0.0",
-    "nan": "v2.22.0"
+    "nan": "v2.22.0",
+    "fast-xml-parser": ">=5.5.6"


The >=5.5.6 resolution is overly permissive — it allows any future major version of fast-xml-parser, which could introduce breaking changes. Consider pinning to a safe range.

suggestion<br> "fast-xml-parser": "^5.5.6"<br>

— Claude Code

claude · 2026-03-18T17:18:43Z

fast-xml-parser resolution uses >=5.5.6 which is unbounded and could pull in future breaking major versions. Use ^5.5.6 instead.

Review by Claude Code

claude · 2026-03-18T18:40:19Z

package.json

    "jsonwebtoken": "^9.0.0",
-    "nan": "v2.22.0"
+    "nan": "v2.22.0",
+    "fast-xml-parser": "5.3.8"


The fast-xml-parser resolution is pinned to an exact version (5.3.8) without a range operator. This means it will not receive patch-level security fixes automatically. Consider using ^5.3.8 or >=5.3.8 unless there is a specific reason to lock this exact version (e.g., a known regression in newer versions). If intentional, a comment in the PR body explaining why would help future maintainers.

— Claude Code

claude · 2026-03-18T18:40:20Z

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

                } catch (err) {
-                    assert.strictEqual(err.message, 'Empty value provided for input HTTP label: Bucket.');
+                    assert.strictEqual(err.$metadata.httpStatusCode, 405);
+                    assert.strictEqual(err.name, 'Unknown');


The assertion on err.name === 'Unknown' is fragile. If the server returns a proper error code in the future, this test will pass silently even if behavior changes. Consider adding a comment explaining why 'Unknown' is the expected error name here (likely because HEAD responses have no body for error details).

— Claude Code

claude · 2026-03-18T18:41:12Z

package.json:88 — fast-xml-parser resolution pinned to exact version 5.3.8. Will not receive automatic patch-level security fixes. Consider using ^5.3.8 unless there is a specific reason to lock this version.
tests/functional/aws-node-sdk/test/bucket/head.js:25 — Asserting err.name is Unknown is fragile for HEAD responses. Add a comment explaining why Unknown is expected.

Review by Claude Code

Issue: CLDSRV-868

claude · 2026-03-18T19:31:36Z

package.json

    "jsonwebtoken": "^9.0.0",
-    "nan": "v2.22.0"
+    "nan": "v2.22.0",
+    "fast-xml-parser": ">=5.5.6"


The fast-xml-parser resolution uses >=5.5.6 which is an open-ended range. This could pull in future major versions with breaking changes. Consider pinning to a tighter range like ^5.5.6.

— Claude Code

claude · 2026-03-18T19:31:46Z

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

                } catch (err) {
-                    assert.strictEqual(err.message, 'Empty value provided for input HTTP label: Bucket.');
+                    assert.strictEqual(err.$metadata.httpStatusCode, 405);
+                    assert.strictEqual(err.name, 'Unknown');


This test previously validated SDK client-side input checking (err.message). The newer SDK no longer validates empty bucket names client-side and sends the request to the server, returning 405. Asserting on err.name === 'Unknown' is fragile — if CloudServer starts returning a named error for this case, the test breaks silently. Consider also asserting on a more descriptive property if available.

— Claude Code

claude · 2026-03-18T19:32:20Z

The fast-xml-parser resolution uses >=5.5.6 (open-ended range), which could pull in future breaking major versions. Use ^5.5.6 instead.
The HEAD bucket empty-name test asserts err.name === Unknown, which is fragile.
The @aws-sdk/client-s3 bump from ^3.908.0 to ^3.1012.0 is a large jump -- validate no regressions in middleware APIs used in production code.
Test removals for SDK client-side validation are correct since newer SDK no longer performs these checks.

Overall the refactoring to @scality/cloudserverclient extended commands is a clean improvement.

Review by Claude Code

maeldonn requested review from a team, DarkIsDude and benzekrimaha March 12, 2026 10:57

maeldonn commented Mar 12, 2026

View reviewed changes

claude bot reviewed Mar 12, 2026

View reviewed changes

package.json Outdated Show resolved Hide resolved

DarkIsDude requested changes Mar 12, 2026

View reviewed changes

maeldonn force-pushed the improvement/CLDSRV-868/user-metadata branch from 5eeb4ea to 6710658 Compare March 13, 2026 13:43

maeldonn requested review from DarkIsDude and SylvainSenechal March 13, 2026 13:43

francoisferrand approved these changes Mar 13, 2026

View reviewed changes

maeldonn force-pushed the improvement/CLDSRV-868/user-metadata branch from 6710658 to f3cd77a Compare March 13, 2026 16:00

claude bot reviewed Mar 13, 2026

View reviewed changes

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

DarkIsDude approved these changes Mar 13, 2026

View reviewed changes

maeldonn force-pushed the improvement/CLDSRV-868/user-metadata branch from f3cd77a to 651272d Compare March 13, 2026 16:38

maeldonn force-pushed the improvement/CLDSRV-868/user-metadata branch from 651272d to 59d9e0a Compare March 13, 2026 19:02

maeldonn force-pushed the improvement/CLDSRV-868/user-metadata branch from 59d9e0a to b317ddc Compare March 16, 2026 14:20

claude bot reviewed Mar 16, 2026

View reviewed changes

Add @scality/cloudserverclient dev dependency

f659ecb

Issue: CLDSRV-868

maeldonn force-pushed the improvement/CLDSRV-868/user-metadata branch from b317ddc to 4b3ce9a Compare March 18, 2026 15:20

claude bot reviewed Mar 18, 2026

View reviewed changes

maeldonn force-pushed the improvement/CLDSRV-868/user-metadata branch from 39610f4 to a5b3c62 Compare March 18, 2026 18:38

claude bot reviewed Mar 18, 2026

View reviewed changes

maeldonn added 4 commits March 18, 2026 20:29

Resolve fast-xml-parser CVE-2026-26278 via yarn resolution

933be16

Issue: CLDSRV-868

Refactor GetObjectAttributes functional tests to use extended command

67abc32

Issue: CLDSRV-868

Refactor ListObjectsV2 functional tests to use extended command

bd0b82e

Issue: CLDSRV-868

Add GetObjectAttributes functional test for non-existent user metadata

58a5074

Issue: CLDSRV-868

maeldonn force-pushed the improvement/CLDSRV-868/user-metadata branch from a5b3c62 to 58a5074 Compare March 18, 2026 19:30

claude bot reviewed Mar 18, 2026

View reviewed changes

	"@scality/cloudserverclient": "github:scality/cloudserverclient#improvement/CLDSRVCLT-10/user-metadata",
	"@scality/cloudserverclient": "^1.0.5",

		@@ -224,18 +224,6 @@ describe('GET object', () => {
		await s3.send(new DeleteBucketCommand({ Bucket: bucketName }));
		});

Conversation

maeldonn commented Mar 12, 2026

Uh oh!

bert-e commented Mar 12, 2026

Hello maeldonn,

Uh oh!

bert-e commented Mar 12, 2026

Incorrect fix version

Uh oh!

maeldonn Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude bot commented Mar 12, 2026

Uh oh!

codecov bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 8 Tests Failed:

Uh oh!

DarkIsDude left a comment

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 13, 2026

Uh oh!

Uh oh!

claude bot commented Mar 13, 2026

Uh oh!

DarkIsDude left a comment

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 13, 2026

Uh oh!

claude bot commented Mar 13, 2026

Uh oh!

claude bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 16, 2026

Uh oh!

claude bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 18, 2026

Uh oh!

claude bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 18, 2026

Uh oh!

claude bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 18, 2026

Uh oh!

claude bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 18, 2026

Uh oh!

claude bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov bot commented Mar 12, 2026 •

edited

Loading