Improvement/cldsrv 724 sur utapi tests #5950
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 |
benzekrimaha
force-pushed
the
improvement/CLDSRV-724-sur-utapi-tests
branch
from
548afab to
4d4e5f0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (79.61%) is below the adjusted base coverage (82.72%). You can increase the head coverage or adjust the Removed Code Behavior. Additional details and impacted files
@@ Coverage Diff @@
## development/9.1 #5950 +/- ##
===================================================
- Coverage 83.72% 79.61% -4.12%
===================================================
Files 191 191
Lines 12233 12233
===================================================
- Hits 10242 9739 -503
- Misses 1991 2494 +503
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
benzekrimaha
force-pushed
the
improvement/CLDSRV-724-sur-utapi-tests
branch
4 times, most recently
from
fdd6e44 to
9844ffb
Compare
tests/sur/quota.js
Outdated
| next => { | ||
| createBucket(bucket, false, next); | ||
| }, |
There was a problem hiding this comment.
| next => { | |
| createBucket(bucket, false, next); | |
| }, | |
| next => createBucket(bucket, false, next); |
| assert.ifError(err); | ||
| return next(); |
There was a problem hiding this comment.
not related to your changes but a quick win could be to just call the next with the error, because an assert in a cb makes it harder to debug, and can even hide the error (as we crash)
tests/sur/quota.js
Outdated
| // Log what headers we're sending | ||
| // eslint-disable-next-line no-console | ||
| console.log('Request headers after stripping:', Object.keys(args.request.headers)); |
There was a problem hiding this comment.
We can probably remove this console log
tests/sur/quota.js
Outdated
| }); | ||
|
|
||
| // Add middleware to strip ALL checksum headers | ||
| s3Client.middlewareStack.add( |
There was a problem hiding this comment.
Can we maybe add a comment to explain why we need this? And maybe a ticket number if that could be removed, so we just support the standard headers
tests/sur/quota.js
Outdated
| .catch(err => { | ||
| callback(err); | ||
| }); |
There was a problem hiding this comment.
| .catch(err => { | |
| callback(err); | |
| }); | |
| .catch(err => callback(err)); |
tests/sur/quota.js
Outdated
| if (!err && !s3Config.isQuotaInflightEnabled()) { | ||
| mockScuba.incrementBytesForBucket(bucket, parts * partSize); | ||
| if (!s3Config.isQuotaInflightEnabled()) { | ||
| mockScuba.incrementBytesForBucket(bucket, -(parts * partSize)); |
There was a problem hiding this comment.
i'm not sure to get why we increment the bytes before the call then we decrement them, is there any issue with the test?
tests/sur/quota.js
Outdated
| .catch(err => { | ||
| cb(err); | ||
| }); |
There was a problem hiding this comment.
| .catch(err => { | |
| cb(err); | |
| }); | |
| .catch(err => cb(err)); |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
| if (unauthenticated) { | ||
| this.s3 = new S3Client({ | ||
| ...s3Config, | ||
| credentials: { accessKeyId: '', secretAccessKey: '' }, | ||
| forcePathStyle: true, | ||
| signer: { sign: async request => request }, | ||
| }); | ||
| } | ||
| else { | ||
| this.s3 = new S3Client({ | ||
| ...s3Config, | ||
| maxAttempts: 0, | ||
| }); | ||
| } |
There was a problem hiding this comment.
| if (unauthenticated) { | |
| this.s3 = new S3Client({ | |
| ...s3Config, | |
| credentials: { accessKeyId: '', secretAccessKey: '' }, | |
| forcePathStyle: true, | |
| signer: { sign: async request => request }, | |
| }); | |
| } | |
| else { | |
| this.s3 = new S3Client({ | |
| ...s3Config, | |
| maxAttempts: 0, | |
| }); | |
| } | |
| if (unauthenticated) { | |
| s3Config = { | |
| ...s3Config, | |
| credentials: { accessKeyId: '', secretAccessKey: '' }, | |
| forcePathStyle: true, | |
| signer: { sign: async request => request }, | |
| }; | |
| } | |
| this.s3 = new S3Client({ | |
| ...s3Config, | |
| maxAttempts: 0, | |
| }); |
Pretty sure that maxAttempts: 0, is working also for unauthenticated client ?
| return this.s3.send(new DeleteBucketCommand({ Bucket: bucketName })) | ||
| .catch(err => { | ||
| throw err; | ||
| }); |
There was a problem hiding this comment.
| return this.s3.send(new DeleteBucketCommand({ Bucket: bucketName })) | |
| .catch(err => { | |
| throw err; | |
| }); | |
| return await this.s3.send(new DeleteBucketCommand({ Bucket: bucketName })); |
One good practice when working with async/await is to always await with return, this will help to preserve the full stack trace (without the await, the stack trace will start at the caller, if you put the await here, the stack trace will display deleteOne
| ) | ||
| .concat((data.DeleteMarkers || []) | ||
| .map(object => | ||
| this.s3.send(new DeleteObjectCommand({ |
| emptyMany(bucketNames) { | ||
| const promises = bucketNames.map( | ||
| bucketName => this.empty(bucketName) | ||
| const promises = bucketNames.map( |
| return Promise.all(promises); | ||
| } | ||
|
|
||
| emptyIfExists(bucketName) { |
There was a problem hiding this comment.
I think our guidelines ask us to add a new line between function definition ?
There was a problem hiding this comment.
good catch missed it here
| request.on('build', () => { | ||
| request.httpRequest.headers['x-scal-s3-version-id'] = vID; | ||
| }); | ||
| // Add custom header using middleware |
There was a problem hiding this comment.
| // Add custom header using middleware |
tests/sur/quota.js
Outdated
| } | ||
| cb(null, data); | ||
| }) | ||
| .catch(err => cb(err)); |
There was a problem hiding this comment.
| .catch(err => cb(err)); | |
| .catch(cb); |
tests/sur/quota.js
Outdated
| .catch(err => { | ||
| cb(err); | ||
| }); |
| .catch(err => { | ||
|
|
||
| if (!s3Config.isQuotaInflightEnabled()) { | ||
| mockScuba.incrementBytesForBucket(bucket, size); | ||
| } | ||
| return callback(err); | ||
| }); |
There was a problem hiding this comment.
| .catch(err => { | |
| if (!s3Config.isQuotaInflightEnabled()) { | |
| mockScuba.incrementBytesForBucket(bucket, size); | |
| } | |
| return callback(err); | |
| }); | |
| .catch(err => { | |
| if (!s3Config.isQuotaInflightEnabled()) { | |
| mockScuba.incrementBytesForBucket(bucket, size); | |
| } | |
| return callback(err); | |
| }); |
nits
tests/sur/quota.js
Outdated
| }), | ||
| next => { | ||
| putObject(bucket, key, size, err => { | ||
| assert.strictEqual(err.Code, 'QuotaExceeded'); |
There was a problem hiding this comment.
Same here, I'm not sure the callback will be called ?
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-724-sur-utapi-tests
branch
3 times, most recently
from
3aa5ada to
61f8a03
Compare
As test files depend on these utilities, they needed to be adapted to match the sdk v3 requirements. Issue: CLDSRV-724
benzekrimaha
force-pushed
the
improvement/CLDSRV-724-sur-utapi-tests
branch
2 times, most recently
from
5ff3711 to
1f24c59
Compare
benzekrimaha
force-pushed
the
improvement/CLDSRV-724-sur-utapi-tests
branch
from
1f24c59 to
adb7ac5
Compare
4 participants
Issue: CLDSRV-724