-
Notifications
You must be signed in to change notification settings - Fork 253
Improvement/cldsrv 724 versionning related functional tests #5978
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-bucket-related-functional-tests
Are you sure you want to change the base?
Conversation
❌ 66 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
f5f1929 to
84f6930
Compare
92f36a8 to
c1fb5b5
Compare
2ce7329 to
9bdb532
Compare
ghost
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.
just one question around listing and special characters, otherwise lgtm
tests/functional/aws-node-sdk/test/versioning/listObjectMasterVersions.js
Outdated
Show resolved
Hide resolved
tests/functional/aws-node-sdk/test/versioning/listObjectMasterVersions.js
Outdated
Show resolved
Hide resolved
tests/functional/aws-node-sdk/test/versioning/listObjectMasterVersions.js
Outdated
Show resolved
Hide resolved
tests/functional/aws-node-sdk/test/versioning/listObjectVersions.js
Outdated
Show resolved
Hide resolved
tests/functional/aws-node-sdk/test/versioning/listObjectVersions.js
Outdated
Show resolved
Hide resolved
tests/functional/aws-node-sdk/test/versioning/listObjectVersions.js
Outdated
Show resolved
Hide resolved
tests/functional/aws-node-sdk/test/versioning/listObjectVersions.js
Outdated
Show resolved
Hide resolved
| { name: 'Pâtisserie=中文-español-English', value: 'foo' }, | ||
| { name: 'Pâtisserie=中文-español-English', value: 'bar' }, | ||
| { name: 'notes/spring/1.txt', value: 'qux' }, |
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.
are we stopping supporting object names like these? There is an advantage of keeping the special characters, as we do listing, it's to ensure it works well even when the object names are uncommon
| afterEach(async () => { | ||
| await removeAllVersions({ Bucket: bucketName }); | ||
| await s3.send(new DeleteBucketCommand({ Bucket: bucketName })); | ||
| // Create objects in batches of 20 concurrently |
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 objects in batches of 20 concurrently |
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.
LGTM. Just some nits but good 👏
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.
test
As test files depend on these utilities, they needed to be adapted to match the sdk v3 requirements. Issue: CLDSRV-724
Issue: CLDSRV-724
0f033a6 to
3e2bdfc
Compare
911d5e2 to
4b11b09
Compare
tests/functional/aws-node-sdk/test/versioning/listObjectMasterVersions.js
Outdated
Show resolved
Hide resolved
| test.nextVersionIdMarker.versionId); | ||
| return done(); | ||
| }); | ||
| it('should handle pagination with MaxKeys', async () => { |
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.
Kinda agree with William's comment, I feel like some of the tests got removed, and we are missing coverage for some corner case / special characters ?
| assert.strictEqual(err.statusCode, 400); | ||
| done(); | ||
| }); | ||
| afterEach(done => { |
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.
arent we doing removeAllVersions twice here ? We have after() and afterEach(), we used to have only afterEach() 🤔
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.
doesn't hurt to keep it as an additionnal security level
| assert.strictEqual(putResult.$metadata.httpStatusCode, 200); | ||
| }); | ||
|
|
||
| describe('on a version-enabled 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.
It's a bit hard to follow the diff, the test names were changes, the old ones had a putBucketVersioning command, but I can't find it with the new tests
| VersionId: putResults[i].VersionId, | ||
| })); | ||
|
|
||
| it('should create a bunch of objects and their versions', done => { |
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 test was written twice in the old code, right ?
SylvainSenechal
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.
Can you address the comments : there are 2 files where the tests looks quite different from the original one (one file where it seems some tests are missing, another one where the tests are renamed/looks differents). It might not be a problem, but I wonder why it was changed this way
79eb2ad to
84669aa
Compare
Issue: CLDSRV-724