-
Notifications
You must be signed in to change notification settings - Fork 252
Improvement/cldsrv 724 bucket related functional tests #5988
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-object-related-functional-tests
Are you sure you want to change the base?
Improvement/cldsrv 724 bucket related functional tests #5988
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
❌ 140 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 45 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
834eefe to
92f36a8
Compare
c56ac68 to
06547e0
Compare
92f36a8 to
c1fb5b5
Compare
| .then(() => awsRequest(auth, ListObjectsV2Command, { Bucket: testBucket })) | ||
| .then(() => done(new Error('Expected failure'))) | ||
| .catch(cbWithError(done)); | ||
| // Don't return the promise! |
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.
| // Don't return the promise! |
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.
up
| // Use unique bucket name for each test to avoid conflicts | ||
| testBucketName = `${bucketName}-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; |
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.
Could probably be a helper function later shared with all tests?
|
|
||
| } | ||
| }); | ||
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.
| data.Contents.forEach(item => { | ||
| newContent.push(item.Key); | ||
| }); | ||
| /* eslint-disable no-param-reassign */ |
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 "next-line", otherwise it disables for the rest of the file
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.
up
| const [pathBase, queryString] = path.split('?'); | ||
| const query = queryString ? Object.fromEntries(new URLSearchParams(queryString)) : {}; | ||
|
|
||
| // Create HTTP request (mimics AWS.HttpRequest with v2-like endpoint structure) |
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 HTTP request (mimics AWS.HttpRequest with v2-like endpoint structure) | |
| // Create HTTP request |
| // Rename 'authorization' to 'Authorization' | ||
| if (signedRequest.headers.authorization) { | ||
| signedRequest.headers.Authorization = signedRequest.headers.authorization; | ||
| delete signedRequest.headers.authorization; |
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.
Is this needed? in node the headers are always lowercased
06547e0 to
1166011
Compare
071dee4 to
381a38a
Compare
0f033a6 to
3e2bdfc
Compare
DarkIsDude
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.
Congrats again 👏
| // Create unsigned client | ||
| const unsignedClient = new BucketUtility('default', { | ||
| ...sigCfg, | ||
| credentials: { accessKeyId: '', secretAccessKey: '' }, | ||
| forcePathStyle: true, | ||
| signer: { sign: async request => request }, | ||
| }); | ||
|
|
||
| // Replace awsAuthMiddleware with a no-op middleware to skip signing |
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 unsigned client | |
| const unsignedClient = new BucketUtility('default', { | |
| ...sigCfg, | |
| credentials: { accessKeyId: '', secretAccessKey: '' }, | |
| forcePathStyle: true, | |
| signer: { sign: async request => request }, | |
| }); | |
| // Replace awsAuthMiddleware with a no-op middleware to skip signing | |
| const unsignedClient = new BucketUtility('default', { | |
| ...sigCfg, | |
| credentials: { accessKeyId: '', secretAccessKey: '' }, | |
| forcePathStyle: true, | |
| signer: { sign: async request => request }, | |
| }); | |
| .then(() => awsRequest(auth, ListObjectsV2Command, { Bucket: testBucket })) | ||
| .then(() => done(new Error('Expected failure'))) | ||
| .catch(cbWithError(done)); | ||
| // Don't return the promise! |
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.
up
| 'getObject': GetObjectCommand, | ||
| 'putObject': PutObjectCommand, | ||
| }; | ||
| const CommandCtor = commandMap[operation]; |
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.
CommandCtor what does it mean ?
| it('should return NoSuchBucket error if bucket does not exist', async () => { | ||
| try { | ||
| await s3.send(new DeleteBucketLifecycleCommand({ Bucket: bucket })); | ||
| // Should not reach here |
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 not reach here |
| it('should return AccessDenied if user is not bucket owner', async () => { | ||
| try { | ||
| await otherAccountS3.send(new DeleteBucketLifecycleCommand({ Bucket: bucket })); | ||
| // Should not reach here |
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 not reach here |
| }); | ||
| s3.send(new GetBucketLoggingCommand({ Bucket: bucketName })) | ||
| .then(data => { | ||
| // When no logging is configured, AWS returns empty object |
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 not sure to understand the why here
| await s3.send(new DeleteBucketCommand({ Bucket: bucket })); | ||
| } catch (err) { | ||
| // eslint-disable-next-line no-console | ||
| console.log(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.
Is it a bug ?
| data.Contents.forEach(item => { | ||
| newContent.push(item.Key); | ||
| }); | ||
| /* eslint-disable no-param-reassign */ |
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.
up
Issue: CLDSRV-724