bugfix for issue #2491

[OfirOshir]

in python2 we will reraise with the original traceback
in python3 will raise 2 canonized exceptions

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;

[RonnyPfannschmidt]

please take a look at using six.reraise/six.raise_from

[nicoddemus]

Agreed, six.reraise seems to be more appropriate here (six.raises_from is the same as raise new_exc in Python 2).

We want to preserve the original traceback as much as possible, but also want to include which plugin was the cause of the exception. After playing around this code seems to do the trick:

import sys
import six

def do_import(name):
    raise ValueError('some bizarre error not related to the plugin name at all')

def import_some_plugin(name):
    do_import(name)

def import_plugins():

    try:
        import_some_plugin("myplugin")
    except Exception as e:
        new_e = type(e)(str(e) + '\n' + 'Error importing pytest plugin: "myplugin"')
        six.reraise(type(e), new_e, sys.exc_info()[2])

import_plugins()

I get in Python 2.7:

.tmp\foo.py:18: in <module>
    import_plugins()
.tmp\foo.py:16: in import_plugins
    six.reraise(type(e), new_e, sys.exc_info()[2])
.tmp\foo.py:13: in import_plugins
    import_some_plugin("myplugin")
.tmp\foo.py:8: in import_some_plugin
    do_import(name)
.tmp\foo.py:5: in do_import
    raise ValueError('some bizarre error not related to the plugin name at all')
E   ValueError: some bizarre error not related to the plugin name at all
E   Error importing pytest plugin: "myplugin"
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!

And Python 3.6:

Traceback (most recent call last):
  File "C:/pytest/.tmp/foo.py", line 17, in import_plugins
    import_some_plugin("myplugin")
  File "C:/pytest/.tmp/foo.py", line 11, in import_some_plugin
    do_import(name)
  File "C:/pytest/.tmp/foo.py", line 7, in do_import
    raise ValueError('some bizarre error not related to the plugin name at all')
ValueError: some bizarre error not related to the plugin name at all

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:/pytest/.tmp/foo.py", line 23, in <module>
    import_plugins()
  File "C:/pytest/.tmp/foo.py", line 20, in import_plugins
    six.reraise(type(e), new_e, sys.exc_info()[2])
  File "C:\pytest\.env36\lib\site-packages\six.py", line 685, in reraise
    raise value.with_traceback(tb)
  File "C:/pytest/.tmp/foo.py", line 17, in import_plugins
    import_some_plugin("myplugin")
  File "C:/pytest/.tmp/foo.py", line 11, in import_some_plugin
    do_import(name)
  File "C:/pytest/.tmp/foo.py", line 7, in do_import
    raise ValueError('some bizarre error not related to the plugin name at all')
ValueError: some bizarre error not related to the plugin name at all
Error importing pytest plugin: "myplugin"

What do you think @RonnyPfannschmidt? In Python 3 we might consider not adding the extra message.

[nicoddemus]

Please use six.reraise as suggested

[OfirOshir]

I changed the code according to your suggestions.

[nicoddemus]

I changed the code according to your suggestions.

Thanks @OfirOshir, LGTM now! 👍

Some things still need to be done if you can:

• Add a regression test for it, I suggest updating test_importplugin_error_message. You can make the test file which imports the module call an internal function, and then ensure the name of the function appears on the output (thus demonstrating we are preserving the original traceback).
• Add a CHANGELOG note.

[OfirOshir]

i have no problem adding a regression test.
but where do i put the changelog and in which format?

[nicoddemus]

Add a file named changelog/2491.bugfix with a short explanation of the fix, something like:

If an exception happens while loading a plugin, pytest no longer hides the original traceback.

The explanation ideally should be helpful to end users to understand the issue that was fixed; technical details are not interesting to them, those should be present in the commit messages.

[OfirOshir]

I updated the test as instructed and I also added a changelog.
I also added six to the required packages.

[The-Compiler]

Since this adds a new dependency, this should probably go to the features branch now, no?

[The-Compiler]

Missing :

[OfirOshir]

fixed

[OfirOshir]

I dont think a new dependecy is qualified as a feature.

[RonnyPfannschmidt]

while i understand the sentiment in this, a new dependency requires a change to anyone using a pinned dependency set

since a new dependency requires a controlled change in users, its reasonable to consider this as a "feature" (since its not breaking, but still more noticeable than just a bugfix)

[nicoddemus]

@RonnyPfannschmidt is right, and sorry @OfirOshir for not realizing this earlier. In fact, the features branch already depends on six.

Unfortunately I think you will need to rebase your commits on features and fix the conflicts. Again, sorry for not noticing it earlier, it was a mistake on our part.

[The-Compiler]

@OfirOshir Did you intend to close this?

[OfirOshir]

created a new pr from feature branch, #2791