-
Notifications
You must be signed in to change notification settings - Fork 252
Improvement/cldsrv 724 multiple backend tests #5960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: improvement/CLDSRV-724-kms-tests-migration
Are you sure you want to change the base?
Improvement/cldsrv 724 multiple backend tests #5960
Conversation
1769d66 to
4619bab
Compare
❌ 366 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 50 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
76ab4cc to
fc9608c
Compare
williamlardier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some quick comments as the pr was listed for review (but still a draft)
otherwise lgtm, some patterns could be simplified, when calling the next functions (by removing the {} or the =>)
tests/functional/aws-node-sdk/test/multipleBackend/delete/delete.js
Outdated
Show resolved
Hide resolved
e031412 to
1c923f3
Compare
a8406fa to
55803c6
Compare
307118e to
d6bfd4b
Compare
226f21d to
0cbf3b9
Compare
00390a0 to
327e859
Compare
327e859 to
58f8c81
Compare
| throw err; | ||
| }); | ||
|
|
||
| await s3.send(new CreateBucketCommand({ Bucket: bucket })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need try/catch but not too sure since its a test, I guess failures would also fail the test ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed no need for a try catch here failure will make the test fail
|
|
||
| after(async () => { | ||
| process.stdout.write('Emptying bucket\n'); | ||
| await bucketUtil.empty(bucket); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an addition right ? There was a flaky or something without it I guess ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes , this is just to make sure we're emptying buckets before deleting them
| }, err => next(err)), | ||
| next => s3.getObject({ | ||
| })).then(() => next()) | ||
| .catch(err => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There a lot of places in this pr where these catch/then can be shortened, but at this point its nitpicking and it can be left as it is
| .then(() => { | ||
| assert.fail('Expected error but got success'); | ||
| }).catch(err => { | ||
| assert.strictEqual(err.code, 'NoSuchKey', 'Expected ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this err.code stays here without failing the pipeline, should it be err.name ?
6d97621 to
d8c1d88
Compare
| .then(() => { | ||
| cb(); | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .then(() => { | |
| cb(); | |
| }) | |
| .then(() => cb()) |
| }); | ||
| } | ||
|
|
||
| // Update AWS S3 direct calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Update AWS S3 direct calls |
| await bucketUtil.empty(bucketAws); | ||
| await bucketUtil.empty(awsServerSideEncryptionbucket); | ||
|
|
||
| process.stdout.write(`Deleting bucket ${bucket}\n`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can remove those debug logs, even if they were there before (not useful, this is an after each?)
| assert.strictEqual(res.ServerSideEncryption, 'AES256'); | ||
| } | ||
|
|
||
| process.stdout.write('Getting object from AWS\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| process.stdout.write('Getting object from AWS\n'); |
| }); | ||
| }); | ||
|
|
||
| // Create bucket using AWS SDK v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Create bucket using AWS SDK v3 |
| await s3.send(new PutObjectCommand({ Bucket: '', Key: key })); | ||
| throw new Error('Expected failure but got success'); | ||
| } catch (err) { | ||
| assert.strictEqual(err.code, 'MethodNotAllowed'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be err.name ?
Also line 105 there is a comment about v2.363, the comment is probably outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah the test is skipped, I guess you can change it or leave it, the pr is already good and it will be spotted if the test is used again
| await s3.send(new PutObjectCommand(params)); | ||
| throw new Error('Expected failure but got success'); | ||
| } catch (err) { | ||
| assert.strictEqual(err.code, 'InvalidArgument'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err.name
| done(); | ||
| }); | ||
|
|
||
| await s3.send(new PutObjectCommand(params)).then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird mix of await with then/catch 🤔
Issue: CLDSRV-724