CLDSRV-863: Checksums for PutObject and UploadPart#6112
CLDSRV-863: Checksums for PutObject and UploadPart#6112leif-scality wants to merge 7 commits intodevelopment/9.4from
Conversation
Hello leif-scality,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 |
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
|
LGTM |
| _flush(callback) { | ||
| Promise.resolve(this.algo.digestFromHash(this.hash)) | ||
| .then(digest => { this.digest = digest; }) | ||
| .then(() => callback(), err => { |
There was a problem hiding this comment.
prefer using catch for error handling
| return callback(); | ||
| } else if (!this.streamClosed && this.readingTrailer && this.trailerBuffer.length !== 0) { | ||
| this.log.error('stream ended without trailer "\r\n"'); | ||
| return callback(errors.IncompleteBody.customizeDescription( |
There was a problem hiding this comment.
you could use errorInstances when using customizeDescription to avoid instantiating twice an error
| crc64nvme: 'AAAAAAAAAAA=', // 12 chars | ||
| }; | ||
|
|
||
| it('no headers: returns crc64nvme with isTrailer=false and expected=undefined', () => { |
There was a problem hiding this comment.
prefer starting tests with should
| error: ChecksumError.XAmzMismatch, | ||
| details: { algorithm: 'crc32', calculated: 'a', expected: 'b' }, | ||
| }); | ||
| assert(result.is.BadDigest); |
There was a problem hiding this comment.
I don't like that is helper is tests because if test fails you compare booleans (false != true) instead of names (InternalError != BadDigest), so you don't have much details.
Can we compre on error name with strings ?
3 participants
No description provided.