Improvement/cldsrv 561 add tests for create and store object#6082
Improvement/cldsrv 561 add tests for create and store object#6082benzekrimaha wants to merge 4 commits intodevelopment/9.3from
Conversation
Hello benzekrimaha,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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
@@ Coverage Diff @@
## development/9.3 #6082 +/- ##
===================================================
- Coverage 84.31% 84.24% -0.07%
===================================================
Files 206 206
Lines 13251 13251
===================================================
- Hits 11172 11163 -9
- Misses 2079 2088 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
benzekrimaha
force-pushed
the
improvement/CLDSRV-561-add-tests-for-createAndStoreObject
branch
from
c6e0798 to
dddff1b
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
benzekrimaha
force-pushed
the
improvement/CLDSRV-561-add-tests-for-createAndStoreObject
branch
from
dddff1b to
84844ff
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
benzekrimaha
force-pushed
the
improvement/CLDSRV-561-add-tests-for-createAndStoreObject
branch
from
0f0e6e9 to
57227d6
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
Looking at code coverage (https://app.codecov.io/github/scality/cloudserver/pull/6082/blob/lib/api/apiUtils/object/createAndStoreObject.js?dropdown=coverage), we can confirm createAndStoreObject is already reletively well "covered".
There are some gaps indeed, but this PR does not cover them.
However, the missing parts is also sometimes more subtle, and requires checking the actual behavior : like do we set the right arsenal flag (needOplogUpdate)...
So it is not just adding tests:
- need to dig deeper to identify the specific behaviors which must be implemented by this function (like when to require oplog update, the
originOp, etc...) - then either find the tests which already cover these cases (there are some, c.f. coverage) and update them; or indeed add some extra tests to validate these specific behaviors
| assert.deepStrictEqual(bodyText, 'Much different'); | ||
| }); | ||
|
|
||
| it('should replace archived object in non-versioned bucket', async () => { |
There was a problem hiding this comment.
is the behavior of putObject really specific when the object is archived? or is this just to handle the case where there are actually no "data" associated (e.g. locations is empty) associated with the object?
| await fakeMetadataArchivePromise(bucketName, objectName, undefined, { | ||
| archiveInfo: { archiveId: 'archive-1' }, | ||
| restoreRequestedAt: new Date(0).toISOString(), | ||
| restoreRequestedDays: 5, | ||
| }); |
There was a problem hiding this comment.
probably more than one case? should we have similar test cases (or a single table-driven test) for every "stage" of the cold object: transition in progress, archived, restored, expired ?
| })); | ||
|
|
||
| const currentMD = await getMetadataPromise(bucketName, objectName, undefined); | ||
| assert.strictEqual(currentMD.archive, undefined); |
There was a problem hiding this comment.
Isn't there a special flag we need to pass to Arsenal/Metadata in that case, to ensure this operation can be picked'up by Backbeat in the oplog? (not sure, may not be needed in this specific case... or for some version-suspended case?)
| assert.strictEqual(currentMD.archive, undefined); | ||
| }); | ||
|
|
||
| it('should overwrite archived current object in versioned bucket', async () => { |
There was a problem hiding this comment.
in a versioned bucket, there is no "overwrite" : it just creates another revision
|
|
||
| const currentVersionMD = await getMetadataPromise(bucketName, objectName, undefined); | ||
| assert.strictEqual(currentVersionMD.archive, undefined); | ||
| }); |
There was a problem hiding this comment.
I feel we are missing may test cases : e.g.
- bucket can be version suspended
- previous version can be a "null version" (created before versioning was enabled)
- there is PutObject, but also our own extension "PutObject with version id" which is used when restoring an object : which creates many of the code paths in this function. (not sure how much is really tested, and probably should not go in "objectOverwrite" case : but we need to review if such cases are missing)
(but code coverage says we cover all these, so must be tested somewhere already, somehow...)
3 participants
This PR strengthens coverage around createAndStoreObject, the core path used by putObject, putRestoredObject, and putObjectVersion, with emphasis on cold-storage restore and archived-object edge cases.
The goal is to improve confidence in behavior-heavy flows
Issue: CLDSRV-561