1333114 - Refactor wpt test runner as a prerequisite for leak checking for web-platform-tests

Status: RESOLVED FIXED

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

Description

[James Graham [:jgraham]]

Because this should have been done some time ago.

One problem is how to deal with tests from upstream that leak on import. We can't delay the import in order to fix them, so we might need to add a metadata mechansim for skipping leak checking during those tests, or similar.

Comment 1

[James Graham [:jgraham]]

Attachment: wip squashed patch

Added the extra bits I had on try to help people trying to reproduce issues.

Comment 2

[James Graham [:jgraham]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53497702c4a5d1ff17229725d5675dd3935318fe&selectedJob=70411056 was the try run

Comment 3

[James Graham [:jgraham]]

Attachment: Bug 1333114 - Update to latest wptrunner, a=testonly

Review commit: https://reviewboard.mozilla.org/r/108264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108264/

Comment 4

[James Graham [:jgraham]]

Attachment: Bug 1333114 - Refactor wpt TestRunner,

In order to support leak checking in web-platform-tests, we have to
support restarting the browser before a test starts, which is
complicated by the architecture of wptrunner that puts the TestRunner
and associated Executor in a separate process. In the previous
architecture the Executor process grabbed the test to run from the
queue so it wasn't possible to restart the browser once the test was
known without considerable difficulty.

The primary goal of this refactor is to give the TestRunner access to
the test before it is passed to the executor. However imlementing this
was complicated by the poor structure of the existing code. To make
things more comprehensible the TestRunner class is factored into a
more explicit state machine, with a run() loop that either transitions
the runner into the next state or waits for an external command which
causes such a state transition.

Review commit: https://reviewboard.mozilla.org/r/108266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108266/

Comment 5

[James Graham [:jgraham]]

Attachment: Bug 1333114 - Enable leak checking for web-platform-tests,

Enable gecko leak checking for web-platform-tests. Allows disabling
leak checks for a specific test using a 'leaks' key in test metadata.

Review commit: https://reviewboard.mozilla.org/r/108268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108268/

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8831721 [details]
Bug 1333114 - Enable leak checking for web-platform-tests,

https://reviewboard.mozilla.org/r/108268/#review109566

::: testing/web-platform/README.md:176
(Diff revision 1)
>                  dom.caches.enabled:true]
>  
> +Disabiling Leak Checks
> +----------------------
> +
> +When a test is imported that leak, it may be necessary to temporarily

"that leaks"

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8831720 [details]
Bug 1333114 - Refactor wpt TestRunner,

https://reviewboard.mozilla.org/r/108266/#review109562

::: testing/web-platform/harness/wptrunner/testrunner.py:114
(Diff revision 1)
> +            self.send_message("error",
> +                              "Timed out when waiting for %s" %
> +                              (command,))
> +            raise
> +
> +        if command != command:

What?

::: testing/web-platform/harness/wptrunner/testrunner.py:216
(Diff revision 1)
> +                succeeded = True
> +                self.started = True
> +
> +        # This has to happen after the lock is released
> +        if not succeeded:
> +            return False

What's the point of this `if`?

Comment 8

[James Graham [:jgraham]]

Comment on attachment 8831720 [details]
Bug 1333114 - Refactor wpt TestRunner,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108266/diff/1-2/

Comment 9

[James Graham [:jgraham]]

Comment on attachment 8831721 [details]
Bug 1333114 - Enable leak checking for web-platform-tests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108268/diff/1-2/

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8831720 [details]
Bug 1333114 - Refactor wpt TestRunner,

https://reviewboard.mozilla.org/r/108266/#review114522

::: testing/web-platform/harness/wptrunner/testrunner.py:54
(Diff revision 2)
>          """Class implementing the main loop for running tests.
>  
>          This class delegates the job of actually running a test to the executor
>          that is passed in.
>  
>          :param test_queue: subprocess.Queue containing the tests to run

Remove

::: testing/web-platform/harness/wptrunner/testrunner.py:613
(Diff revision 2)
>      def error(self, message):
>          self.logger.error(message)
>          self.restart_runner()
>  
> +    def stop_runner(self, force=False):
> +        """Stop the TestRunner and the Firefox binary."""

Is this code specific to Fx?

Comment 11

[James Graham [:jgraham]]

Comment on attachment 8831720 [details]
Bug 1333114 - Refactor wpt TestRunner,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108266/diff/2-3/

Comment 12

[James Graham [:jgraham]]

Comment on attachment 8831720 [details]
Bug 1333114 - Refactor wpt TestRunner,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108266/diff/3-4/

Comment 13

[David Burns :automatedtester]

Comment on attachment 8831720 [details]
Bug 1333114 - Refactor wpt TestRunner,

https://reviewboard.mozilla.org/r/108266/#review124618

Comment 14

[David Burns :automatedtester]

Comment on attachment 8831720 [details]
Bug 1333114 - Refactor wpt TestRunner,

https://reviewboard.mozilla.org/r/108266/#review124620

Comment 15

[David Burns :automatedtester]

Comment on attachment 8831720 [details]
Bug 1333114 - Refactor wpt TestRunner,

https://reviewboard.mozilla.org/r/108266/#review124624

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0e6a0c4712a
Refactor wpt TestRunner, r=automatedtester,Ms2ger

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a0e6a0c4712a

Comment 18

[James Graham [:jgraham]]

I poisoned the well on this bug by using it to land a prerequisite via mozreview. So created a new meta bug for the whole feature and moved the dependencies there.

Comment 19

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.