Log test results one json object per test #123

krisis · 2017-08-25T00:39:14Z

Based on the following spec,


{
  "name":"mc",                                                     // SDK Name
  "function":"makeBucket(String bucketName)",                      // API name
  "args":"",                                                       // key value map, varName:value. Only arguements that devs may be interested in
  "duration":"15",                                                 // duration of the whole test in milli seconds
  "status":"PASS",                                                 // can be PASS, FAIL, NA
  "alert":"Information like whether this is a Blocker/ Gateway, Server etc can go here",
  "message":" descriptive error message"                           // error related, human readable  message. Should be taken care of if present
  "error":"stack-trace/exception message(only in case of failure)" // actual low level exception/error thrown by the program
}

Fixes #112

nitisht

Tested locally, LGTM with one small comment

nitisht · 2017-08-25T05:22:15Z

ms can be removed from the duration section, unit is implicit.

Current output:

{
  "name": "aws-sdk-php",
  "function": "getBucketLocation ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413"
  },
  "duration": "5ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "listBuckets ( array $params = [] )",
  "args": [],
  "duration": "8ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "listObjects ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413",
    "Object": "obj1"
  },
  "duration": "47ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "listMultipartUploads ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413",
    "Object": "obj1"
  },
  "duration": "159ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "headBucket ( array $params = [] )",
  "args": [
    "aws-sdk-php-bucket-20413",
    "aws-sdk-php-bucket-10432"
  ],
  "duration": "11ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "headObject ( array $params = [] )",
  "args": {
    "aws-sdk-php-bucket-20413": "obj1",
    "aws-sdk-php-bucket-10432": "obj2"
  },
  "duration": "5ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "getObject ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413",
    "Object": "obj1"
  },
  "duration": "11ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "copyObject ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413",
    "Object": "obj1"
  },
  "duration": "13ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "deleteObjects (array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413",
    "Object": "obj1"
  },
  "duration": "23ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "anonDeleteObjects ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413",
    "Object": "obj1"
  },
  "duration": "34ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "createMultipartUpload ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413",
    "Object": "obj1"
  },
  "duration": "259ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "uploadPart ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413",
    "Object": "obj1"
  },
  "duration": "154ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "abortMultipartupload ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-20413",
    "Object": "obj1"
  },
  "duration": "32ms",
  "status": "PASS"
}
{
  "name": "aws-sdk-php",
  "function": "getBucketPolicy ( array $params = [] )",
  "args": {
    "Bucket": "aws-sdk-php-bucket-42310"
  },
  "duration": "57ms",
  "status": "PASS"
}

nitisht · 2017-08-25T05:24:50Z

run/core/aws-sdk-php/quick-tests.php

+            "duration" => sprintf("%dms", ($end_time - $start_time) * 1000), // elapsed time in ms
+            "status" => $status,
+        ];
+        if ($error !== "") {


should this be ($error != "") ?

I preferred !== over != for the following reason.

Example | Name | Result
$a !== $b | Not identical | TRUE if $a is not equal to $b, or they are not of the same type.

Ref: https://secure.php.net/manual/en/language.operators.comparison.php

got it. 👍

ebozduman · 2017-08-25T18:56:59Z

run/core/aws-sdk-php/quick-tests.php

+            "duration" => sprintf("%d", ($end_time - $start_time) * 1000), // elapsed time in ms
+            "status" => $status,
+        ];
+        if ($error !== "") {


I have a single comment and request for a change.
Although it may not be that intuitive, the decision made for the spec dictates...
"error" field is for low level error information like stack trace info etc., and the "message" filed is for error messages.

Spec:

: : : "alert":"Information like whether this is a Blocker/ Gateway, Server etc can go here", "message":" descriptive error message" // error related, human readable message. Should be taken care of if present "error":"stack-trace/exception message(only in case of failure)" // actual low level exception/error thrown by the program

Please replace error for message.

ebozduman · 2017-08-25T19:15:27Z

run/core/aws-sdk-php/quick-tests.php

@@ -143,7 +143,8 @@ function runExceptionalTests($s3Client, $apiCall, $exceptionMatcher, $exceptionP
 function testListBuckets(S3Client $s3Client) {
    $buckets = $s3Client->listBuckets();
    foreach ($buckets['Buckets'] as $bucket){
-        echo $bucket['Name']."\n";
+        // Uncomment the following line during debugging.


I think it would be better to define a debug mode, that can be turned on and off, in the code to eliminate manually commenting out all the debug print out/echo lines one by one when done testing and the opposite when debugging.

https://stackoverflow.com/questions/10397454/php-turn-on-off-able-function-to-print-debugging-messages

Here is one way to achieve it:

$debugmode = true; interface Debugger { public function out($data); } class EchoDebugger implements Debugger { public function out($data) { echo $data; } } class NullDebugger implements Debugger { public function out($data) { // Do nothing } } if($debugmode) $debugger = new EchoDebugger(); else $debugger = new NullDebugger(); $debugger->out('This will be output if $debugmode is true');

ebozduman · 2017-08-25T19:36:31Z

run/core/aws-sdk-php/quick-tests.php

-    runTestFunction('testBucketPolicy', $s3Client, $emptyBucket);
+    $testParams = ['Bucket' => $firstBucket, 'Object' => $firstObject];
+    runTestFunction($s3Client, 'testGetBucketLocation', "getBucketLocation ( array \$params = [] )", ['Bucket' => $firstBucket]);
+    runTestFunction($s3Client, 'testListBuckets', "listBuckets ( array \$params = [] )", []);


PHP function definition supports default parameter values.
So, defining runtestFunction as:
function runTestFunction($s3Client, $myfunc, $fnSignature, $args) {
=>
function runTestFunction($s3Client, $myfunc, $fnSignature, $args = []) {

enables us not to specify a value for args, when there is nothing to provide during the function call.
runTestFunction($s3Client, 'testListBuckets', "listBuckets ( array \$params = [] )", []);
=>
runTestFunction($s3Client, 'testListBuckets', "listBuckets ( array \$params = [] )");

ebozduman · 2017-08-25T19:57:12Z

run/core/aws-sdk-php/quick-tests.php

  * @param args parameters to be passed to test function
  *
  * @return void
  */
-function runTestFunction($myfunc, ...$args) {


Minor comment in naming the function runTestFunction.
No need to have Function in the name.
runTest sounds better and good enough to me.

Based on the following spec, ``` { "name":"mc", // SDK Name "function":"makeBucket(String bucketName)", // API name "args":"", // key value map, varName:value. Only arguements that devs may be interested in "duration":"15", // duration of the whole test in milli seconds "status":"PASS", // can be PASS, FAIL, NA "alert":"Information like whether this is a Blocker/ Gateway, Server etc can go here", "message":" descriptive error message" // error related, human readable message. Should be taken care of if present "error":"stack-trace/exception message(only in case of failure)" // actual low level exception/error thrown by the program } ```

ebozduman

LGTM

nitisht requested review from nitisht and ebozduman August 25, 2017 01:16

nitisht added this to the Current milestone Aug 25, 2017

nitisht reviewed Aug 25, 2017

View reviewed changes

krisis force-pushed the issue/112 branch from 3b6be8e to 01c0a72 Compare August 25, 2017 17:37

nitisht approved these changes Aug 25, 2017

View reviewed changes

ebozduman requested changes Aug 25, 2017

View reviewed changes

ebozduman reviewed Aug 25, 2017

View reviewed changes

krisis force-pushed the issue/112 branch from 01c0a72 to e7e8599 Compare August 25, 2017 20:47

ebozduman approved these changes Aug 25, 2017

View reviewed changes

nitisht merged commit 921ffae into minio:master Aug 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log test results one json object per test #123

Log test results one json object per test #123

krisis commented Aug 25, 2017 •

edited

Loading

nitisht left a comment

nitisht commented Aug 25, 2017 •

edited

Loading

nitisht Aug 25, 2017

krisis Aug 25, 2017

nitisht Aug 25, 2017

ebozduman Aug 25, 2017

ebozduman Aug 25, 2017

ebozduman Aug 25, 2017 •

edited

Loading

ebozduman Aug 25, 2017

ebozduman left a comment

Log test results one json object per test #123

Log test results one json object per test #123

Conversation

krisis commented Aug 25, 2017 • edited Loading

nitisht left a comment

Choose a reason for hiding this comment

nitisht commented Aug 25, 2017 • edited Loading

nitisht Aug 25, 2017

Choose a reason for hiding this comment

krisis Aug 25, 2017

Choose a reason for hiding this comment

nitisht Aug 25, 2017

Choose a reason for hiding this comment

ebozduman Aug 25, 2017

Choose a reason for hiding this comment

ebozduman Aug 25, 2017

Choose a reason for hiding this comment

ebozduman Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

ebozduman Aug 25, 2017

Choose a reason for hiding this comment

ebozduman left a comment

Choose a reason for hiding this comment

krisis commented Aug 25, 2017 •

edited

Loading

nitisht commented Aug 25, 2017 •

edited

Loading

ebozduman Aug 25, 2017 •

edited

Loading