Add +LINUX and +WINDOWS doctest options
#2507
Conversation
This allows to selectively run tests only on a single platform. We can add `# doctest: +LINUX` comments to tests that cannot work on Windows and the other way around. To easily skip a lot of tests the `doctest_additional_flags` global variable can be defined in a `testsetup`. This is achieved by monkey patching sphinx doctest's DocTestBuilder to use our own DocTestRunner which removes examples from the tests that have flags that don't match the platform we're running on.
Avoid major versions which might change the API. We have to check if the platform optionflags still work on newer versions once they are available.
Disable all non-trivial tests on Windows for now. The goal is to reduce the amount of linux-only tests.
The handrolled coveralls upload cannot handle mixed operating systems. Refs Gallopsled#2480
docs/source/conf.py
Outdated
| signal.alarm(600) # ten minutes | ||
| if sys.platform == 'win32': | ||
| def alrm_handler(): | ||
| raise EndlessLoop() |
There was a problem hiding this comment.
Does it really interrupt hanging tests? If so, this should be the default way for all platforms.
There was a problem hiding this comment.
Good point, I'll actually try this with a shorter timeout. I was blindly trusting stack overflow like one should do.
Do you know why the alarm signal is reset to 180 in the signal handler?
There was a problem hiding this comment.
The following timeout is smaller so that as many hanging tests are discovered as possible in a single Sphinx batch run (and to save CI minutes on hangs, as a byproduct). This is just a debugging measure that happens to be useful more often than it should, so I guess we can skip it altogether if there is no platform support. But would be nice to have regardless.
There was a problem hiding this comment.
I read up on timeouts and signals in Python. I think I came up with a platform independent way which behaves the same as the signal.alarm implementation does right now.
|
We can totally have features only implemented for one platform with bail outs like this if sys.platform =="win32":
log.error("X is not supported on Windows")I think that would keep changes/additions as low friction as possible. If someone needs the feature for other operating systems, they can implement it later. |
|
Yes, but I think we should test for features rather than platform names where possible, in order to be future-proof. |
To interrupt the code running on the main thread, we send a signal using `_thread.interrupt_main()`. By default this causes a KeyboardInterrupt exception, which might be handled explicitly. To raise an explicit EndlessLoop exception inside the code that is taking too long, register a SIGABRT signal handler which raises the EndlessLoop exception. The exception from the signal handler is added to the call stack and handled by the code currently running. This allows to print a better stack trace on timeout. It is the same concept as the old implementation using `signal.alarm` but platform agnostic. https://anonbadger.wordpress.com/2018/12/15/python-signal-handlers-and-exceptions/
Run test on other UNIX systems too if they don't use Linux specifics. Add a TODO optionflag too to mark platform restrictions that might be too strict and should be looked at.
docs/source/conf.py
Outdated
| class EndlessLoop(Exception): pass | ||
| def alrm_handler(sig, frame): | ||
| signal.alarm(180) # three minutes | ||
| class EndlessLoop(BaseException): pass |
There was a problem hiding this comment.
BaseException? so it will now interrupt doctest flow and not just the one test, right?
There was a problem hiding this comment.
No, it's still just one test, but the exception won't be caught by other exception handlers in our own code.
File "..\..\pwnlib\context\__init__.py", line ?, in default
Failed example:
r=remote('google.com', 80)
Expected:
Traceback (most recent call last):
...
ProxyConnectionError: Error connecting to SOCKS5 proxy localhost:1080: [Errno 111] Connection refused
Got:
Traceback (most recent call last):
File "C:\Users\Jannik\Documents\GitHub\pwntools\venv\Lib\site-packages\socks.py", line 787, in connect
super(socksocket, self).connect(proxy_addr)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
ConnectionRefusedError: [WinError 10061] Es konnte keine Verbindung hergestellt werden, da der Zielcomputer die Verbindung verweigerte
<BLANKLINE>
During handling of the above exception, another exception occurred:
<BLANKLINE>
Traceback (most recent call last):
File "C:\Users\Jannik\AppData\Local\Programs\Python\Python313\Lib\doctest.py", line 1395, in __run
exec(compile(example.source, filename, "single",
~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
compileflags, True), test.globs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<doctest default[1]>", line 1, in <module>
r=remote('google.com', 80)
File "C:\Users\Jannik\Documents\GitHub\pwntools\pwnlib\tubes\remote.py", line 85, in __init__
self.sock = self._connect(fam, typ)
~~~~~~~~~~~~~^^^^^^^^^^
File "C:\Users\Jannik\Documents\GitHub\pwntools\pwnlib\tubes\remote.py", line 130, in _connect
sock.connect(sockaddr)
~~~~~~~~~~~~^^^^^^^^^^
File "C:\Users\Jannik\Documents\GitHub\pwntools\venv\Lib\site-packages\socks.py", line 47, in wrapper
return function(*args, **kwargs)
File "C:\Users\Jannik\Documents\GitHub\pwntools\venv\Lib\site-packages\socks.py", line 791, in connect
self.close()
~~~~~~~~~~^^
File "C:\Users\Jannik\Documents\GitHub\pwntools\venv\Lib\site-packages\socks.py", line 413, in close
def close(self):
<BLANKLINE>
File "C:\Users\Jannik\Documents\GitHub\pwntools\docs\source\conf.py", line 494, in sigabrt_handler
raise EndlessLoop()
EndlessLoop
docs/source/conf.py
Outdated
| import _thread as thread | ||
| # pre Python 3.10 this raises a KeyboardInterrupt in the main thread. | ||
| # it might not show a traceback in that case, but it will stop the endless loop. | ||
| thread.interrupt_main(signal.SIGABRT) |
There was a problem hiding this comment.
Does this work in 3.9? I think the if should be here rather than there. Or should we os.kill the main thread directly (works on win32 supposedly)? Does this work with tests hanging on IO (read from a socket/pipe)? Maybe we can just support this development feature on platforms with alarm(2) and give up trying to achieve better portability? Not sure if hanging tests are bound to happen frequently on win32 and not on Linux for example.
There was a problem hiding this comment.
The SIGINT raised by calling thread.interrupt_main without an argument causes the doctest to stop and print "Interrupted!" without a traceback. os.kill just stops the process without any further output.
Which Python version do we want to support and test on? 3.8+ or 3.10+? CI only tests 3.10 and 3.12 right now. I think we can keep the old alarm based interrupt handling for host systems that support it and use this one for others? I think it's better to have some timeout in place even if it potentially doesn't handle all cases. I don't know about IO, but it's an exception from a signal handler similar to the alarm way before.
This allows to selectively run tests only on a single platform. We can add
# doctest: +LINUXcomments to tests that cannot work on Windows and the other way around. I've noticed this pattern in the ctypes docs but couldn't find where they are evaluated. (Those options appear to have been there since the docs were initially imported into the repo for Python 2, but tests aren't run using doctest anymore for CPython)To easily skip a lot of tests the
doctest_additional_flagsglobal variable can be defined in atestsetup.This is achieved by monkey patching sphinx doctest's DocTestBuilder to use our own DocTestRunner which removes examples from the tests that have flags that don't match the platform we're running on. It's only implemented for Sphinx 8 on Python 3.
The goal is to reduce the amount of linux-only tests and add tests for the Windows specific features over time.