-
Notifications
You must be signed in to change notification settings - Fork 352
tests: Negative tests should ignore exceptions properly #658
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
Conversation
30a76a3 to
fd79361
Compare
tests/functional/tests.py
Outdated
| except ResponseError as err: | ||
| pass | ||
| if err.code != 'XMinioInvalidObjectName': | ||
| raise Exception(err) |
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 looks ugly to wrap an exception with Exception which is unnecessary. I see this is used all over the code.
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.
Yes I kept it as it is.. will need to change comprehensively later.
tests/functional/tests.py
Outdated
| finally: | ||
| try: | ||
| client.remove_object(bucket_name, object_name) | ||
| try: |
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.
There are 2 try blocks back to back.
First try block is unnecessary.
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.
you need it... otherwise, we will crash. Look at the original issue why this fix is necessary. #5954
tests/functional/tests.py
Outdated
| else: | ||
| pass | ||
| except Exception as err: | ||
| raise Exception(err) |
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.
@harshavardhana , lines 635 and 636 can be removed -since these 2 lines are just re-raising the exception, which would have happened anyway.
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 am not able to understand... how would that happen?
tests/functional/tests.py
Outdated
| raise Exception(err) | ||
| else: | ||
| pass | ||
| except Exception as err: |
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.
lines 646,647 can be removed -since they are just re-raising the exception that would have been raised even without these statements.
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 am not able to understand... how would that happen?
fd79361 to
08283c6
Compare
08283c6 to
d1a32e6
Compare
No description provided.