[WIP] Selectively skip doctest #1251

timokau · 2019-01-20T13:29:55Z

The intention of this is to make it possible to test pwntools on systems where some functionality is known not to work. This makes use of a new sphinx feature to selectively disable doctests. In particular, doctests that require binutils/qemu for a specific architecture, doctests that require internet access and doctests that depend on specific machine setup (marked travis) can be skipped.

This is a work in progress. Currently all flags are hardcoded as False, instead they should default to True and be changeable through flags or environment variables. But it should be ready for general feedback on the main idea. Is this something you'd in principle be willing to consider @zachriggle?

My use-case for this is automated testing for distribution packaging. Another usecase would be a possibility for contributers to run at least some of the testsuite locally without having the full setup that is available on travis.

There seems to be no reason for this and it breaks the testsuite with a never gdb version. The lower limit may have a reason (I haven't tested) and suffices as a test for the version function.

Introduce binutils_arch and qemu_arch options to set weather or not the system can analyze/build and execute binaries for arch. Also introduces the `travis` option for various assumptions that are true on travis.

zachriggle · 2019-01-21T04:55:43Z

This is pretty neat! One recommendation that I might suggest is that there should not be a need to modify every single shellcraft/**/foo.py to have this block.

We should be able to add a conditional to the .rst files in docs/ that achieve the same affect without being so very verbose (and prone to future maintenance errors).

Separately, if it's at all possible I would prefer to have dicts instead of multiple snake_case variables. For example skip if test_qemu['arm'] instead of skip if not qemu_arm. This should let us be more flexible and set module-level variables to be used, e.g. in arm.rst we can set:

skip_binutils = skip_binutils or not have_binutils['arm']

This would allow us to use skip_binutils everywhere, instead of having per-architecture details spread throughout the test metadata. It would just be set to the "right" value by the .rst file.

zachriggle · 2019-01-21T05:02:47Z

Following up further, it may also be useful to create our own doctest_xyz Sphinx directive / class / decorator / whatever the correct term is.

This would allow us to have more streamlined docs, e.g.

.. doctest::
   :skipif: not binutils_arm

vs

.. doctest_binutils("arm"):
# or if we use a dict as per above
.. doctest_binutils['arm']:
# or, possibly if we can auto-detect the architecture from a module-level variable
.. doctest_binutils:

Finally, in order to accept this pull request I think we need an update to the Travis CI test matrix / test setup stuff which actually ensures that all of these configurations bits work. We don't need every combination, but at least the following:

No foreign-architecture binutils at all
Each individual binutils foreign architecture
All binutils architecture but no qemu
All qemu architecture but no binutils

zachriggle · 2019-01-21T05:06:36Z

One very final request would be that we should add a standard way for end-users of pwntools (and our own internal use) to determine at-runtime what toolchains are available.

For example, pwnlib.asm.supported(arch=None) will return whether we are able to assemble code for the provided (or, default to context.arch) architecture.

timokau · 2019-01-21T08:57:15Z

This is pretty neat!

I'm great there is a positive general sentiment :)

One recommendation that I might suggest is that there should not be a need to modify every single shellcraft/**/foo.py to have this block.

I agree, that also felt a little wrong while doing it. It does have the advantage of explicitness and consistency, but that is maybe not worth it.

We should be able to add a conditional to the .rst files in docs/ that achieve the same affect without being so very verbose (and prone to future maintenance errors).

I wasn't sure if that is possible (my experience with sphinx is very limited). Not sure how :skipif: works together with automodule.

Separately, if it's at all possible I would prefer to have dicts instead of multiple snake_case variables. For example skip if test_qemu['arm'] instead of skip if not qemu_arm.

That should be a trivial change.

This should let us be more flexible and set module-level variables to be used, e.g. in arm.rst we can set:
skip_binutils = skip_binutils or not have_binutils['arm']
This would allow us to use skip_binutils everywhere, instead of having per-architecture details spread throughout the test metadata. It would just be set to the "right" value by the .rst file.

I'm not sure about that one. I see little benefit of skip_binutils vs. test_binutils['arm']. skip_binutils wouldn't work in every case (since there may be a single test using mips in the arm file, other files may not have a sensible default at all) so this only increases the amount of stuff a developer has to remember. I don't feel strongly about this however.

timokau · 2019-01-21T08:59:44Z

Following up further, it may also be useful to create our own doctest_xyz Sphinx directive / class / decorator / whatever the correct term is.

That may be neat, I didn't know that was possible. I don't think .. doctests_binutils['arm'] vs. .. doctest:: :skipif no test_binutils['arm'] make that much of a difference though. I'm fine with either.

timokau · 2019-01-21T09:00:30Z

Also having it in the test matrix would be amazing! Is there no problem with compute resources if most tests need to be run 4 times?

timokau · 2019-01-21T09:01:50Z

One very final request would be that we should add a standard way for end-users of pwntools (and our own internal use) to determine at-runtime what toolchains are available.

For example, pwnlib.asm.supported(arch=None) will return whether we are able to assemble code for the provided (or, default to context.arch) architecture.

That could basically reduce to whatever context.arch already does when I set it to an invalid architecture right? Not sure if that is in-scope of this PR though.

zachriggle · 2019-01-21T09:16:09Z

One very final request would be that we should add a standard way for end-users of pwntools (and our own internal use) to determine at-runtime what toolchains are available.
For example, pwnlib.asm.supported(arch=None) will return whether we are able to assemble code for the provided (or, default to context.arch) architecture.

That could basically reduce to whatever context.arch already does when I set it to an invalid architecture right? Not sure if that is in-scope of this PR though.

You can set context.arch to any valid value, regardless of whether there is (1) a toolchain to assemble shellcode or (2) QEMU support for running binaries of that architecture.

zachriggle · 2019-01-21T09:17:04Z

Also having it in the test matrix would be amazing! Is there no problem with compute resources if most tests need to be run 4 times?

It's not a huge deal as long as it's restricted to the stable and beta branches. Doing it on dev would indeed take far too long.

timokau · 2019-01-21T11:18:24Z

You can set context.arch to any valid value, regardless of whether there is (1) a toolchain to assemble shellcode or (2) QEMU support for running binaries of that architecture.

Oh, I thought that would check for a valid binutils. But I just re-tried. Must have misremembered.

timokau · 2019-01-21T22:25:47Z

After quite some digging into sphinx, the only way to skip the shellcraft tests without touching every file is skipping them completely (when in doctest mode) by (ab-)using autodoc's autodoc-skip-member function.

Feels pretty hacky. What do you think? Did you have something specific in mind when you made that suggestion?

zachriggle · 2019-01-22T18:25:09Z

docs/source/intro.rst

@@ -199,7 +211,7 @@ ELF Manipulation

 Stop hard-coding things!  Look them up at runtime with :mod:`pwnlib.elf`.

-    >>> e = ELF('/bin/cat')
+    >>> e = ELF('/bin/cat') # doctest: +SKIP


Why are we skipping this?

Because all the follow-up tests were already skipped.

zachriggle · 2019-01-22T18:25:21Z

docs/source/intro.rst

@@ -211,6 +223,9 @@ Stop hard-coding things!  Look them up at runtime with :mod:`pwnlib.elf`.

 You can even patch and save the files.

+ .. doctest::
+    :skipif: not travis


Why would we skip these if we're not on Travis?

zachriggle · 2019-01-22T18:26:03Z

pwnlib/context/__init__.py

@@ -192,6 +192,9 @@ class Thread(threading.Thread):

    Examples:

+    .. doctest::


There is no need to skip this. We're not actually using binutils here.

zachriggle · 2019-01-22T18:27:07Z

pwnlib/context/__init__.py

@@ -287,6 +290,9 @@ class ContextType(object):

    Examples:

+    .. doctest::


I believe that this will break the formatting under "Examples". Have you looked at the output of make -C docs html to see if the output changes or additional formatting warnings are generated?

I haven't yet. According to sphinx doc,

Some text >>> print("Inline example") Inline example

Is equivalent to

Some text .. doctest:: >>> print("some inline example") some inline example

zachriggle · 2019-01-22T18:28:44Z

pwnlib/fmtstr.py

+Examples: (FIXME)
+
+.. doctest::
+   :skipif: True


Why are we skipping this? Is the test broken?

zachriggle · 2019-01-22T18:30:30Z

pwnlib/qemu.py

@@ -90,6 +90,9 @@ def archname():
    Returns the name which QEMU uses for the currently selected
    architecture.

+.. doctest::


Why are we skipping this test here? And only for powerpc? It's doing a dict lookup.

zachriggle · 2019-01-22T18:30:35Z

pwnlib/qemu.py

    >>> pwnlib.qemu.user_path(arch='thumb')
-    'qemu-arm-static'


zachriggle · 2019-01-22T18:33:16Z

pwnlib/rop/rop.py

@@ -398,6 +413,9 @@ class ROP(object):
    0x0078:             0x2b ss
    0x007c:              0x0 fpstate

+.. doctest::


Why have you broken this into multiple doctest blocks?

They are all part of the same example, AND they all have the same requirements. Separately, binutils_i386 and qemu_i386 should not exist at all. We always assume that Pwntools is running from an amd64 machine which has binutils installed, and can execute i386 binaries natively.

amd64 is the ONLY supported architecture to run Pwntools from.

It already was multiple doctest blocks. Empty lines are interpreted as a block separator.

zachriggle · 2019-01-22T18:33:56Z

docs/source/conf.py

+binutils_thumb=False
+qemu_mips=False
+qemu_arm=False
+qemu_amd64=False


All instances of {qemu,binutils}_{i386,amd64} should be removed entirely. These are always available. If they are not available, you're running Pwntools from an unsupported machine type and the tests are not expected to work.

They are not always available. The intention is to be able to verify that basic pwntools functionality works as intended, even if for example no mips binutils are available.

Are multiple people using this account? I feel like your comments contradict your previous sentiment.

Please re-read my comment. It does not refer to MIPS binutils, but specifically the i386 and amd64 binutils and QEMU.

No valid testing environment exists where you cannot execute i386 and amd64 binaries, as a requirement for using Pwntools is to use amd64 as the host architecture. Other architectures may work, but it is not worth the support burden to the maintenance team to ensure they work.

Separately, in a testing environment, why is the MIPS toolchain not available? We have the ability to install any packages we want.

This is my personal account. I am currently the only Pwntools maintainer.

zachriggle · 2019-01-22T18:35:14Z

docs/source/conf.py

+qemu_aarch64=False
+qemu_powerpc=False
+qemu_thumb=False
+travis=False


I don't understand the intent of not travis. The only supported environments for running the Pwntools tests are:

1.) From the included Docker image, which sets up the environment similar to Travis. See travis/Docker or just run it: make -C travis/docker ANDROID=no.
2.) On Travis itself.

It is poorly named. It is used when certain binaries are expected to be in certain locations. It should be named not fhs or something similar instead.

When will e.g. /bin/sh not exist at /bin/sh? There are lots of situations that I can think of, but none for which we should be bending our tests.

zachriggle · 2019-01-22T18:36:53Z

Overall I disagree with the ~~intent~~ implementation of this Pull Request. See TESTING.md and travis/Docker/readme.md for the correct way to run tests not-on-Travis CI.

Separately, the support burden is pretty high here -- much higher than just using Docker to run the tests.

Finally, you can run individual module tests by specifying them in the same way we do for the Docker image. Again, see travis/Docker for more information.

If there is interest in supporting vendor packaging without using Travis, I recommend creating a Dockerfile which has all of the appropriate dependencies in the same way as travis/Docker. This will allow you to perform whatever setup you need, without pushing the support burden onto the Pwntools maintenance team.

timokau · 2019-01-22T18:54:03Z

I'm surprised. What happened in between

This is pretty neat!

and

Overall I disagree with the intent implementation of this Pull Request. See TESTING.md and travis/Docker/readme.md for the correct way to run tests not-on-Travis CI.

?

timokau · 2019-01-22T18:56:19Z

Separately, the support burden is pretty high here -- much higher than just using Docker to run the tests.

That goes against the point of testing weather or not pwntools works in a particular environment though. It also won't solve the networking issue.

timokau · 2019-01-22T18:58:26Z

Regarding (nearly) all of your inline comments: This was not ready for detailed review. I posted a WIP to get general feedback before investing more time into this. Lots of hacky parts to get the distro package to test successfully.

zachriggle · 2019-01-23T19:53:22Z

I'm surprised. What happened in between

This is pretty neat!

and

Overall I disagree with the intent implementation of this Pull Request. See TESTING.md and travis/Docker/readme.md for the correct way to run tests not-on-Travis CI.

?

I re-read the reason for the pull request -- which is to support testing on some environment that isn't a Ubuntu LTS release:

My use-case for this is automated testing for distribution packaging.

My initial hot take was to support running tests in an environment where e.g. qemu-user is simply not possible (e.g. on Darwin / macOS).

For any Linux distribution, it is infinitely easier to install all of the appropriate binutils and QEMU, and ensure network connectivity -- than to force a Python package's test infrastructure to support the self-elected limitations of the testing environment.

Neat!: Selectively disabling tests for where they're impossible to run
Burden: Making tests more complicated in order to support some restricted testing environment.

Another usecase would be a possibility for contributers to run at least some of the testsuite locally without having the full setup that is available on travis.

See TESTING.md.

timokau · 2019-04-18T20:22:33Z

I know this has been a while, but anyways.

I re-read the reason for the pull request -- which is to support testing on some environment that isn't a Ubuntu LTS release:

I just wanted to say that I don't think you handled that optimally. I intentionally posted a very early version first to ask for feedback. I did that because I wasn't sure if the work would be welcome upstream. After very positive feedback, I invested a lot of time into improving this PR, just for it to suddenly get shot down. That doesn't make me enjoy contributing very much.

I know this probably wasn't your intention, which is why I'm providing feedback for the future :)

For any Linux distribution, it is infinitely easier to install all of the appropriate binutils and QEMU, and ensure network connectivity -- than to force a Python package's test infrastructure to support the self-elected limitations of the testing environment.

I don't agree with this. First the point of distro testing is making sure that the version of the package the user actually installs is working as expected. That will usually not include all binutils & QEMU dependencies. Second internet based tests are always problematic, as they may fail for various reasons and depend on external services.

I won't push this further, just giving my opinion.

timokau added 3 commits January 20, 2019 10:29

Don't test the upper gdb version limit

55f1565

There seems to be no reason for this and it breaks the testsuite with a never gdb version. The lower limit may have a reason (I haven't tested) and suffices as a test for the version function.

Mark internet tests as such

b2150c7

Various fixes

4c76c45

Introduce binutils_arch and qemu_arch options to set weather or not the system can analyze/build and execute binaries for arch. Also introduces the `travis` option for various assumptions that are true on travis.

timokau mentioned this pull request Jan 20, 2019

Selectively disable doctests #1250

Closed

zachriggle requested changes Jan 22, 2019

View reviewed changes

zachriggle closed this Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Selectively skip doctest #1251

[WIP] Selectively skip doctest #1251

timokau commented Jan 20, 2019

zachriggle commented Jan 21, 2019

zachriggle commented Jan 21, 2019 •

edited

zachriggle commented Jan 21, 2019

timokau commented Jan 21, 2019

timokau commented Jan 21, 2019

timokau commented Jan 21, 2019

timokau commented Jan 21, 2019

zachriggle commented Jan 21, 2019

zachriggle commented Jan 21, 2019

timokau commented Jan 21, 2019

timokau commented Jan 21, 2019

zachriggle Jan 22, 2019

timokau Jan 22, 2019

zachriggle Jan 22, 2019

zachriggle Jan 22, 2019

zachriggle Jan 22, 2019

timokau Jan 22, 2019

zachriggle Jan 22, 2019

zachriggle Jan 22, 2019

zachriggle Jan 22, 2019

zachriggle Jan 22, 2019

timokau Jan 22, 2019

zachriggle Jan 22, 2019

timokau Jan 22, 2019

zachriggle Jan 23, 2019

zachriggle Jan 22, 2019

timokau Jan 22, 2019

zachriggle Jan 23, 2019 •

edited

zachriggle commented Jan 22, 2019 •

edited

timokau commented Jan 22, 2019

timokau commented Jan 22, 2019

timokau commented Jan 22, 2019

zachriggle commented Jan 23, 2019 •

edited

timokau commented Apr 18, 2019

		@@ -192,6 +192,9 @@ class Thread(threading.Thread):

		Examples:

		.. doctest::

		@@ -287,6 +290,9 @@ class ContextType(object):

		Examples:

		.. doctest::

[WIP] Selectively skip doctest #1251

[WIP] Selectively skip doctest #1251

Conversation

timokau commented Jan 20, 2019

zachriggle commented Jan 21, 2019

zachriggle commented Jan 21, 2019 • edited

zachriggle commented Jan 21, 2019

timokau commented Jan 21, 2019

timokau commented Jan 21, 2019

timokau commented Jan 21, 2019

timokau commented Jan 21, 2019

zachriggle commented Jan 21, 2019

zachriggle commented Jan 21, 2019

timokau commented Jan 21, 2019

timokau commented Jan 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachriggle Jan 23, 2019 • edited

Choose a reason for hiding this comment

zachriggle commented Jan 22, 2019 • edited

timokau commented Jan 22, 2019

timokau commented Jan 22, 2019

timokau commented Jan 22, 2019

zachriggle commented Jan 23, 2019 • edited

timokau commented Apr 18, 2019

zachriggle commented Jan 21, 2019 •

edited

zachriggle Jan 23, 2019 •

edited

zachriggle commented Jan 22, 2019 •

edited

zachriggle commented Jan 23, 2019 •

edited