Modernize several tests in test_importlib

[aeros]

BPO	37890
Nosy	@brettcannon, @ncoghlan, @ericsnowcurrently, @serhiy-storchaka, @aeros

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-feature', 'tests', '3.9'] title = 'Modernize several tests in test_importlib' updated_at = user = 'https://github.com/aeros' ``` bugs.python.org fields: ```python activity = actor = 'aeros' assignee = 'none' closed = False closed_date = None closer = None components = ['Tests'] creation = creator = 'aeros' dependencies = [] files = [] hgrepos = [] issue_num = 37890 keywords = [] message_count = 10.0 messages = ['349984', '349985', '349993', '350029', '350040', '350042', '350096', '350099', '350217', '350239'] nosy_count = 5.0 nosy_names = ['brett.cannon', 'ncoghlan', 'eric.snow', 'serhiy.storchaka', 'aeros'] pr_nums = [] priority = 'normal' resolution = None stage = 'needs patch' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue37890' versions = ['Python 3.9'] ```

[aeros]

Last month, several tests were moved into test_importlib (https://bugs.python.org/issue19696): "test_pkg_import.py", "test_threaded_import.py", and "threaded_import_hangers.py".

Those tests were created quite a while ago though and do not currently utilize importlib directly. They should be updated accordingly.

Brett Cannon:

BTW, if you want to open a new issue and modernize the tests to use importlib directly that would be great!

I'm interested in helping with this issue, but I may require some assistance as I'm not overly familiar with the internals of importlib. I'll probably start with "test_pkg_import.py".

Anyone else can feel free to work on the other two in the meantime, but they should be worked on together as "threaded_import_hangers.py" is a dependency for "test_threaded_import.py".

[aeros]

I'm not entirely certain as to which parts should be modernized, and which ones can remain the same. A large part of my uncertainty is that there are no header comments for "test_pkg_import.py" to explain the test coverage, so I don't know if there are additional tests beyond the existing ones that should be added.

The first steps I can think of:

1) Use importlib.import_module() instead of the built-in __import__()

2) Use with self.assertRaises(<exception>, <msg>): ... instead of

try: __import__(self.module_name)
        except SyntaxError: pass
        else: raise RuntimeError('Failed to induce SyntaxError') # self.fail()?

...

3) Replace self.assertEqual(getattr(module, var), 1) with self.assertEqual(getattr(module, var, None), 1) so that AttributeErrors are not raised when unexpected behaviors occur

4) Provide useful error messages for failed assertions

I'll begin working on those, probably with a separate PR for each of them for ease of review. Let me know if there's anything else I should do, or if any of the above steps are unneeded. If I think of anything else I'll update the issue accordingly.

[serhiy-storchaka]

1) __import__() can be used for purpose. I would not change this.

3) How would you distinguish the case when the module have an attribute with the value is None and when it does not have the attribute at all? This information would lost with your change.

[brettcannon]

What Serhiy said. :) There's code to be able to easily test both builtins.__import and importlib.__import in tests so that there's no drift between the two implementations.

[aeros]

Ah okay, I wasn't sure what exactly would be involved with the "modernization" process, so those points were just rough ideas more than anything. I haven't started working on anything yet since I figured it'd be worthwhile to wait for approval first.

1) __import__() can be used for purpose. I would not change this.

Okay, I'll keep that part as is then. This idea was primarily based on importlib's documentation:

"Programmatic importing of modules should use importmodule() instead of [importlib.\_import__()]"

But that probably applies more to users of importlib, rather than the internal tests for it. That would make sense.

3) How would you distinguish the case when the module have an attribute with the value is None and when it does not have the attribute at all? This information would lost with your change.

Good point. This might be a decent way to prevent the AttributeErrors, but still allows for differentiation of actual None values:

try:
    self.assertEqual(getattr(module, var), 1)
except AttributeError:
    self.fail(f"{module} should have attribute {var}")

Personally I think it makes a bit more sense to use self.fail() with a helpful message rather than raising errors within the tests. There's a comment on line 56, "# self.fail() ?", which gave me this idea. Is there a particular preference in the context of Python's tests?

Also, do either of you (or anyone else) have any ideas for other modernization improvements that could be made to either test_pkg_import.py or to the other two? In the meantime, I can start working on ideas (2) and (4) if those ones would be appropriate. (2) should be fairly straightforward, but (4) will probably be a bit more subjective.

[aeros]

This might be a decent way to prevent the AttributeErrors, but still allows for differentiation of actual None values

Another alternative solution might be to use hasattr() before getattr(), if it is not desirable for test_pkg_import.py to raise exceptions:

 self.assertTrue(hasattr(module, var), msg=f"{module} should have attribute {var}")
 self.assertEqual(getattr(module, var), 1)

That would follow more of a LBYL style, but I know that EAFP is more common within Python. The better alternative depends on the answer to my earlier question regarding exceptions being raised from the unit tests:

Is there a particular preference in the context of Python's tests?

[brettcannon]

A key question here is why are you trying to avoid the AttributeError case so much? If something has a bug and an attribute doesn't exist that should then that's a bug and the test needs to catch that. Now whether that's via an error from AttributeError being raise or an explicit failure case you still get notified, but there's a reason that we don't have attribute existence tests before every single attribute access throughout the test suite. ;) Basically unless the attribute is dynamic and thus you're explicitly testing the attribute got set then don't worry about it.

[aeros]

A key question here is why are you trying to avoid the AttributeError case so much?

but there's a reason that we don't have attribute existence tests before every single attribute access throughout the test suite

Hmm, good point. I may have been fixating on avoiding exceptions within the tests a bit too much, perhaps the exception would be appropriate in this case.

Basically unless the attribute is dynamic and thus you're explicitly testing the attribute got set then don't worry about it.

Ah, that answers my earlier question, thanks. (:

With an initial glance, do you have any specific ideas as to how test_pkg_import.py should be changed to utilize importlib directly? I've been reading through the docs looking for ideas, but I'll admit that I'm not particularly experienced with using importlib.

[brettcannon]

I would just read through the other tests files under test_importlib to see how other tests were done.

[aeros]

I would just read through the other tests files under test_importlib to see how other tests were done.

Okay, I'll start with that and report back with any ideas for potential changes to test_pkg_import. Thanks.