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

Fix issue #3372 #3373

Merged
merged 4 commits into from
Apr 7, 2018
Merged

Fix issue #3372 #3373

merged 4 commits into from
Apr 7, 2018

Conversation

backbord
Copy link

@backbord backbord commented Apr 6, 2018

Add bugfix and test for #3372: Exception classes may have a type that looks iterable which caused problems with pytest.raises.

Unfortunately, I wasn't able to use base_type as proposed by Ronny: pytest.raises is passed an exception class and I didn't find a way around calling issubclass.

Only tested in py3.6.


Not a regular contributer, so I hope everything is up to speed and in accordance with your guidelines.

@coveralls
Copy link

coveralls commented Apr 6, 2018

Coverage Status

Coverage increased (+0.05%) to 92.841% when pulling 5bd8561 on backbord:master into 5d4fe87 on pytest-dev:master.

" derived from BaseException, not %s")
raise TypeError(msg % type(exc))
if not isclass(expected_exception) or not issubclass(expected_exception, BaseException):
for exc in filterfalse(isclass, always_iterable(expected_exception)):
Copy link
Member

Choose a reason for hiding this comment

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

please try using base_type=type

Copy link
Author

Choose a reason for hiding this comment

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

oh, that sounds nice! just a moment.

Copy link
Author

Choose a reason for hiding this comment

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

just to be clear: do you want base_type=type or base_type=(type, six.text_type, six.binary_type)?

Copy link
Member

Choose a reason for hiding this comment

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

i beleive the shorter variant is sufficient, please try ^^

Copy link
Author

Choose a reason for hiding this comment

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

it works in both variants. :)

upon further inspection, i'd prefer the longer variant as i believe it to lead to nicer error messages as strings don't get split into individual characters.

base_type = (type, text_type, binary_type)
for exc in filterfalse(isclass, always_iterable(expected_exception, base_type)):
    msg = ("exceptions must be old-style classes or"
           " derived from BaseException, not %s")
    raise TypeError(msg % type(exc))

however, please feel free to modify the PR to your liking. ^^

Copy link
Author

Choose a reason for hiding this comment

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

and (sorry for the many messages): thank you for reviewing and commenting! it's greatly appreciated. :D

Copy link
Member

Choose a reason for hiding this comment

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

you are doing great work 👍

msg = ("exceptions must be old-style classes or"
" derived from BaseException, not %s")
" derived from BaseException, not %s")
Copy link
Member

Choose a reason for hiding this comment

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

this one is the unfortunate dedent mistake that broke the linting job ^^

Copy link
Author

Choose a reason for hiding this comment

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

oh man. thanks for the pointer

@nicoddemus
Copy link
Member

Awesome contribution @backbord, thanks a lot. Expect it to be published in the next bug-fix release. 😁

@nicoddemus nicoddemus merged commit e012dbe into pytest-dev:master Apr 7, 2018
@nicoddemus
Copy link
Member

Sent an invitation to @backbord as our policy.

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.

5 participants