Skip to content
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 json output for mint mc test cases #121

Merged
merged 1 commit into from
Sep 1, 2017

Conversation

nitisht
Copy link
Contributor

@nitisht nitisht commented Aug 23, 2017

Fixes: #114

Here is the new output for mint mc tests:

{
  "name": "mc",
  "duration": "14",
  "function": "./mc rm --force --recursive target/mc-mint-test-bucket-19857",
  "status": "PASS"
}
{
  "name": "mc",
  "duration": "74",
  "function": "./mc rm --force --recursive target/mc-mint-test-bucket-8268",
  "status": "PASS"
}
{
  "name": "mc",
  "duration": "971",
  "function": "./mc rm --force --recursive target/mc-mint-test-bucket-3041",
  "status": "PASS"
}
{
  "name": "mc",
  "duration": "68",
  "function": "./mc rm --force --recursive target/mc-mint-test-bucket-27023",
  "status": "PASS"
}
{
  "name": "mc",
  "duration": "99",
  "function": "./mc rm --force --recursive target/mc-mint-test-bucket-12823",
  "status": "PASS"
}
{
  "name": "mc",
  "duration": "1088",
  "function": "./mc rm --force --recursive target/mc-mint-test-bucket-6179",
  "status": "PASS"
}
{
  "name": "mc",
  "duration": "48",
  "function": "./mc rm --force --recursive target/mc-mint-test-bucket-18387",
  "status": "PASS"
}
{
  "name": "mc",
  "duration": "5",
  "function": "./mc mb target/Mc-mint-test-bucket-1023",
  "status": "PASS"
}
{
  "name": "mc",
  "duration": "37",
  "function": "./mc rm --force --recursive target/mc-mint-test-bucket-5913",
  "status": "PASS"
}

log_pass() {
jq -n --arg name "${1}" --arg function "${2}" --arg args "${3}" \
--arg duration "${4}" --arg status "pass" \
'{"name": $name, "function": $function, "args": $args, "duration": $duration, "status": $status }'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Unable to understand jq usage for this json structure. You could use echo or printf command
  2. As this function is for PASS case, you would need to have hard-coded status in the json

log_fail() {
jq -n --arg name "${1}" --arg function "${2}" --arg args "${3}" \
--arg duration "${4}" --arg status "fail" --arg message "${5}" --arg error "${6}" \
'{"name": $name, "function": $function, "args": $args, "duration": $duration, "status": $status, "message": $message, "error": $error }'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

'{"name": $name, "function": $function, "args": $args, "duration": $duration, "status": $status, "message": $message, "error": $error }'
}

log_fail_with_alert() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could name it as log_alert()

# Create a bucket and check if it exists on server
makeBucket(){
make_bucket(){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I would recommend to follow function <name> { style which is more readable.
  2. You could need to fix formatting by given proper spacing for {

# Create a bucket and check if it exists on server
makeBucket(){
make_bucket(){
name="mc"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would need to keep this globally for all functions

if ! ./mc mb "target/${bucketName}" > /dev/null
then
>&2 echo "Make Bucket Test 1 Failure"
function="./mc mb target/${bucketName}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

camelCase style is not a convention. You would need to use bucket_name

# log end time in ms
end=$(($(date +%s%N)/1000000))
# find the duration
duration=$(( end - start ))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You would need to calculate duration before this if block.
  2. You would need to round to ms at duration computing than getting start and end
  3. start and end need to be named as start_time and end_time

# find the duration
duration=$(( end - start ))
# update log file
log_fail_with_alert "${name}" "${function}" "${args}" "${duration}" "Blocker" "mc mb failed" "$error"
return 1
fi

# remove bucket and cleanup
remove_bucket "${bucketName}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you skip checking remove_bucket?

@nitisht nitisht force-pushed the mc-json-logs branch 2 times, most recently from 1f812bf to f4c9740 Compare August 25, 2017 04:57
@nitisht
Copy link
Contributor Author

nitisht commented Aug 25, 2017

@balamurugana updated the PR.


remove_bucket() {
function remove_bucket() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can fix this function too.

printf '{"name": "mc", "duration": "%d", "function": "%s", "status": "FAIL", "error": "%d"}\n' "$1" "$2" "$3"
}

function log_failure_and_alert() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can name this function as log_alert() or log_alert_failure()

}

function log_failure() {
printf '{"name": "mc", "duration": "%d", "function": "%s", "status": "FAIL", "error": "%d"}\n' "$1" "$2" "$3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"error" is "%s"

}

function log_failure_and_alert() {
printf '{"name": "mc", "duration": "%d", "function": "%s", "status": "FAIL", "alert": "%d", error": "%d"}\n' "$1" "$2" "$3" "$4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not used anywhere. You could remove it.

if [ $rv -eq 0 ]; then
log_success "$(get_duration "$start_time")" "${function}"
else
log_failure "$(get_duration "$start_time")" "${function}" "${out}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could leave unquoted get_duration and start_time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shellcheck (the linter we use) recommends adding quotes here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


# Make bucket
./mc mb "target/${bucketName}" > /dev/null
bucket_name="mc-mint-test-bucket-$RANDOM"
function="${MC_CMD} mb ${SERVER_ALIAS}/${bucket_name}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do have setup and teardown style than creating/removing bucket every time.

return 1
# if download succeeds, verify downloaded file
if [ $rv -eq 0 ]; then
if [ "$hash1" == "$hash2" ] && [ "$(basename "$(echo "$out" | jq -r .target)")" == "datafile-1-MB" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to check target with the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to check the downloaded file name same as the uploaded file name.


# Make bucket - invalid bucket name
bucket_name="Mc-mint-test-bucket-$RANDOM"
function="${MC_CMD} mb ${SERVER_ALIAS}/${bucket_name}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unknown to understand this test and the behavior. This function always fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a negative test to make sure a bucket with invalid name is not created on the server.

local bucketName
bucketName="Abcd"
# Upload an object, with invalid object name
function put_object_error() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function would need a better than conveying about invalid object name.

printf "\n %s End of tests %s \n\n" "${blue}" "${normal}" 1
# Negative tests
make_bucket_error
put_object_error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to have test function name prefixing test_

bucketName=$(create_random_string)
# Tests for the cat object success by comparing the STDOUT of "cat run.sh",
# to "mc cat run.sh" by uploading run.sh, and comparing the two outputs.
function cat_objects() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this test for server? It doesn't make any different than cp in server point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not introduced in this commit. I assume the test is to check if cat streams the content properly.

I think it doesn't hurt to make sure the mc specific commands are working as expected with Minio server.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mint is meant for server testing, adding mc's cat command test is not a good idea.

For server, there is no difference whether returned http response body is saved, streamed or printed on the screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

bucketName=$(create_random_string)
# Tests for list object success, by mirroring an fs store to minio,
# and then comparing the ls results.
function mirror_list_objects() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not good idea to upload data dir for list objects. You could create objects using smallest file with different names. Mirror is not the right choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not introduced in this commit. I assume the test was to check if mirror works and uploads all the files and then it is verified via ls.

I will update the comments in the test case accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mint is for server testing, adding mc's mirror test is not applicable. It is mc specific feature syncing directory.

As this test is for list objects, using mirror is not a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mint is for server testing, adding mc's mirror test is not applicable. It is mc specific feature syncing directory.

The point of Mint is to test functionally if various libraries/SDKs/tools are working as expected with Minio server and of course if the server is working as expected.

If you say mirror is not applicable as mint does server testing, and mirror is just one feature of mc that does cp in the background. It implies we don't even need multiple SDKs, we can just test one SDK and be done -- since all the SDKs will call the same server APIs.

We use all SDKs to check if there is something broken in a specific scenario which may otherwise get overlooked. As mint runs all the SDKs / tools with their specific way to interact with server, we have the confidence that overall system (various clients + server) are all working fine.

So, I think mirror is needed and should not be removed. I can for sure add a new test case that does what you proposed - upload multiple files via cp and then perform a ls.

@nitisht nitisht force-pushed the mc-json-logs branch 2 times, most recently from d6a9283 to a6504af Compare September 1, 2017 04:28
@nitisht
Copy link
Contributor Author

nitisht commented Sep 1, 2017

Updated with make_bucket delete_bucket @balamurugana . Please take a look

Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am taking this immediately for minio PR verification test

@balamurugana balamurugana merged commit 62d3383 into minio:master Sep 1, 2017
Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nitisht nitisht deleted the mc-json-logs branch September 14, 2017 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants