S3UTILS-238: regression test for crrExistingObjects with non-ASCII keys#391
S3UTILS-238: regression test for crrExistingObjects with non-ASCII keys#391tcarmet wants to merge 3 commits into
Conversation
Hello tcarmet,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
LGTM |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development/1.17 #391 +/- ##
====================================================
+ Coverage 44.90% 45.02% +0.12%
====================================================
Files 88 88
Lines 6456 6456
Branches 1352 1352
====================================================
+ Hits 2899 2907 +8
+ Misses 3511 3503 -8
Partials 46 46 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4647b55 to
df67850
Compare
|
LGTM |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Lift the CRR setup and teardown helpers out of the listObjectsByReplicationStatus functional test and into tests/utils/S3Setup.js so they can be reused by other functional tests. configureCrr gains an optional `storageClass` option for callers whose flow matches by destination storage class.
…SCII keys End-to-end regression test that drives ReplicationStatusUpdater against the workbench cloudserver with objects whose keys contain Polish diacritics (BŚ-test.txt, ąęóćśźżł-all-diacritics.txt, mixed/żółć/Łódź.dat) and an ASCII control. Asserts no "error updating object" log was emitted and that each source object's ReplicationStatus is updated to PENDING. Guards against future regressions in the path-encoding behavior of the SigV4 signer used by @scality/cloudserverclient to talk to cloudserver's /_/backbeat/metadata route - the failure mode reported in S3C-11235 / RD-1751.
df67850 to
85b77e9
Compare
|
LGTM |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
…-8 sequences Add test cases for CJK/Arabic characters (3-byte UTF-8, U+0800-U+FFFF) and emoji/supplementary scripts (4-byte UTF-8, U+10000-U+10FFFF) alongside the existing Polish diacritics coverage (2-byte UTF-8).
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
LGTM |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
|
/create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
| Statement: [ | ||
| { | ||
| Effect: 'Allow', | ||
| Action: ['s3:GetObjectVersion', 's3:GetObjectVersionAcl', 's3:ReplicateObject'], |
There was a problem hiding this comment.
style nitpick
| Action: ['s3:GetObjectVersion', 's3:GetObjectVersionAcl', 's3:ReplicateObject'], | |
| Action: [ | |
| 's3:GetObjectVersion', | |
| 's3:GetObjectVersionAcl', | |
| 's3:ReplicateObject', | |
| ], |
| }; | ||
| } | ||
|
|
||
| function runUpdater(updater) { |
There was a problem hiding this comment.
I think this can be replaced at the call site by await promisify(updater.run)
| const wrap = level => (msg, data) => { | ||
| captured.push({ level, message: msg, ...(data || {}) }); | ||
| return inner[level](msg, data); | ||
| }; |
There was a problem hiding this comment.
Adding parenthesis would show more clearly what is returned by wrap
| const wrap = level => (msg, data) => { | |
| captured.push({ level, message: msg, ...(data || {}) }); | |
| return inner[level](msg, data); | |
| }; | |
| const wrap = level => ( | |
| (msg, data) => { | |
| captured.push({ level, message: msg, ...(data || {}) }); | |
| return inner[level](msg, data); | |
| } | |
| ); |
| log.info('Test accounts deleted'); | ||
| }); | ||
|
|
||
| async function runAndAssert(testObjects) { |
There was a problem hiding this comment.
Nitpick: the function is called runAndAssert but there is no call to assert, just expect, what about simply naming it runTest
Intent: why does this change exist?
S3C-11235 / RD-1751 reported that
crrExistingObjectsfails withSignatureDoesNotMatch(HTTP 403) for any object whose key contains non-ASCII characters such as Polish diacritics, blocking the customer from replicating ~131k existing objects. The underlying bug was fixed in@scality/cloudserverclient1.0.8, which s3utils picked up in S3UTILS-232, but no automated test exercises the path. This change adds the regression test so any future encoding regression in the dependency stack is caught in CI.References: S3UTILS-238 · S3C-11235 · RD-1751 · CLDSRVCLT-13 · S3UTILS-232