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

Adds missing tests and fixes tests with problems #609

Merged
merged 1 commit into from Jan 11, 2018

Conversation

ebozduman
Copy link
Collaborator

Fixes #562

@@ -1208,7 +1208,6 @@ def remove_incomplete_upload(self, bucket_name, object_name):
if object_name == upload.object_name:
self._remove_incomplete_upload(bucket_name, object_name,
upload.upload_id)
return
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

client.make_bucket(bucket_name)
# Check if bucket was created properly
log_output.function = 'bucket_exists(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.

Isn't it little confusing to change function name multiple times in the same test function ? I thought log_output.function should represent the name of the function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is the name of the API being tested

Copy link
Collaborator Author

@ebozduman ebozduman Jan 10, 2018

Choose a reason for hiding this comment

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

If you check the end of tests.py file (starting with line#1555), you'll notice, we first create an instance of LogOutput class, log_output, and we initialize its function attribute with the name of the api command/method that will go under test.

For example, in the above test case "test_make_bucket_default_region", log_output.function is set to "make_bucket(bucket_name, location)". This is the default value and the same logic is used for all test cases.

Because some tests, like "test_make_bucket_default_region", depend on some other helper api commands, it would be misleading to report a helper api command,"bucket_exists" in our case, failure with the name of the api command, "make_bucket" in our case, under test. That's why, I set log_output.function to "bucket_exists(bucket_name)" before calling "bucket_exists" api/helper command and change it to the later helper api command, if one is called in the next step, "remove_bucket" in our case. This way, the name of the failed api command will be reflected correctly in the generated Json format output in the function field. If everything goes well, then we set the function field back to the name of the api command under test before reporting a "PASS" status for the test.

Since the code is not clear, (@vadmeste and I exchanged ideas on Slack and we agreed on) it would be helpful to have comments that explain the default value of log_output.function for each test case.

Copy link
Collaborator Author

@ebozduman ebozduman Jan 11, 2018

Choose a reason for hiding this comment

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

Done and squashed.

Copy link
Member

Choose a reason for hiding this comment

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

we can simplify the code in another PR

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.

LGTM

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM

@kannappanr kannappanr merged commit 379711f into minio:master Jan 11, 2018
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.

None yet

4 participants