Test all constants reexported

[stevenjackson121]

trafficstars

The test actually fails because EAGAIN is not exported on my machine (which shows it may be a valuable test, but also means there's potentially work to do in the exporting magic, some of which goes above my head...).

Python 3.7.2 (tags/v3.7.2:9a3ffc0492, Dec 23 2018, 23:09:28) [MSC v.1916 64 bit (AMD64)] on win32
import socket
socket.EAGAIN
Out[3]: 11
from trio import socket as tsocket
tsocket.EAGAIN
Traceback (most recent call last):
  File "C:\Users\kiwini\PycharmProjects\trio\venv\lib\site-packages\IPython\core\interactiveshell.py", line 3326, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-5-14d153927893>", line 1, in <module>
    tsocket.EAGAIN
AttributeError: module 'trio.socket' has no attribute 'EAGAIN'

Guidance on where to head from here appreciated! ~(As I learned while making this PR, exports are platform dependent, is this test even going to be valuable on the CI machine or would it take a dev running windows like me to actually cause failures?)~ Oh it looks like the CI does run against a wide variety of machines, and definitely catches some errors. I'll try wading through them to see what specific exports are being missed.

Closes #825

[stevenjackson121]

So it looks like the things that aren't being re-exported aren't in stdlib_socket.__all__ . If membership in stdlib_socket.__all__ is the correct test for whether is should be exported by trio_socket then (Option 1) I should modify the test. If "is it exported by stdlib_socket " is the correct test, then the import code in trio_socket.py needs to be updated, either by (Option 2) switching away from __all__ in the following block:

globals().update(
    {
        _name: getattr(_stdlib_socket, _name)
        for _name in _stdlib_socket.__all__ if _name.isupper()
    }
)

or else by (Option 3) handling all the non-__all__ cases down below.

Please let me know what you'd consider preferable!

[pquentin]

(I edited the description of the pull request to mention that it's going to close #825 so that Github closes the issue for us when we merge your PR! When we don't do that, we usually forget to close the issue.)

The correct fix is Option 1. We want trio.socket to behave like socket for platform-dependant symbols, which is why we don't export every symbol for every platform. The subprocess export test was removed in #858, but it looked like that:

def test_all_constants_reexported():
    trio_subprocess_exports = set(dir(subprocess))
    import subprocess as stdlib_subprocess

    for name in dir(stdlib_subprocess):
        if name.isupper() and name[0] != "_":
            stdlib_constant = name
            assert stdlib_constant in trio_subprocess_exports

You want the same thing, but for socket and trio.socket. Does that make sense? (And this is why #825 was not a good first issue, sorry about that.)

[stevenjackson121]

@pquentin thanks for the reply, but I think there is some miscommunication occurring. I believe that

The correct fix is Option 1.

and

We want trio.socket to behave like socket for platform-dependant symbols

are mutually exclusive statements.

From my understanding, __all__ does not include everything that's exported, but only those things which should be imported when a user uses the from socket import * syntax (see https://docs.python.org/3/tutorial/modules.html#importing-from-a-package).

Based on the rest of your comment, it appears that we want to go further than just ensuring from trio.socket import * behaves as from socket import *, we also want from trio.socket import EAGAIN to work iff from socket import EAGAIN works. This currently does not work (see the code snippet in my first comment), and would require code changes. In other words: the test is good as is, and socket.py itself needs to be updated to make sure the test passes (using either something besides __all__ in the dictionary comprehension--which I called Option 2-- or by handling special cases down below-- which I termed Option 3)

"Option 1" was a suggestion to modify the test so that we only care about __all__, which is the opposite approach (leave socket.py alone and pare down what the tests checks for).

Please let me know if what I said makes sense, or if I've confused the issue further.

[pquentin]

Thanks, your explanation makes sense! I think the misunderstanding is here:

we also want from trio.socket import EAGAIN to work iff from socket import EAGAIN works.

We don't want that to work! I just realized that EAGAIN is a constant from the errno module:

Python 3.7.4 (default, Sep  7 2019, 18:27:02)
[Clang 10.0.1 (clang-1001.0.46.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import errno
>>> errno.EAGAIN
35
>>> import socket
>>> socket.EAGAIN
35
>>> socket.EAGAIN is errno.EAGAIN
True

In other words, the fact that from socket import EAGAIN works is an implementation detail: it works only because socket currently imports it, but that could change in the future. I'd argue that it's unfortunate that it even works in the presence of __all__. This whole module thing is a mess that I always struggle to understand. :( Anyway, it's not something we want to expose in trio.socket. We really only care about the content of __all__, which is the public API of the socket module.

Does that make sense?

[stevenjackson121]

Your explanation makes sense. Thanks for clearing that up!

In that case, I'm pretty confident we should probably also modify lines 22-108 socket.py (not directly, I guess by modifying test_exports.py) since in (line 58)[https://github.com/python-trio/trio/blob/837a56ec6972a4f3d872ddf04be8e9f0f1d22cd3/trio/socket.py#L58] we do explicitly claim to provide EAGAIN (at least, static analysis tools like mypy will believe this to be the case, and confuse anyone who uses them). We should pare down everything that's not in __all__ (or explicitly declared in socket.py) there, since there's no way we're actually going to expose it to users. Do you agree?

[pquentin]

Ooh, that's a really good catch. I'm glad that your work here is going to help improve the exporting logic.

I agree, EAGAIN should not be there!

[pquentin]

However, fixing EAGAIN and similar constants is a little complicated for various reasons and can be fixed in a future pull request, I think.

[pquentin]

@stevenjackson121 Is there anything I can help with?