961138 - Land initial iteration of mozharness for web-platform-tests

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[James Graham [:jgraham]]

web-platform-tests has a new mozharness script which needs to land so that I can test the actual harness on cedar.

Comment 1

[James Graham [:jgraham]]

Attachment: 961138.diff

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8361829 [details] [diff] [review]
961138.diff

Review of attachment 8361829 [details] [diff] [review]:
-----------------------------------------------------------------

We'll eventually need one or more production config files as well, depending on which os's this will run on.

::: configs/web_platform_tests/test_config.py
@@ +10,5 @@
> +    "options": [
> +        "--metadata-root=%(test_path)s/metadata"
> +    ]
> +}
> +

This file should contain a default_actions list which omits 'read-buildbot-config'

::: scripts/web_platform_tests.py
@@ +11,5 @@
> +import shutil
> +import glob
> +import json
> +import time
> +import logging

nit: lots of unused imports here, and below in the mozharness section

@@ +34,5 @@
> +
> +class WebPlatformTest(TestingMixin, MercurialScript, BlobUploadMixin):
> +    config_options = copy.deepcopy(testing_config_options) + \
> +                     copy.deepcopy(blobupload_config_options)
> +    def __init__(self, require_config_file=True):

nit: need a newline before __init__

@@ +50,5 @@
> +            require_config_file=require_config_file,
> +            config={'require_test_zip': True})
> +        #Surely this should be in the superclass
> +        c = self.config
> +        self.installer_url = c.get('installer_url')

Yes, likely this should be in a superclass; feel free to raise a bug.

@@ +166,5 @@
> +        tbpl_status, log_level = parser.evaluate_parser(return_code)
> +
> +        self.buildbot_status(tbpl_status, level=log_level)
> +
> +class StructuredFormatter(object):

Cool, does it make sense to promote this to its own file, so other scripts that use structured logging can make use of it eventually?

@@ +218,5 @@
> +            return "TEST-END %s | %s | took %ims" % (
> +                data["status"], self.id_str(data["test"]), time)
> +
> +    def format_suite_end(self, data):
> +        start_time = self.test_start_times.pop(None)

nit: None is superfluous here.

@@ +298,5 @@
> +
> +# main {{{1
> +if __name__ == '__main__':
> +    web_platform_tests = WebPlatformTest()
> +    web_platform_tests.run_and_exit()

This looks like obsolete testing code which should be removed.

Comment 3

[James Graham [:jgraham]]

> @@ +166,5 @@
> > +        tbpl_status, log_level = parser.evaluate_parser(return_code)
> > +
> > +        self.buildbot_status(tbpl_status, level=log_level)
> > +
> > +class StructuredFormatter(object):
> 
> Cool, does it make sense to promote this to its own file, so other scripts
> that use structured logging can make use of it eventually?

Yeah, I think that should happen in bug 956738 which I need to rebase the web-platform-test work on top of. But I am reluctant to block everything on that bug being finished, so I think it makes sense to leave this in a temporary location for now.

> 
> @@ +218,5 @@
> > +            return "TEST-END %s | %s | took %ims" % (
> > +                data["status"], self.id_str(data["test"]), time)
> > +
> > +    def format_suite_end(self, data):
> > +        start_time = self.test_start_times.pop(None)
> 
> nit: None is superfluous here.

No, it isn't although perhaps that means the code isn't obvious enough. 'None' is used as a sentinel in the start times dictionary to indicate the suite start time. pop(None) returns this value. pop() would raise a TypeError.

I'll change it to use a separate attribute.

> @@ +298,5 @@
> > +
> > +# main {{{1
> > +if __name__ == '__main__':
> > +    web_platform_tests = WebPlatformTest()
> > +    web_platform_tests.run_and_exit()
> 
> This looks like obsolete testing code which should be removed.

All the other scripts (or at least the ones I looked at) seem to have this code?

Comment 4

[James Graham [:jgraham]]

Attachment: 961138.diff

Updated for review comments.

Comment 5

[Jonathan Griffin (:jgriffin)]

(In reply to James Graham [:jgraham] from comment #3)

> 
> > @@ +298,5 @@
> > > +
> > > +# main {{{1
> > > +if __name__ == '__main__':
> > > +    web_platform_tests = WebPlatformTest()
> > > +    web_platform_tests.run_and_exit()
> > 
> > This looks like obsolete testing code which should be removed.
> 
> All the other scripts (or at least the ones I looked at) seem to have this
> code?

Ha, you're right!  Sorry, it is needed.

Comment 6

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8362538 [details] [diff] [review]
961138.diff

Review of attachment 8362538 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks!  In order to work, the prod_config.py file will also need the following sections from http://hg.mozilla.org/build/mozharness/file/e49a766f7e5b/configs/marionette/prod_config.py:  exes, find_links, pip_index, and buildbot_json_path.  That will result in a config file that can be used in production on OSX and Linux.  For Windows, you'd need similar values from http://hg.mozilla.org/build/mozharness/file/e49a766f7e5b/configs/marionette/windows_config.py, plus the virtualenv fields.

Comment 7

[James Graham [:jgraham]]

Attachment: 961138.diff

Copy across some more production configuration details.

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8363080 [details] [diff] [review]
961138.diff

Review of attachment 8363080 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 9

[Aki Sasaki (not active)]

I think you're missing an |import os|:

### Running scripts/configtest.py --log-level warning
16:06:17    ERROR - /src/fota/mozharness/scripts/../configs/web_platform_tests/prod_config.py is invalid python.
16:06:17    ERROR - NameError("name 'os' is not defined",)
16:06:17    ERROR - /src/fota/mozharness/scripts/../configs/web_platform_tests/prod_config.py is invalid python.

Comment 10

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #9)
> I think you're missing an |import os|:
> 
> ### Running scripts/configtest.py --log-level warning
> 16:06:17    ERROR -
> /src/fota/mozharness/scripts/../configs/web_platform_tests/prod_config.py is
> invalid python.
> 16:06:17    ERROR - NameError("name 'os' is not defined",)
> 16:06:17    ERROR -
> /src/fota/mozharness/scripts/../configs/web_platform_tests/prod_config.py is
> invalid python.

fixed https://hg.mozilla.org/build/mozharness/rev/6488361632a8

Comment 11

[Jordan Lund (:jlund)]

a mozharness patch is in production! :)