Improvement/cldsrv 561 add tests for create and store object#6082
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:
|
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 |
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 |
0f0e6e9 to
57227d6
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
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
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
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 |
SylvainSenechal
left a comment
There was a problem hiding this comment.
wouldn't we want (some of) these tests to be in zenko, to avoid relying on mocks ? I guess should be possible, running mongo commands to directly inspect database data
some (if not all) of these tests are already in Zenko, as part of functional / e2e tests : i.e. can we restore an object (in the various cases)? do we get the proper bucket notification? ... But IMHO "too late" (in the dev process) to inspect mongo content in Zenko tests. relying on mocks is good to test corner cases, and ideally we would have integration tests to cross-check the actual backend works as expected: i.e. running some tests against mongo to verify it behaves as expected. How relevant (or not) the integration tests are really depends on the specific case: e.g. if we are already sure mongo (or metadata) write the object MD as we provide them, then good enough to verify we call this metadata layer with the right information.... But we should try to keep these tests as close to the code as possible: i.e. we want to make sure Cloudserver behaves as expected with mongo backend, should be done here (but integration between backbeat and cloudserver would probably be in Zenko) |
|
|
|
|
LGTM |
b6f83bc to
a26bb42
Compare
|
LGTM |
|
/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 |
|
@bert-e create_pull_requests |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
Follow integration pull requests if you would like to be notified of The following options are set: create_pull_requests, create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_pull_requests, create_integration_branches |
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve, create_pull_requests, create_integration_branches |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-561. Goodbye benzekrimaha. |
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