Commit 2983fc67 authored by Chris Lamb's avatar Chris Lamb 💬

Treat missing tools on Debian autopkgtests as individual test failures.

parent 50a9572b
Pipeline #44333 passed with stage
in 15 minutes and 28 seconds
......@@ -8,6 +8,7 @@ if ! [ -d "$ADTTMP" ]; then
fi
export LIBGUESTFS_MEMSIZE=128
export DIFFOSCOPE_TESTS_FAIL_ON_MISSING_TOOLS=1
cp -r tests $ADTTMP
(cd $ADTTMP; py.test-3 -vv -l -r a)
  • @lamby I haven't actually checked the current set of recommends, but will this change not lead to failures when a recommends is only available in unstable, but not in testing? The way we run autopkgtest on ci.d.n integrated with britney has only testing available during the reference run, while it has both testing and unstable available during the regression run.

    In other words, are all potential tools you require actually available in testing?

  • Hm, I had not considered this. I'm not sure how to handle this outside of "are we testing testing" conditional however — suggestions? (Is this even possible without hacks?)

  • I wonder if it isn't even worse, if I remember correctly , diffoscope supported using tools not even in unstable, right? That would make the autopkgtest always fail.

  • Unless I'm misunderstanding, wasn't failing on missing tools the entire point here? (Of course, we could "whitelist" some ones that will never be fixed)

  • I'm not sure that's much relevant now in the context of autopkgtest since the run requiring all tools have all the tools listed as dependencies. Could anybody remind me what this change was trying to fix?

    I actually fear this change already leads to failed autopkgtest, since the test that is supposed to run without any extra tool now requires those tools...

  • remind me what this change was trying to fix?

    https://bugs.debian.org/905885

  • I ran 114 in unstable. It fails: https://ci.debian.net/packages/d/diffoscope/unstable/amd64/

    With lots of messages like msg = 'requires zipinfo (DIFFOSCOPE_TESTS_FAIL_ON_MISSING_TOOLS=1)'

  • @mattia Any insight? I'm also seeing that we are missing black too. This is, of course, not a package "Recommended" by the diffoscope binary package but rather a nocheck build-depends. Shouldn't the autopkgtest control file include this? (Pinging you as you added and worked on this mechanism)

    In case it helps, the control file in question is https://salsa.debian.org/reproducible-builds/diffoscope/blob/master/debian/tests/control

  • So, I added black to the test dependencies (0eddfab7) and had the autopkgtest export that variable only when running with all the recommends (2e111824).

    Now, I was evaluating how to do something about the packages that we know are missing. I was considering adding a cli option to the tests, but I'm having a lot of troubles accessing the value from that object from the tests.utils.tools.skipif() function, which seems to be too nested to be able to decently access the fixtures. So, I'll now do it through another environment variable instead.

  • for example:

    diff --git a/tests/utils/tools.py b/tests/utils/tools.py
    index d5beea6..d979ced 100644
    --- a/tests/utils/tools.py
    +++ b/tests/utils/tools.py
    @@ -54,6 +54,10 @@ def skipif(*args, **kwargs):
         if os.environ.get('DIFFOSCOPE_TESTS_FAIL_ON_MISSING_TOOLS', None) != '1':
             return pytest.mark.skipif(*args, **kwargs)
     
    +    missing_tools = os.environ.get('DIFFOSCOPE_TESTS_MISSING_TOOLS', '').split()
    +    if any(t for t in kwargs.get('tools', []) if t in missing_tools):
    +        return pytest.mark.skipif(*args, **kwargs)
    +
         msg = "{} (DIFFOSCOPE_TESTS_FAIL_ON_MISSING_TOOLS=1)".format(
             kwargs['reason']
         )
    @@ -74,12 +78,13 @@ def skip_unless_tools_exist(*required):
         return skipif(
             tools_missing(*required),
             reason="requires {}".format(" and ".join(required)),
    +        tools=[*required],
         )
     
     
     def skip_if_tool_version_is(tool, actual_ver, target_ver, vcls=LooseVersion):
         if tools_missing(tool):
    -        return skipif(True, reason="requires {}".format(tool))
    +        return skipif(True, reason="requires {}".format(tool), tools=[tool])
         if callable(actual_ver):
             actual_ver = actual_ver()
         return skipif(
    @@ -87,12 +92,13 @@ def skip_if_tool_version_is(tool, actual_ver, target_ver, vcls=LooseVersion):
             reason="requires {} != {} ({} detected)".format(
                 tool, target_ver, actual_ver
             ),
    +        tools=[tool],
         )
     
     
     def skip_unless_tool_is_at_least(tool, actual_ver, min_ver, vcls=LooseVersion):
         if tools_missing(tool) and module_is_not_importable(tool):
    -        return skipif(True, reason="requires {}".format(tool))
    +        return skipif(True, reason="requires {}".format(tool), tools=[tool])
         if callable(actual_ver):
             actual_ver = actual_ver()
         return skipif(
    @@ -100,12 +106,13 @@ def skip_unless_tool_is_at_least(tool, actual_ver, min_ver, vcls=LooseVersion):
             reason="requires {} >= {} ({} detected)".format(
                 tool, min_ver, actual_ver
             ),
    +        tools=[tool],
         )
     
     
     def skip_unless_tool_is_at_most(tool, actual_ver, max_ver, vcls=LooseVersion):
         if tools_missing(tool) and module_is_not_importable(tool):
    -        return skipif(True, reason="requires {}".format(tool))
    +        return skipif(True, reason="requires {}".format(tool), tools=[tool])
         if callable(actual_ver):
             actual_ver = actual_ver()
         return skipif(
    @@ -113,6 +120,7 @@ def skip_unless_tool_is_at_most(tool, actual_ver, max_ver, vcls=LooseVersion):
             reason="requires {} <= {} ({} detected)".format(
                 tool, max_ver, actual_ver
             ),
    +        tools=[tool],
         )
     
     
    @@ -120,7 +128,7 @@ def skip_unless_tool_is_between(
         tool, actual_ver, min_ver, max_ver, vcls=LooseVersion
     ):
         if tools_missing(tool):
    -        return skipif(True, reason="requires {}".format(tool))
    +        return skipif(True, reason="requires {}".format(tool), tools=[tool])
         if callable(actual_ver):
             actual_ver = actual_ver()
         return skipif(
    @@ -129,6 +137,7 @@ def skip_unless_tool_is_between(
             reason="requires {} >= {} >= {} ({} detected)".format(
                 min_ver, tool, max_ver, actual_ver
             ),
    +        tools=[tool],
         )
     
     
    @@ -139,6 +148,7 @@ def skip_if_binutils_does_not_support_x86():
         return skipif(
             'elf64-x86-64' not in get_supported_elf_formats(),
             reason="requires a binutils capable of reading x86-64 binaries",
    +        tools=['objdump'],
         )
     
     

    then we could export DIFFOSCOPE_TESTS_MISSING_TOOLS="cbfstool procyon otool lipo oggDump wasm2wat"

  • Great stuff. And defining which tools are missing is certainly more appropriately handled under debian/ rather than in the codebase itself as it is, of course, distribution-specific. Nice one!

  • @lamby What do you think about the above diff? Please share your opinions on it, and whatever you think it could be done better.

  • Apart from some minor style things (eg. using x as anonymous variables, not t despite "tool") and preferring tuples (eg. tools=('objdump',)) for read-only vars, then — without testing it myself — I am a fan. :)

  • I also noticed just now, that currently, all the test somehow using @skip_unless... are failing, because using DIFFOSCOPE_TESTS_FAIL_ON_MISSING_TOOLS cuases pytest.fail to be called unconditionally, instead of doing it depending on the condition…

    i.e.

    @@ -69,7 +70,8 @@ def skipif(*args, **kwargs):
         # executed.
         def outer(*args1, **kwargs1):
             def inner(*args2, **kwargs2):
    -            return pytest.fail(msg)
    +            if args[0]:
    +                return pytest.fail(msg)
     
             return inner
     
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment