-
Notifications
You must be signed in to change notification settings - Fork 249
Improvement/cldsrv 724 kms tests migration #5951
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-sur-utapi-tests
Are you sure you want to change the base?
Improvement/cldsrv 724 kms tests migration #5951
Conversation
❌ 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 |
d6238a4
to
0bd90df
Compare
// ensure versioned bucket is empty | ||
await helpers.bucketUtil.empty(bkt.vname); | ||
let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }).promise(); | ||
let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || []; |
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.
Versions
can then be undefined ?
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 probably be like this to ensure we do not have an error trying to access Versions
from an array that has no fields
let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || []; | |
let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || { Versions: [] }; |
console.log('Run cleanup', | ||
{ profile: helpers.credsProfile, accessKeyId: helpers.s3.config.credentials.accessKeyId }); | ||
const allBuckets = (await helpers.s3.listBuckets().promise()).Buckets.map(b => b.Name); | ||
const allBuckets = ((await helpers.s3.listBuckets()).Buckets || []).map(b => b.Name); |
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.
We have same stuff for other tests (for example in beforeMigration). Seems like a debug ? Should we keep it ? Check the length ?
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.
what do you mean by the same stuff , are you referring to the console logs ? as they have been there before they're being kept
} | ||
|
||
async function getBucketSSEError(Bucket) { | ||
await assert.rejects(helpers.s3.getBucketEncryption({ Bucket }).promise(), 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.
Why we don't keep the reject assertion ?
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.
the try-catch approach provides clearer information in this case thus the replacement
// check MPU headers against the MPU overview MD | ||
// because there is no migration for ongoing MPU | ||
function assertMPUSSEHeaders(actual, expected, algo) { | ||
// eslint-disable-next-line no-console |
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.
To be deleted 🙏
|
||
// before has no headers to assert | ||
async function mpuComplete({ UploadId, Bucket, Key }, { existingParts, newParts }, mpuOverviewMDSSE, algo, testCase) { | ||
const extractETag = part => { |
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.
As we are in a test, we should prefer assert
instead of throw ? Also return part.CopyPartResult?.ETag || part.ETag || undefined
is better with a check that the result is not undefined ?
const s3config = getConfig(credsProfile, { signatureVersion: 'v4' }); | ||
const s3 = new S3(s3config); | ||
|
||
// Create custom agents with specific pooling settings |
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 custom agents with specific pooling settings |
if you want to make it clear, you can change the name of the var httpAgent
with httpAgentWithSpecificPooling
if this is important when we read tests
|
||
const bucketUtil = new BucketUtility(credsProfile); | ||
|
||
// Wrapper for SDK v3 commands to return promises directly |
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.
// Wrapper for SDK v3 commands to return promises directly |
Your code is clear enough
|
||
const s3Client = new S3Client(s3config); | ||
|
||
// Remove logger middleware if present |
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.
// Remove logger middleware if present |
.Rules[0] | ||
.ApplyServerSideEncryptionByDefault; | ||
try { | ||
const sse = await s3Client.send(new GetBucketEncryptionCommand({ 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.
Should you use your wrapper instead of s3Client
?
action: 'copyObject', detail: ' when getting from source', kmsAction: 'Decrypt', | ||
fct: async () => | ||
helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }).promise(), | ||
helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }) , |
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.
helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }) , | |
helpers.s3.copyObject({ Bucket, Key: 'copy', CopySource: `${Bucket}/${Key}` }), |
You have some other iterations of that in your file
// ensure versioned bucket is empty | ||
await helpers.bucketUtil.empty(bkt.vname); | ||
let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }).promise(); | ||
let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || []; |
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 probably be like this to ensure we do not have an error trying to access Versions
from an array that has no fields
let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || []; | |
let { Versions } = await helpers.s3.listObjectVersions({ Bucket: bkt.vname }) || { Versions: [] }; |
httpAgent, | ||
httpsAgent, | ||
}), | ||
maxAttempts: 8, |
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.
Usually we can keep 3, this should be enough (and one could argue it should be 1 for a test suite, as if we hide transient errors, we might miss real bugs; and in such case we could just remove the retry logic)
Issue: CLDSRV-724