classification
Title: Modernize several tests in test_importlib
Type: enhancement Stage: needs patch
Components: Tests Versions: Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: aeros, brett.cannon, eric.snow, ncoghlan, serhiy.storchaka
Priority: normal Keywords:

Created on 2019-08-20 02:48 by aeros, last changed 2019-08-22 23:12 by aeros.

Messages (10)
msg349984 - (view) Author: Kyle Stanley (aeros) * (Python triager) Date: 2019-08-20 02:48
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".
msg349985 - (view) Author: Kyle Stanley (aeros) * (Python triager) Date: 2019-08-20 03:43
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.
msg349993 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-20 07:02
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.
msg350029 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-08-20 18:21
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.
msg350040 - (view) Author: Kyle Stanley (aeros) * (Python triager) Date: 2019-08-20 23:49
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 import_module() 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.
msg350042 - (view) Author: Kyle Stanley (aeros) * (Python triager) Date: 2019-08-21 01:09
> 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?
msg350096 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-08-21 16:45
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.
msg350099 - (view) Author: Kyle Stanley (aeros) * (Python triager) Date: 2019-08-21 17:14
> 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.
msg350217 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-08-22 17:50
I would just read through the other tests files under test_importlib to see how other tests were done.
msg350239 - (view) Author: Kyle Stanley (aeros) * (Python triager) Date: 2019-08-22 23:12
> 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.
History
Date User Action Args
2019-08-22 23:12:26aerossetmessages: + msg350239
2019-08-22 17:50:24brett.cannonsetmessages: + msg350217
2019-08-21 17:14:54aerossetmessages: + msg350099
2019-08-21 16:45:40brett.cannonsetmessages: + msg350096
2019-08-21 01:09:47aerossetmessages: + msg350042
2019-08-20 23:49:17aerossetmessages: + msg350040
2019-08-20 18:21:58brett.cannonsetmessages: + msg350029
2019-08-20 07:02:11serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg349993
2019-08-20 05:04:25aerossettype: enhancement
stage: needs patch
2019-08-20 03:49:09aerossetnosy: + ncoghlan, eric.snow
2019-08-20 03:43:42aerossetmessages: + msg349985
2019-08-20 02:48:16aeroscreate