bpo-31315: Fix an assertion failure in imp.create_dynamic(), when spec.name is not a string

[orenmn]

• in importdl.c - add a check whether spec.name is not a string.
• in test_imp.py - add a test to verify that the assertion failure is no more.

https://bugs.python.org/issue31315

[ncoghlan]

The actual change looks good, just needs a blank line before the new decorator function in the tests

[ncoghlan]

Readability improvement: blank line before the function

[ericsnowcurrently]

Thanks for doing this, Oren. I've left a few minor comments for what I consider nits. I don't consider them blockers so at least just consider them. :) Otherwise LGTM.

[ericsnowcurrently]

Let me be clear that my recommendation here is not critical. Rather it is a reflection of my belief that little suboptimal things add up over time to become pain points. Docstrings (including in test code) are one of those things we cut corners on sometimes.

While there is precedent (unfortunately) for docstrings formatted this way, I'd rather this docstring were a bit more idiomatic. It's better to take a little time now to strengthen a healthier precedent. This is a case where a little bit of extra time spent adds up over time to make a positive difference. I don't mean to be pedantic here about a relatively insignificant style point or to waste anyone's time, but instead want to encourage a more thoughtful approach with regard to the little things. :) With that out of the way...

I'd expect a docstring here like one of the following (and including some shortening):

# one line
"""Decorator to skip a test if not running under CPython or lacking imp.create_dynamic()."""

or

# one line with separate quotation mark lines
"""
Decorator to skip a test if not running under CPython or lacking imp.create_dynamic().
"""

or

# split into multiple lines (my preference)
"""A decorator to skip tests that rely on imp.create_dynamic().

Not all Python implementations necessarily provide imp.create_dynamic().
This decorators causes decorated tests to be skipped through the normal
unittest mechanism.
"""

[ericsnowcurrently]

FWIW, as a reader it's easy to miss something when more than one thing is happening on a line. It might be more clear to break this up:

deco = unittest.skipIf(not hasattr(imp, 'create_dynamic'),
                                  'imp.create_dynamic() required')
return deco(meth)

or even

supported = hasattr(imp, 'create_dynamic')
deco = unittest.skipIf(not supported, 'imp.create_dynamic() required')
return deco(meth)

[serhiy-storchaka]

Use something more realistic. A bytes object or None.

[serhiy-storchaka]

If origin is not a string SystemError can be raised. This should be fixed too.

[orenmn]

what code causes a SystemError?
I got a TypeError: bad argument type for built-in operation both on my Windows and Ubuntu.

[serhiy-storchaka]

Your are right. I thought _PyUnicode_AsUnicode() raises SystemError, but it raises TypeError. No additional check is needed.

[serhiy-storchaka]

I would prefer skipUnless().

But why the requires_create_dynamic decorator is needed? It is used just once. Why not just use two decorators directly?

@unittest.skipUnless(hasattr(imp, 'create_dynamic'), 'imp.create_dynamic() required')
@support.cpython_only
def test_issue31315(self):
    ...

Or use special purposed support.get_attribute():

@support.cpython_only
def test_issue31315(self):
    create_dynamic = support.get_attribute(imp, 'create_dynamic')
    ...

[miss-islington]

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

[bedevere-bot]

GH-3653 is a backport of this pull request to the 3.6 branch.