[issue37890] Modernize several tests in test_importlib

[Kyle Stanley]

Kyle Stanley  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Brett Cannon]

Brett Cannon  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Kyle Stanley]

Kyle Stanley  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Brett Cannon]

Brett Cannon  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Kyle Stanley]

Kyle Stanley  added the comment:

> 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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Kyle Stanley]

Kyle Stanley  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Brett Cannon]

Brett Cannon  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Kyle Stanley]

Change by Kyle Stanley :


--
stage:  -> needs patch
type:  -> enhancement

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Kyle Stanley]

Change by Kyle Stanley :


--
nosy: +eric.snow, ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Kyle Stanley]

Kyle Stanley  added the comment:

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(, ): ...`` 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37890] Modernize several tests in test_importlib

[Kyle Stanley]

New submission from Kyle Stanley :

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".

--
components: Tests
messages: 349984
nosy: aeros167, brett.cannon
priority: normal
severity: normal
status: open
title: Modernize several tests in test_importlib
versions: Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com