1579864 - "./mach wpt" returns wrong exit code and breaks bisection

Status: RESOLVED FIXED

(Testing :: web-platform-tests, defect, P1)

Description

[Andreas Tolfsen ❲:ato❳]

./mach wpt negates the value returned by wptrunner and causes
successful test runs to exit with a non-zero exit code. This breaks
bisection with git-bisect(1) because it relies on the exit code to
tell if the tests pass:

% git bisect start HEAD 58f77b759484bcee720024a1af108bbe52fa4290
% git bisect run ./mach wpt --headless testing/web-platform/tests/webdriver/tests/accept_alert/accept.py
[lots of chatter]
0ed9313abb4c37947a9b6fd33acd304b738748c5 is the first bad commit

The commit it claims is the first bad commit is in fact the head
of the branch, and I verified manually it is not the commit that
caused the test failure. This means unsuccessful test runs are
wrongly reported as successful, and vice versa.

wptrunner.start() negates the return value of wptrunner.run_tests():

def run_tests(…):
    …
    return unexpected_total == 0

def start(…):
    …
        return not run_tests(**kwargs)

The returned boolean is then negated again and coerced to an integer
in WebPlatformTestsRunner:

class WebPlatformTestsRunner(object):
    …

    def run(…):
        …
        result = wptrunner.start(**kwargs)
        return int(not result)

Before it is finally returned by MachCommands.run_web_platform_tests() to mach:

class MachCommands(MachCommandBase):
    def run_web_platform_tests(…):
        …
        return wpt_runner.run(logger, **params)

I’m presuming mach calls sys.exit() or eventually makes it the
return value of __main__. In either case it doesn’t matter since
we are dealing with an integer at this point, which means we are
not subject to the boolean-to-integer coercion that happens with
the return value from main functions.

To summarise:

>>> unexpected_total = 0
>>> run_tests_rv = unexpected_total == 0
>>> start_rv = not run_tests_rv
>>> run_rv = int(not start_rv)
>>> run_rv
1

As none of the return values are documented it’s hard to say what
the correct behaviour should be. I personally think it would be
better if wptrunner returned a structured dict of the results instead
of implying an exit code for the CLI implementation, but without
changing the API there is arguably a bug with
WebPlatformTestsRunner.run(): if we assume wptrunner.start()
is correct to return false on success, run() should not try to
negate that before coercing it to an integer:

diff --git a/testing/web-platform/mach_commands_base.py b/testing/web-platform/mach_commands_base.py
index 830c4614166a..bd6f5b89d954 100644
--- a/testing/web-platform/mach_commands_base.py
+++ b/testing/web-platform/mach_commands_base.py
@@ -38,7 +38,7 @@ class WebPlatformTestsRunner(object):
         else:
             raise ValueError("Unknown product %s" % kwargs["product"])
         result = wptrunner.start(**kwargs)
-        return int(not result)
+        return int(result)
 
     def update_manifest(self, logger, **kwargs):
         import manifestupdate

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1579864: wpt: fix negation of wptrunner success indicator;

wptrunner.start() negates the return value of wptrunner.run() so
that false is used to indicate that the test run was successful.
This is then (wrongly) negated again by WebPlatformTestsRunner.run()
and coerced into an integer.

Quick demonstration:

>>> unexpected_total = 0
>>> run_tests_rv = unexpected_total == 0
>>> start_rv = not run_tests_rv
>>> run_rv = int(not start_rv)
>>> run_rv
1

This causes git-bisect(1) (specifically "git bisect run ./mach wpt")
to not work because it relies on the exit code to tell if a commit
is good or bad.

This patch offers the least invasive fix for the problem by not
altering the API of the wptrunner test harness.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/608ae1c5038e
wpt: fix negation of wptrunner success indicator; r=jgraham

Comment 3

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/608ae1c5038e