Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add +LINUX and +WINDOWS doctest options #2507

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

peace-maker
Copy link
Member

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

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
Copy link
Member

@Arusekk Arusekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that I failed to dissuade you from supporting Windows, so be it 😉

signal.alarm(600) # ten minutes
if sys.platform == 'win32':
def alrm_handler():
raise EndlessLoop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really interrupt hanging tests? If so, this should be the default way for all platforms.

Copy link
Member Author

@peace-maker peace-maker Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pwnlib/util/sh_string.py Show resolved Hide resolved
pwnlib/util/packing.py Show resolved Hide resolved
pwnlib/context/__init__.py Outdated Show resolved Hide resolved
pwnlib/context/__init__.py Outdated Show resolved Hide resolved
docs/source/conf.py Show resolved Hide resolved
@peace-maker
Copy link
Member Author

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.

@Arusekk
Copy link
Member

Arusekk commented Dec 18, 2024

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants