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

bugfix for issue #2491 #2791

Merged
merged 4 commits into from
Oct 25, 2017
Merged

bugfix for issue #2491 #2791

merged 4 commits into from
Oct 25, 2017

Conversation

OfirOshir
Copy link
Contributor

@OfirOshir OfirOshir commented Sep 19, 2017

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bug)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of removal, feature, bugfix, vendor, doc or trivial
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
  • Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS;

…es the original traceback.

 In python2 it will show the original traceback with a new message that explains in which plugin.
 In python3 it will show 2 canonized exceptions, the original exception while loading the plugin in addition to an exception that PyTest throws about loading a plugin.
…s that the original traceback has been shown to the users
@OfirOshir OfirOshir mentioned this pull request Sep 19, 2017
4 tasks
@OfirOshir
Copy link
Contributor Author

the tests failed but i don't understand why can someone plz explain to me?

@obestwalter
Copy link
Member

python3.5: command not found\n\nThe `python3.5' command exists in these Python versions:\n 3.5\n 3.5.3\n\n"'

Looks like a glitch on travis - nothing to do with your changes.

@nicoddemus
Copy link
Member

Indeed this is fixed by #2790

@OfirOshir
Copy link
Contributor Author

ok, so what can i do?

@obestwalter
Copy link
Member

You can close and reopen the pr then the tests are triggered again.

@nicoddemus
Copy link
Member

Actually #2790 has not been merged yet. @OfirOshir please hold on until we merge it, thanks for your patience.

@@ -0,0 +1,4 @@
If an exception happens while loading a plugin, PyTest no longer hides the original traceback.
Copy link
Member

Choose a reason for hiding this comment

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

It is actually pytest, not PyTest. 😉

@obestwalter
Copy link
Member

ups ... sorry 😊

@nicoddemus
Copy link
Member

No worries! 😄

@nicoddemus nicoddemus closed this Oct 24, 2017
@nicoddemus nicoddemus reopened this Oct 24, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 92.652% when pulling d96869f on OfirOshir:features into 9273e11 on pytest-dev:features.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

This can be merged now, the linting issues will be addressed by #2869 and the xdist/nobyte failures are related to #2843

@RonnyPfannschmidt RonnyPfannschmidt merged commit f743e95 into pytest-dev:features Oct 25, 2017
@RonnyPfannschmidt
Copy link
Member

thanks 👍

new_exc = new_exc_type(new_exc_message)

six.reraise(new_exc_type, new_exc, sys.exc_info()[2])

Copy link
Contributor

Choose a reason for hiding this comment

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

why this verbose? Why not do it this way?

six.reraise(ImportError, ImportError('Error importing plugin "%s": %s' % (modname, safe_str(e.args[0]))), sys.exc_info()[2])

Copy link
Member

Choose a reason for hiding this comment

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

Well both versions are correct, it is more of a matter of style in this case I guess.

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.

6 participants