-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add test for re-created bucket and bucket policy #104
Conversation
run/core/aws-sdk-php/quick-tests.php
Outdated
$params = [ | ||
'404' => ['Bucket' => $bucket] | ||
]; | ||
runExceptionalTests($s3Client, 'getBucketPolicy', 'getStatusCode', $params); |
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.
runExceptionalTests()
? what is the gist behind Exceptional
? is it to indicate Exceptions being generated by these 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.
Is the test catching 404? we should catch 403 instead after creating the bucket. To ensure completely that the policy is not in effect.
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 the test catching 404? we should catch 403 instead after creating the bucket. To ensure completely that the policy is not in effect.
@harshavardhana Yes, the test is looking for NoSuchBucketPolicy
. Are you suggesting that I also check that anonymous GET request fails with 403? I can add that too.
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.
runExceptionalTests() ? what is the gist behind Exceptional ? is it to indicate Exceptions being generated by these tests?
@harshavardhana the function has the following comments in code, let me know how we can change it to make it more useful.
/**
* runExceptionalTests executes a collection of tests that will throw
* a known exception.
*
* @param $s3Client AWS\S3\S3Client object
*
* @param $apiCall Name of the S3Client API method to call
*
* @param $exceptionMatcher Name of Aws\S3\Exception\S3Exception
* method to fetch exception details
*
* @param $exceptionParamMap Associative array of exception names to
* API parameters. E.g,
* $apiCall = 'headBucket'
* $exceptionMatcher = 'getStatusCode'
* $exceptionParamMap = [
* // Invalid bucket name
* '400' => ['Bucket' => $bucket['Name'] . '--'],
*
* // Non existent bucket
* '404' => ['Bucket' => $bucket['Name'] . '-non-existent'],
* ];
*
* @return string - HTTP status code. E.g, "400" for Bad Request.
*/
b19ac94
to
29ab814
Compare
// Create an object to test anonymous GET object | ||
$object = 'test-anon'; | ||
if (!file_exists($MINT_DATA_DIR . '/' . FILE_10KB)) | ||
throw new Exception('File not found ' . $MINT_DATA_DIR . '/' . FILE_10KB); |
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.
Shouldn't we programmatically create some data if MINT_DATA_DIR variable is not specified ?
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 guess not for these . These are part of mint . It's applicable for our SDK tests.
// Create an object to test anonymous GET object | ||
$object = 'test-anon'; | ||
if (!file_exists($MINT_DATA_DIR . '/' . FILE_10KB)) | ||
throw new Exception('File not found ' . $MINT_DATA_DIR . '/' . FILE_10KB); |
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.
@krisis, I ran your changes on docker minio/mint:latest image and the tests fail for completeMultipartUpload.
root@0bd36cddde66:/mint# cat log/aws-sdk-php/error.log
PHP Fatal error: Uncaught Exception: Expected completeMultipartUpload to fail with EntityTooSmall in /mint/run/core/aws-sdk-php/quick-tests.php:130
Stack trace:
#0 /mint/run/core/aws-sdk-php/quick-tests.php(508): runExceptionalTests(Object(Aws\S3\S3Client), 'completeMultipa...', 'getAwsErrorCode', Array)
#1 /mint/run/core/aws-sdk-php/quick-tests.php(868): testMultipartUploadFailure(Object(Aws\S3\S3Client), 'aws-sdk-php-buc...', 'obj1')
#2 /mint/run/core/aws-sdk-php/quick-tests.php(926): runTestFunction('testMultipartUp...', Object(Aws\S3\S3Client), 'aws-sdk-php-buc...', 'obj1')
#3 {main}
thrown in /mint/run/core/aws-sdk-php/quick-tests.php on line 130
root@0bd36cddde66:/mint#
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.
@poornas, which version of minio server did you run mint on? I tried it locally with minio commit id ec5293c
and all tests passed. Previously, minio on FS mode didn't validate if parts uploaded where smaller than 5MB in a multipart upload. This was fixed recently - minio/minio#4642.
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.
@krisis, ran this against play server, it is on Version : 2017-06-24T23:30:00Z.
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.
@poornas minio/minio#4642 is fixed in version 2017-07-24T18-27-35Z. This explains why you are seeing the exception.
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.
Tested & LGTM
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. Tested with localhost running latest minio
Regression test for minio/minio#4714