web-platform-tests: Create a test runner for web-platform-tests suite

RESOLVED FIXED in mozilla35

Status

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: jgraham, Assigned: jgraham)

Tracking

(Depends on 3 bugs)

unspecified
mozilla35
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 16 obsolete attachments)

23.81 KB, patch
Details | Diff | Splinter Review
Assignee

Description

6 years ago
Create a marionette-based test runner for the web-platform-tests testsuite.
Assignee

Comment 1

6 years ago
I have a largely working patch for this, which I will link shortly.
Assignee

Updated

6 years ago
Blocks: 945221
Assignee

Updated

6 years ago
Summary: web-platform-tests: Create a test runner for web-platfoem-tests suite → web-platform-tests: Create a test runner for web-platform-tests suite
Assignee

Comment 2

6 years ago
Added mq patch repo to bitbucket temporarily whilst I wait for proper commit access to be sorted out (see the url field). 

At this stage |mach web-platform-tests| should work and run all tests, at least on linux. I expect OSX to work too; Windows could be problematic.
Assignee

Comment 3

6 years ago
I'm not sure what the right way to ask for this is, but it would be useful to get super-high-level review to tell me what I need to do so that this works in automation rather than just on local machines. For example the logging is obviously not going to work as-is, but I don't know what the modern approach for that is.
Assignee

Updated

6 years ago
Depends on: structured-logging
To get this working in buildbot/TBPL, you'll need to write a mozharness script for it, see the files in http://hg.mozilla.org/build/mozharness/file/cff6b4fcedb4/scripts as examples.  Mozharness scripts are typically invoked with a config file (always, in the case of TBPL), so you'll need one of those too; see http://hg.mozilla.org/build/mozharness/file/cff6b4fcedb4/configs.

The mozharness script is responsible for parsing the output of the test harness and deciding which lines to highlight as errors in the log, and is also responsible for returning a final status code to TBPL which will cause the run to be flagged as passed, failed, or error.

Bug 916295 isn't a strict blocker for this, since TBPL doesn't currently make use of structured logging.  Treeherder likely will, but that's still in a prototype stage.
Assignee

Comment 5

6 years ago
The reason I marked bug 916295 as a blocker is that this currently doesn't use the standard logging module, but implements the format described in bug 916260. That can change of course, but the current behaviour is very nice; it makes the results really easy to post-process.
Assignee

Comment 6

6 years ago
Should this be a new mozharness script, or extend DesktopUnittest? Since it uses a different result format, and generally doesn't make the same assumptions about the meaning of results, a new script seems more obvious, but I don't know how this is invoked on the buildbot side and whether it is desirable to keep all these testsuites in the same file/class.
I suspect it depends on how much of the code you wind up sharing. The existing suites are shared in there because they have a lot of common functionality: download the build, the test package, unpack them, interpret the results.
What Ted said.  It's probably simpler to write a new script than to try to shoehorn a new suite into DesktopUnittest that needs a bunch of special casing.

There's no cost to using a new script vs an old one when setting up the tests in buildbot.
Assignee

Comment 9

6 years ago
Mozharness patch in https://bitbucket.org/jgraham/mozharness-patches (needs more work, but seems to run the tests at least).

I need to work out how to upload the raw structured logs at least (and how to test that).
Using the machinery from bug 749421 (which isn't enabled on all trees yet), you should be able to just put them in $MOZ_UPLOAD_DIR. There's a whitelist of file extensions that are accepted for upload though, so you may need to check that your filetype is allowed:
https://github.com/catlee/blobber/blob/master/blobber/config.py#L12

See also bug 938745.
Assignee

Comment 11

6 years ago
Updated the mozharness stuff to allow the full log to be uploaded (I think).

Also added some expected data from a run I did to the main patch so that you can see how that works. With this expected data, a subsequent run gave 10 unexpected results, so we are not entirely noise free, but it isn't too bad.
Assignee

Comment 12

6 years ago
Posted patch add_wptrunner.diff (obsolete) — Splinter Review
Added a patch with just the code changes (no test imports or metadata) to get feedback on the approach used.
Attachment #8348120 - Flags: feedback?(ahalberstadt)
Comment on attachment 8348120 [details] [diff] [review]
add_wptrunner.diff

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

Just some random comments. I assume the print/dump statements will go. I'd also like some overview documentation somewhere.

::: testing/web-platform-tests/Makefile.in
@@ +18,5 @@
> +  $(NULL)
> +
> +_DEST_DIR = $(DEPTH)/_tests/web-platform-tests
> +libs:: $(WPT_HARNESS)
> +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)

Build peers won't approve :)

@@ +26,5 @@
> +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)
> +
> +stage-package: PKG_STAGE = $(DIST)/test-package-stage
> +stage-package:
> +	$(NSINSTALL) -D $(PKG_STAGE)/peptest

peptest?

::: testing/web-platform-tests/mach_commands.py
@@ +39,5 @@
> +        self._logger = structuredlog.getOutputLogger("WPT.mach")
> +        MozbuildObject.__init__(self, log_manager, settings, log_manager, topobjdir)
> +
> +    def run_tests(self, **kwargs):
> +        # TODO Bug 794506 remove once mach integrates with virtualenv.

This bug is marked fixed.

::: testing/web-platform-tests/runtests.py
@@ +1,1 @@
> +#!/usr/bin/env python

Don't forget to add mpl headers to all new files

::: testing/web-platform-tests/update.py
@@ +8,5 @@
> +mozbase = os.path.realpath(os.path.join(here, '..', 'mozbase'))
> +
> +deps = []
> +
> +for dep in deps:

I don't understand how this does anything

::: testing/web-platform-tests/wpttests/expected.py
@@ +14,5 @@
> +    return unicode_str.encode("utf8").encode("string_escape")
> +
> +def manifest_tokenizer(f):
> +    comment_chars = "#;"
> +    seperators = ":="

separators

@@ +104,5 @@
> +        if subtest not in self.data:
> +            raise ValueError("Unknown subtest %s" % subtest)
> +
> +        del self.data[subtest][key]
> +        if (subtest not in self.reserved_names and not

'not' at EOL looks weird

::: testing/web-platform-tests/wpttests/testharness.js
@@ +1,1 @@
> +window.wrappedJSObject.timeout_multiplier = 1;

This isn't the testharness.js I know, is it?

@@ +2,5 @@
> +
> +function listener(e) {
> +    if(e.data.type == "complete") {
> +        clearTimeout(timer);
> +        removeEventListener("message", listener); 

Trailing ws

::: testing/web-platform-tests/wpttests/utils.py
@@ +31,5 @@
> +
> +def is_git_root(path):
> +    rv = git("rev-parse", "--show-cdup", repo=path)
> +    print rv
> +    return  rv == "\n"

Note double space

::: testing/web-platform-tests/wpttests/wptrunner.py
@@ +765,5 @@
> +    return manager_group.unexpected_count() == 0
> +
> +
> +def main():
> +    """Main entry point when calling from the command line"""

I don't think we should support that, in general. I'd add a separate python file that's explicitly the entry point for automation, and only deals with arguments that are specifically required for that use case.
Comment on attachment 8348120 [details] [diff] [review]
add_wptrunner.diff

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

At this point I'll only give super high level feedback, though I noticed some minor style nits.

In general I think the approach is good, though some bits seem like they could either use mozbase or be added to mozbase. A general rule of thumb is if you have some code with a clear logical separation and you think it could potentially be used by another harness or tool, add it to mozbase! Some parts also seem needlessly complex to me, though this could just be because I don't understand what the harness is trying to accomplish.

::: testing/web-platform-tests/update.py
@@ +1,5 @@
> +#!/usr/bin/env python
> +import os
> +import sys
> +
> +from wpttests import update

That whole file (wpttests/update.py) looks like a good candidate for a standalone module (written generically to update a variety of external repos). Though maybe in practice it wouldn't be needed.

::: testing/web-platform-tests/wpttests/machlogging.py
@@ +5,5 @@
> +
> +import logging
> +from mach import logging as mach_logging
> +
> +import structuredlog

Would like to use mozlog. I think mach will eventually use mozlog too.

::: testing/web-platform-tests/wpttests/metadata.py
@@ +44,5 @@
> +#  - Delete expected result
> +
> +# Open issues
> +#
> +# - Results from multiple platforms/configurations

In general I think these look better as """docstrings""". Plus there's the added benefit that tools like sphinx can pull them out and auto-generate docs.

::: testing/web-platform-tests/wpttests/update.py
@@ +4,5 @@
> +import uuid
> +import shutil
> +
> +import utils
> +from utils import git, hg

Generally we've been trying to stay away from utility modules. If the methods in there share a common theme, consider creating a specific module for them e.g vcs.py or something. If they are only imported in one place, consider in-lining them in the file that needs them (and underscore prefix them so other modules don't start importing them from there).

::: testing/web-platform-tests/wpttests/wptcommandline.py
@@ +1,3 @@
> +import os
> +
> +import argparse

This will fail if your jobs get scheduled on a slave pool still running python 2.6 (unfortunately they still do exist). We can always fix later if it comes to that though.

::: testing/web-platform-tests/wpttests/wptrunner.py
@@ +18,5 @@
> +import mozprocess
> +from mozprofile.profile import Profile
> +from mozrunner import FirefoxRunner
> +
> +import structuredlog

Pretty sure this is on your roadmap already, but I'd really like it if you used mozlog. Feel free to modify mozlog if there is crucial functionality missing.

@@ +292,5 @@
> +                                         subtest["message"]) for subtest in result["tests"]])
> +
> +
> +class ReftestTestRunner(TestRunner):
> +    def __init__(self, *args, **kwargs):

Why are we creating a second reftest harness? In case you didn't know, the other harness lives in layout/tools/reftest, not under testing for some reason. I was also under the impression that we already do import reftests from somewhere else, though I have no idea if the ones you are importing are the same or not.

::: testing/web-platform-tests/wpttests/wpttest.py
@@ +9,5 @@
> +
> +#These are quite similar to moztest, but slightly different
> +class Result(object):
> +    def __init__(self, status, message, expected=None):
> +        if status not in self.statuses:

Please use https://github.com/mozilla/mozbase/blob/master/moztest/moztest/results.py for result collection. Again feel free to modify it if functionality is missing.
Attachment #8348120 - Flags: feedback?(ahalberstadt) → feedback+
(In reply to Andrew Halberstadt [:ahal] from comment #14)
> This will fail if your jobs get scheduled on a slave pool still running
> python 2.6 (unfortunately they still do exist).

They do?
Flags: needinfo?(jmaher)
I am not aware of any tests running python 2.6.  This has been verified on all desktop and mobile platforms.  Could we be talking about b2g tests?  

ahal, please educate us on this.
Flags: needinfo?(jmaher) → needinfo?(ahalberstadt)
Assignee

Comment 17

6 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #14)
> Comment on attachment 8348120 [details] [diff] [review]
> add_wptrunner.diff
> 
> Review of attachment 8348120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> At this point I'll only give super high level feedback, though I noticed
> some minor style nits.

Thanks for your feedback, it's much appreciated.

> In general I think the approach is good, though some bits seem like they
> could either use mozbase or be added to mozbase. A general rule of thumb is
> if you have some code with a clear logical separation and you think it could
> potentially be used by another harness or tool, add it to mozbase!

Yup. I just haven't separated out those parts yet, because it isn't really clear if my vision for how things should work matches what other people are thinking. I will try and deal with that specific subset of the feedback here.

> ::: testing/web-platform-tests/update.py
> @@ +1,5 @@
> > +#!/usr/bin/env python
> > +import os
> > +import sys
> > +
> > +from wpttests import update
> 
> That whole file (wpttests/update.py) looks like a good candidate for a
> standalone module (written generically to update a variety of external
> repos). Though maybe in practice it wouldn't be needed.

Yeah, so at the moment this is rather specific. I think I would prefer to generalise once we actually have a second use case rather than writing something abstract with only one concrete implementation.

> ::: testing/web-platform-tests/wpttests/machlogging.py
> @@ +5,5 @@
> > +
> > +import logging
> > +from mach import logging as mach_logging
> > +
> > +import structuredlog
> 
> Would like to use mozlog. I think mach will eventually use mozlog too.

So, I think this is at the heart of where I need to understand the long-term vision. Fundamentally, I think my point of view is that "structuredlog" is badly named because what it's doing isn't really "logging". Instead it's the producer side of an event/message stream that is the fundamental output/API of a testing process. 

Although it is of course possible to model the whole thing as a series of log messages but I don't think it makes much sense. For example I don't see how it is useful to try and provide an unique ordering of all event types according to severity in the way that the log module does. It also doesn't seem all that useful to have the tree of loggers that the logging module allows; each test harness should produce exactly one event stream as its output.

So whilst this could probably "use mozlog" in some way, I don't think that's the right direction to go in. Unless there is some compelling advantage of the "logging" paradigm for test-harness produced messages.

> ::: testing/web-platform-tests/wpttests/wptrunner.py
> @@ +18,5 @@
> > +import mozprocess
> > +from mozprofile.profile import Profile
> > +from mozrunner import FirefoxRunner
> > +
> > +import structuredlog
> 
> Pretty sure this is on your roadmap already, but I'd really like it if you
> used mozlog. Feel free to modify mozlog if there is crucial functionality
> missing.

See above. I'm not sure why basing this on the logging module makes sense.
 
> @@ +292,5 @@
> > +                                         subtest["message"]) for subtest in result["tests"]])
> > +
> > +
> > +class ReftestTestRunner(TestRunner):
> > +    def __init__(self, *args, **kwargs):
> 
> Why are we creating a second reftest harness? In case you didn't know, the
> other harness lives in layout/tools/reftest, not under testing for some
> reason. I was also under the impression that we already do import reftests
> from somewhere else, though I have no idea if the ones you are importing are
> the same or not.

So essentially the reason that there's a second reftest harness here is
a) It was easy to do (and make parallel, I'm not sure if reftests already are or not)
b) The metadata isn't in mozilla-style manifest files, so different code is needed to work out which tests exist (alternatively, such manifest files would have to be generated on import)
c) It is useful (e.g. for implementation reports) to run all the tests from this repository together rather than having them split across multiple test harnesses

It may be that these reasons are not compelling enough to duplicate the functionality.

> ::: testing/web-platform-tests/wpttests/wpttest.py
> @@ +9,5 @@
> > +
> > +#These are quite similar to moztest, but slightly different
> > +class Result(object):
> > +    def __init__(self, status, message, expected=None):
> > +        if status not in self.statuses:
> 
> Please use
> https://github.com/mozilla/mozbase/blob/master/moztest/moztest/results.py
> for result collection. Again feel free to modify it if functionality is
> missing.

I can do, I guess, although it will need modification. In general it's hard to work out what the use case for all the methods in those classes is supposed to be since there isn't any documentation. It's also not really clear to me why I would need to store things like the test duration since that can be worked out from the event stream (comparing the timestamps of the test start and test end messages).
Assignee

Comment 18

6 years ago
(In reply to :Ms2ger from comment #13)
> Comment on attachment 8348120 [details] [diff] [review]
> add_wptrunner.diff
> 
> Review of attachment 8348120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/web-platform-tests/Makefile.in
> @@ +18,5 @@
> > +  $(NULL)
> > +
> > +_DEST_DIR = $(DEPTH)/_tests/web-platform-tests
> > +libs:: $(WPT_HARNESS)
> > +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)
> 
> Build peers won't approve :)

I just copied this; I don't really understand make or the Mozilla build system properly. So I need rather more help on what exactly I have done wrong and how I am supposed to fix it :)
Flags: needinfo?(gps)
(In reply to Joel Maher (:jmaher) from comment #16)
> I am not aware of any tests running python 2.6.  This has been verified on
> all desktop and mobile platforms.  Could we be talking about b2g tests?  
> 
> ahal, please educate us on this.

We needed to land https://github.com/MihaiTabara/blobuploader/commit/4813b5fe197a5b9ea97754c0073eed5c36a8072f recently because the {} string formatting syntax is 2.7 only. I don't remember what slave pool it was failing on, I think it was the fedora-r3's which we are trying to deprecate. Maybe that's the only pool still using 2.6, in which case ignore my comment :)
Flags: needinfo?(ahalberstadt)
(In reply to James Graham [:jgraham] from comment #17)
> > ::: testing/web-platform-tests/wpttests/machlogging.py
> > @@ +5,5 @@
> > > +
> > > +import logging
> > > +from mach import logging as mach_logging
> > > +
> > > +import structuredlog
> > 
> > Would like to use mozlog. I think mach will eventually use mozlog too.
> 
> So, I think this is at the heart of where I need to understand the long-term
> vision. Fundamentally, I think my point of view is that "structuredlog" is
> badly named because what it's doing isn't really "logging". Instead it's the
> producer side of an event/message stream that is the fundamental output/API
> of a testing process. 
> 
> Although it is of course possible to model the whole thing as a series of
> log messages but I don't think it makes much sense. For example I don't see
> how it is useful to try and provide an unique ordering of all event types
> according to severity in the way that the log module does. It also doesn't
> seem all that useful to have the tree of loggers that the logging module
> allows; each test harness should produce exactly one event stream as its
> output.
> 
> So whilst this could probably "use mozlog" in some way, I don't think that's
> the right direction to go in. Unless there is some compelling advantage of
> the "logging" paradigm for test-harness produced messages.

So what you described structuredlog as, is also what I envision mozlog should be. I agree that you don't want to use mozlog in its current state, I'm trying to say we should move it towards a state where it *would* make sense to use. I also agree we don't care about log levels, mozlog already has a JSONFormatter which does away with that notion. We should expand and improve upon this. I would even consider completely re-writing mozlog and not basing it off of python logging, if that makes sense.

My motivation for this is simple. Each harness has its own slightly special log format and I want every harness to have the same log format. The easiest way to do this is to make them all use the same log producer/consumer. Right now mozlog acts as both a producer and a consumer (albeit bare bones) and we still need to write a standard JS producer.

> > Why are we creating a second reftest harness? In case you didn't know, the
> > other harness lives in layout/tools/reftest, not under testing for some
> > reason. I was also under the impression that we already do import reftests
> > from somewhere else, though I have no idea if the ones you are importing are
> > the same or not.
> 
> So essentially the reason that there's a second reftest harness here is
> a) It was easy to do (and make parallel, I'm not sure if reftests already
> are or not)

Bug 813742 tracks parallelizing them. I think they are close to landing.

> b) The metadata isn't in mozilla-style manifest files, so different code is
> needed to work out which tests exist (alternatively, such manifest files
> would have to be generated on import)

Valid reason. Generating them probably wouldn't be terribly difficult though. But I can see why we wouldn't want to do that.

> c) It is useful (e.g. for implementation reports) to run all the tests from
> this repository together rather than having them split across multiple test
> harnesses

We can still run them separately. It would be easy to schedule a separate job that just passes in a different manifest to the main reftest harness.

I guess if the harness is that stupid simple, it works and it won't need to change much, then there isn't too much harm in duplicating.

> I can do, I guess, although it will need modification. In general it's hard
> to work out what the use case for all the methods in those classes is
> supposed to be since there isn't any documentation. It's also not really
> clear to me why I would need to store things like the test duration since
> that can be worked out from the event stream (comparing the timestamps of
> the test start and test end messages).

Does it force you to store stuff like that? I think marionette is the only thing using that results module at the moment, so it should be easy to change it.
(In reply to Andrew Halberstadt [:ahal] from comment #20)
> > b) The metadata isn't in mozilla-style manifest files, so different code is
> > needed to work out which tests exist (alternatively, such manifest files
> > would have to be generated on import)
> 
> Valid reason. Generating them probably wouldn't be terribly difficult
> though. But I can see why we wouldn't want to do that.

We could also add the ability to parse manifestdestiny ini style manifests to the main harness in addition to the "classic" reftest style. May not be a good idea though.
Assignee

Comment 22

6 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #21)
> (In reply to Andrew Halberstadt [:ahal] from comment #20)
> > > b) The metadata isn't in mozilla-style manifest files, so different code is
> > > needed to work out which tests exist (alternatively, such manifest files
> > > would have to be generated on import)
> > 
> > Valid reason. Generating them probably wouldn't be terribly difficult
> > though. But I can see why we wouldn't want to do that.
> 
> We could also add the ability to parse manifestdestiny ini style manifests
> to the main harness in addition to the "classic" reftest style. May not be a
> good idea though.

Sorry, I was a little unclear here, I mean that web-platform-tests uses a JSON manifest format (with separate manifest-desinty-like human-editable files for expected results and for disabling tests), which is neither manifest-destiny style nor mozilla-reftest style.
drive by- I would really like to see if there is a way to have a common format (either reftest or manifest destiny) for defining conditions/expected results.  I am fine with a json format for the internal and using our common format as a bridge- adding yet another manifest format that is hard to figure out is not a win/win for mozilla developers.
Assignee

Comment 24

6 years ago
So the JSON is only used for non-human-editable autogenerated metadata i.e. the list of tests. This is automatically extracted from the test files themselves. It should be unnecessary for anyone to learn that file format unless they are writing tools that work with the tests. 

All human-editable things are using something rather like manifest destiny, but with a few differences because manifest destiny makes a lot of assumptions that don't really make sense here. For example in manifest destiny each section heading is the path to a test and, at least as far as I can tell, there is no explicit way to represent any subtests. So the format here is one file per test that has non-default data with subheadings for the overall file and for each subtest, something like:

[FILE]
status = ERROR

[some subtest in the file]
status = FAIL

[some other subtest]
disabled = bug1234

Conditions aren't supported at the moment, but if they were they would reuse the manifest destiny syntax.

Is that close enough to the existing practice to assuage your concerns? If not do you have a suggestion for a better approach?
that is reasonable. subtests are a hard one to resolve, maybe a manifest destiny 3.14 in the future could figure out a good solution for that- no time soon.
Comment on attachment 8348120 [details] [diff] [review]
add_wptrunner.diff

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

General nit: need moar license headers.

::: testing/web-platform-tests/Makefile.in
@@ +22,5 @@
> +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)
> +libs:: $(WPT_TESTS)
> +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)
> +libs:: $(WPT_METADATA)
> +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)

Generally speaking, any $(INSTALL) in a Makefile.in should not exist and the file install should be facilitated through a moz.build variable, a test manifest, or INSTALL_TARGETS in Makefile.in if none of those are available.

We currently lack a good solution for installing test harness files via moz.build, so an INSTALL_TARGETS in Makefile.in is likely your best bet.

Test files themselves, however, should be defined in manifests of some sort and we should not require people to update Makefile.in (only moz.build or manifest files) when adding new tests.

::: testing/web-platform-tests/mach_commands.py
@@ +23,5 @@
> +    CommandProvider,
> +    Command,
> +)
> +
> +from wpttests import wptcommandline, machlogging, structuredlog

Don't import non-core modules from module level in mach_commands.py, as it slows down mach. We have a bug somewhere to introduce lazy importing of modules to mach command modules. Until then.

@@ +46,5 @@
> +            sys.path.append(build_path)
> +
> +        from wpttests import wptrunner
> +
> +        dummy_log = StringIO()

Variable not used.

@@ +71,5 @@
> +arg_list = wptcommandline.create_argument_list()
> +
> +@CommandProvider
> +class MachCommands(MachCommandBase):
> +    @arg_list.decorate(Command, CommandArgument)

Nice trick.

::: testing/web-platform-tests/wpttests/machlogging.py
@@ +86,5 @@
> +                data["_subtests"] = self.buffer.get(test, {"count":0, "unexpected":0, "pass":0})
> +
> +            structuredlog.StreamHandler.__call__(self, data)
> +
> +class StructuredHumanFormatter(object):

This code seems familiar. mach.logging contained a lot of crappy code IMO. I'd prefer to not see it emulated/copied.

::: testing/web-platform-tests/wpttests/wptcommandline.py
@@ +49,5 @@
> +            if command_wrapper:
> +                g = command_wrapper(*self.args,
> +                                    **self.kwargs)(g)
> +            return g
> +        return func

This is a gnarly function. I'd prefer we just add a mechanism to mach to allow it to pull in an existing function that populates an argparse.ArgumentParser. The hacky way to do this today is to pass in allow_all_args=True to the mach @Command and you can instantiate an ArgumentParser and pass the passed in arguments list to that parser. The unfortunate side-effect of this is that |mach help command| won't display useful help. That's why native integration with an existing function to populate an argument parser is a better idea.

::: testing/web-platform-tests/wpttests/wptrunner.py
@@ +4,5 @@
> +import os
> +import urlparse
> +import json
> +from Queue import Queue, Empty
> +from multiprocessing import Process, Pipe

Watch out for parts of multiprocessing not working on BSDs because of a missing lock implementation there. I'm pretty sure multiprocessing.Process works and I /think/ multiprocessing.Pipe works. But if they don't, the BSD people will get upset that they can't adequately test the tree.
Attachment #8348120 - Flags: feedback+
Flags: needinfo?(gps)
Assignee

Comment 27

6 years ago
(In reply to Gregory Szorc [:gps] from comment #26)

> Test files themselves, however, should be defined in manifests of some sort
> and we should not require people to update Makefile.in (only moz.build or
> manifest files) when adding new tests.

Not a problem (as such) in this case because the tests come from upstream and there is an auto-generated manifest file (i.e. the manifest is just a cache of information that you can extract from the source tree).

> ::: testing/web-platform-tests/wpttests/machlogging.py
> @@ +86,5 @@
> > +                data["_subtests"] = self.buffer.get(test, {"count":0, "unexpected":0, "pass":0})
> > +
> > +            structuredlog.StreamHandler.__call__(self, data)
> > +
> > +class StructuredHumanFormatter(object):
> 
> This code seems familiar. mach.logging contained a lot of crappy code IMO.
> I'd prefer to not see it emulated/copied.

Hmm, so I think the bit of code you commented on is one part that actually wasn't copied from mach.logging, although it is similar to something there. I'm not sure what you recommend instead for this case.

> ::: testing/web-platform-tests/wpttests/wptcommandline.py
> @@ +49,5 @@
> > +            if command_wrapper:
> > +                g = command_wrapper(*self.args,
> > +                                    **self.kwargs)(g)
> > +            return g
> > +        return func
> 
> This is a gnarly function. I'd prefer we just add a mechanism to mach to
> allow it to pull in an existing function that populates an
> argparse.ArgumentParser. The hacky way to do this today is to pass in
> allow_all_args=True to the mach @Command and you can instantiate an
> ArgumentParser and pass the passed in arguments list to that parser. The
> unfortunate side-effect of this is that |mach help command| won't display
> useful help. That's why native integration with an existing function to
> populate an argument parser is a better idea.

I just tried this and it seems to work OK. Maybe I should create a seperate bug for those changes?
Assignee

Comment 28

6 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #20)
>
> So what you described structuredlog as, is also what I envision mozlog
> should be. I agree that you don't want to use mozlog in its current state,
> I'm trying to say we should move it towards a state where it *would* make
> sense to use. I also agree we don't care about log levels, mozlog already
> has a JSONFormatter which does away with that notion. We should expand and
> improve upon this. I would even consider completely re-writing mozlog and
> not basing it off of python logging, if that makes sense.
> 
> My motivation for this is simple. Each harness has its own slightly special
> log format and I want every harness to have the same log format. The easiest
> way to do this is to make them all use the same log producer/consumer. Right
> now mozlog acts as both a producer and a consumer (albeit bare bones) and we
> still need to write a standard JS producer.

OK, it's great that we have the same vision :) What, concretely, do you want me to do with the code I have? Move structuredlog.py to mozlog and slowly migrate towards that? Do something (what?) to the existing mozlog code to add the extra features here? I think I am in favour of the first approach, where we don't build on top of the logging module, or even consider it logging per-se. But there could be much I am missing.
Fwiw, gaston (OpenBSD) isn't currently running tests, and I think he will probably have full multiprocessing support before he wants to run these tests, so no need to avoid mp for his sake.
Yes, our (OpenBSD) libc gained sem_open() recently and our python now picks it up, so i can confirm multiprocessing.synchronize & multiprocessing.Pool now works (they were broken before), and i can run "from multiprocessing import Process, Pipe" without issues.

Thanks ms2ger for caring about us :)
Assignee

Updated

5 years ago
Depends on: 959709
So I think windows builds are perma broken on cedar since Jan 23rd due to web-platform-tests made from one of these commits:
https://tbpl.mozilla.org/?showall=all&tree=Cedar&pusher=james@hoppipolla.co.uk
  3262913b7a0c James Graham – Bug 945222 - Update web-platform-tests expected data to revision
  aac19517c5f7 James Graham – Bug 945222 - Update web-platform-tests to revision
  97504eb5cc73 James Graham – Bug 945222 - Add test harness for running W3C web-platform-tests. 

Since those commits and judging by the fact that other platforms have been working, I think there has been a bad pathname defined and windows is complaining about it (maybe slashes or escapes?).
    https://tbpl.mozilla.org/php/getParsedLog.php?id=33691330&tree=Cedar#error0

I am in the middle of trying to get some metro talos tests out the door but was hoping to get one last test on cedar before enabling on m-c. 

This is simply an FYI in case it hasn't been noticed. If this is known, is there an ETA on fix?
wahoo! Thank you jgraham. looks like windows is building green again :)
Assignee

Comment 33

5 years ago
Sorry it took so long :)
Depends on: 965445
Assignee

Updated

5 years ago
Depends on: 966300
Depends on: 974358
Assignee

Comment 34

5 years ago
Most of the code for this now has its canonical home on the W3C github [1] and there is a code review open for the initial landing [2]. Review for that would be appreciated; I guess ahal is the obvious reviewer. I can help get you started with critic if needed. Once that is reviewed it should be relatively straightforward to land the mozilla-specific parts.

The current state of the tests on cedar is that we are seeing a low-single-digits number of unstable tests per run; the last run was green on OSX 10.6/10.9, had 4 spurious results on Linux 64 and 2 on Linux 32. However the actual number of tests will some instability is of course higher than this; one run is only a small sample.

[1] https://github.com/w3c/wptrunner
[2] https://critic.hoppipolla.co.uk/r/1431
Assignee

Updated

5 years ago
Depends on: 914563
Assignee

Updated

5 years ago
Depends on: 1018936
Assignee

Comment 38

5 years ago
Starting to add some of the integration bits for review since they are rather stable. The main bulk of the harness code is being reviewed at [1], so consult that for the remote side of the API.

[1] https://critic.hoppipolla.co.uk/r/1431
Attachment #8434125 - Attachment is obsolete: true
Attachment #8442207 - Flags: review?(ahalberstadt)
Comment on attachment 8442209 [details] [diff] [review]
Mach-related parts of web-platform-tests integration

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

I only looked at part of this. Will get to the rest another time. Although I may redirect it - I don't need to be the bottleneck for every mach command.

::: testing/web-platform/mach_commands.py
@@ +25,5 @@
> +)
> +
> +here = os.path.abspath(os.path.split(__file__)[0])
> +sys.path.insert(0, os.path.join(here, "harness"))
> +sys.path.insert(0, here)

This file gets imported when mach runs so these 3 lines would be executed for every mach invocation. That's not right.

We'd like to move all our Python to look like a virtualenv, which means creating packages for all imported modules and adding paths of packages to the virtualenv.

I suspect this should have been done in the prior patch.

You can use https://hg.mozilla.org/mozilla-central/rev/f9614eb176ad as a rough template for creating Python packages. Essentially:

1) Create a new directory that has the name of the package you are creating (mozwebidlcodegen in that example)
2) Add an empty __init__.py file to denote it as a directory containing Python modules
3) Add the directory to build/virtualenv_packages.txt

@@ +27,5 @@
> +here = os.path.abspath(os.path.split(__file__)[0])
> +sys.path.insert(0, os.path.join(here, "harness"))
> +sys.path.insert(0, here)
> +
> +from wptrunner import wptcommandline

Please delay import this until a @Command function so it doesn't get loaded on every mach invocation (yes, we really need a delay import implementation in mach to make this go away).

::: testing/web-platform/machlogging.py
@@ +1,3 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

This file looks suspiciously like python/mach/mach/logging.py. Why the copying?
Attachment #8442209 - Flags: feedback+
Comment on attachment 8442207 [details] [diff] [review]
Integration of wptrunner excluding mach components

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

::: .hgignore
@@ +72,5 @@
>  GSYMS
>  GPATH
> +
> +#Ignore staging area for updating web-platform-tests
> +^testing/web-platform-tests/sync/

We shouldn't be writing staging content into the srcdir. In my ideal world, we can build with srcdir on a read-only filesystem. Please stage to the object directory.

Looking at the Makefile, I'm not sure why you even need this. Perhaps it is/was a bug in the stage-package target?
Assignee

Comment 42

5 years ago
(In reply to Gregory Szorc [:gps] from comment #41)
> Comment on attachment 8442207 [details] [diff] [review]
> Integration of wptrunner excluding mach components
> 
> Review of attachment 8442207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: .hgignore
> @@ +72,5 @@
> >  GSYMS
> >  GPATH
> > +
> > +#Ignore staging area for updating web-platform-tests
> > +^testing/web-platform-tests/sync/
> 
> We shouldn't be writing staging content into the srcdir. In my ideal world,
> we can build with srcdir on a read-only filesystem. Please stage to the
> object directory.

Possibly the comment is misleading. This is a "staging" area for updating the tests from upstream, nothing to do with building or packaging. It's a directory that contains a git checkout of the upstream web-platform-tests repo, the contents of which are subsequently copied over to a directory that is committed to m-c. That obviously won't work on a read-only file system, so I don't think that's a relevant concern. It also doesn't require a build, so it's not obvious that putting this in the objdir would be better. But I'm happy to move it if there's a good reason to do so.
Assignee

Comment 43

5 years ago
(In reply to Gregory Szorc [:gps] from comment #40)

> > +here = os.path.abspath(os.path.split(__file__)[0])
> > +sys.path.insert(0, os.path.join(here, "harness"))
> > +sys.path.insert(0, here)
> > +
> > +from wptrunner import wptcommandline
> 
> Please delay import this until a @Command function so it doesn't get loaded
> on every mach invocation (yes, we really need a delay import implementation
> in mach to make this go away).

That file provides the parser arguments for the commands, so it has to be loaded at the top level.
Assignee

Comment 44

5 years ago
(In reply to Gregory Szorc [:gps] from comment #40)

> ::: testing/web-platform/machlogging.py
> @@ +1,3 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> This file looks suspiciously like python/mach/mach/logging.py. Why the
> copying?

Honestly, because I'm not sure what LoggingManager is trying to achieve. I think the right thing to do might just be to rewrite it to work better with structured logging conventions, but I don't really know what the requirements are.
Comment on attachment 8442207 [details] [diff] [review]
Integration of wptrunner excluding mach components

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

This looks good. r+ with nits addressed or explained.

::: testing/web-platform/runtests.py
@@ +1,5 @@
> +#!/usr/bin/env python
> +import os
> +import sys
> +
> +here = os.path.dirname(__file__)

You may want to do 'os.path.abspath(os.path.dirname(__file__))' to avoid putting a relative path in sys.path.

@@ +7,5 @@
> +
> +marionette_deps = [('client', 'marionette'),
> +                   ('transport', 'marionette_transport')]
> +
> +mozbase_deps = ([('manifestdestiny', 'manifestparser')] +

It's no longer called manifestdestiny as of recently.

@@ +32,5 @@
> +    if (os.path.exists(path) and
> +        path not in sys.path and
> +        module not in sys.modules):
> +        try:
> +            __import__(module)

Why the need for __import__ over import?

::: testing/web-platform/update.py
@@ +1,5 @@
> +#!/usr/bin/env python
> +import sys
> +import os
> +
> +here = os.path.dirname(__file__)

Same as above.

@@ +4,5 @@
> +
> +here = os.path.dirname(__file__)
> +sys.path.insert(0, os.path.join(here, "harness"))
> +
> +from wptrunner import update, wptcommandline

Unused wptcommandline?
Attachment #8442207 - Flags: review?(ahalberstadt) → review+
Assignee

Updated

5 years ago
Depends on: 1023371
Assignee

Updated

5 years ago
Depends on: 1027665
Assignee

Comment 46

5 years ago
Comment on attachment 8442209 [details] [diff] [review]
Mach-related parts of web-platform-tests integration

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

::: testing/web-platform/mach_commands.py
@@ +25,5 @@
> +)
> +
> +here = os.path.abspath(os.path.split(__file__)[0])
> +sys.path.insert(0, os.path.join(here, "harness"))
> +sys.path.insert(0, here)

AFAICT this doesn't work in this case because the command line arguments are in the package itself. Therefore it has to be in the path when mach is scanning for commands, which appears to be before it has created the virtualenv.
Assignee

Comment 47

5 years ago
Comment on attachment 8442207 [details] [diff] [review]
Integration of wptrunner excluding mach components

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

::: testing/web-platform/runtests.py
@@ +32,5 @@
> +    if (os.path.exists(path) and
> +        path not in sys.path and
> +        module not in sys.modules):
> +        try:
> +            __import__(module)

The idea here is to see whether the package is already in the environment and if not add the in-tree version to sys.path. Actually importing it isn't really the point.

Therefore I don't think the import statement works. importlib.import_module might be preferred, but it's not clear (to me) why.
(In reply to James Graham [:jgraham] from comment #42)
> (In reply to Gregory Szorc [:gps] from comment #41)
> > Comment on attachment 8442207 [details] [diff] [review]
> > Integration of wptrunner excluding mach components
> > 
> > Review of attachment 8442207 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: .hgignore
> > @@ +72,5 @@
> > >  GSYMS
> > >  GPATH
> > > +
> > > +#Ignore staging area for updating web-platform-tests
> > > +^testing/web-platform-tests/sync/
> > 
> > We shouldn't be writing staging content into the srcdir. In my ideal world,
> > we can build with srcdir on a read-only filesystem. Please stage to the
> > object directory.
> 
> Possibly the comment is misleading. This is a "staging" area for updating
> the tests from upstream, nothing to do with building or packaging. It's a
> directory that contains a git checkout of the upstream web-platform-tests
> repo, the contents of which are subsequently copied over to a directory that
> is committed to m-c. That obviously won't work on a read-only file system,
> so I don't think that's a relevant concern. It also doesn't require a build,
> so it's not obvious that putting this in the objdir would be better. But I'm
> happy to move it if there's a good reason to do so.

Ahh. How much work would it be to rename it to "upstream-staging" or something? All this test packaging logic is difficult to understand as it is. If I got thrown off by this, you can manage others will as well.
Comment on attachment 8442209 [details] [diff] [review]
Mach-related parts of web-platform-tests integration

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

I think :ahal is a sufficient reviewer for this.
Attachment #8442209 - Flags: review?(gps) → review?(ahalberstadt)
(In reply to James Graham [:jgraham] from comment #46)
> ::: testing/web-platform/mach_commands.py
> @@ +25,5 @@
> > +)
> > +
> > +here = os.path.abspath(os.path.split(__file__)[0])
> > +sys.path.insert(0, os.path.join(here, "harness"))
> > +sys.path.insert(0, here)
> 
> AFAICT this doesn't work in this case because the command line arguments are
> in the package itself. Therefore it has to be in the path when mach is
> scanning for commands, which appears to be before it has created the
> virtualenv.

Yeah, it certainly isn't great.. but most of the other mach commands just re-define the arguments that are important when running locally (it turns out our harnesses have a lot of arguments that no one would ever care about).

One way to potentially get around this is to use 'nargs=argparse.REMAINDER' and then passing the resulting list into wptrunner wholesale. E.g:
https://github.com/ahal/b2g-commands/blob/master/b2gcommands/sync_commands.py#L29

The downside to this approach is running |mach help web-platform-tests| won't show you the available arguments.. though running |mach web-platform-tests --help| will.
Comment on attachment 8442209 [details] [diff] [review]
Mach-related parts of web-platform-tests integration

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

This looks good, but giving an r- for now because of the top level imports/sys.path changes and the lack of a condition function.

::: testing/web-platform/mach_commands.py
@@ +25,5 @@
> +)
> +
> +here = os.path.abspath(os.path.split(__file__)[0])
> +sys.path.insert(0, os.path.join(here, "harness"))
> +sys.path.insert(0, here)

If you have an __init__.py in harness and wptrunner you should be able to do a relative import ('from .harness.wptrunner import wptcommandline') without needing to touch sys.path.

@@ +27,5 @@
> +here = os.path.abspath(os.path.split(__file__)[0])
> +sys.path.insert(0, os.path.join(here, "harness"))
> +sys.path.insert(0, here)
> +
> +from wptrunner import wptcommandline

I agree with gps here, see my previous comment. I might be persuaded to leave this as is, as long as we aren't touching sys.path.

@@ +93,5 @@
> +        from mozlog.structured import structuredlog
> +
> +        log_manager = machlogging.StructuredLoggingManager()
> +        self._logger = structuredlog.StructuredLogger("web-platform-tests.update.mach")
> +        MozbuildObject.__init__(self, topsrcdir, settings, log_manager, topobjdir)

Might as well just pass through "*args, **kwargs" so this doesn't break if the MozbuildObject definition changes.

@@ +131,5 @@
> +@CommandProvider
> +class MachCommands(MachCommandBase):
> +    @Command("web-platform-tests",
> +             category="testing",
> +             parser=wptcommandline.create_parser(False))

You should add a condition here so this is only enabled if a firefox (or b2g?) build is active, e.g:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py#614

Note condition functions are AND'ed so if you want this to be enabled for both firefox and b2g, you'll need to define a new function in here that OR's those conditions together.
Attachment #8442209 - Flags: review?(ahalberstadt) → review-
Assignee

Comment 52

5 years ago
Comment on attachment 8442209 [details] [diff] [review]
Mach-related parts of web-platform-tests integration

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

::: testing/web-platform/mach_commands.py
@@ +25,5 @@
> +)
> +
> +here = os.path.abspath(os.path.split(__file__)[0])
> +sys.path.insert(0, os.path.join(here, "harness"))
> +sys.path.insert(0, here)

OK, it seems like the right thing to do here is to add the path to the mach SEARCH_PATH. Although this seems to have exactly the same effect, it is at least more consistent (at the expense of poorer code locality).

@@ +27,5 @@
> +here = os.path.abspath(os.path.split(__file__)[0])
> +sys.path.insert(0, os.path.join(here, "harness"))
> +sys.path.insert(0, here)
> +
> +from wptrunner import wptcommandline

It gets loaded on every import because it's needed to set the command line options.

I have profiled the code a bit and removed some slow introspection that was being done to work out what browser implementations are supported by wptrunner. The majority of the time in create_parser now seems to be adding mozlog related arguments. Possibly that code can also be optimised a bit, but I don't think copying the arguments into mach is going to significantly improve things. Indeed I'm not sure how that would work without either foregoing the command line help, or making a static list of mozlog options that would have to be updated every time that mozlog is. Neither solution seems acceptable.

Generally I agree that this is a mach problem. It is quite dynamic (there is no central list of all mach commands) and does quite a lot of mandatory work on startup (parses all the commands and generates a list of all command line arguments). Then we try to enforce the rule that all the commands themselves have to be very static and so do very little work to make up for the shortcoming of mach. Fundamentally I don't think mach needs deferred imports, it just needs to defer building command line parsers for commands that aren't being run. Then, either with the current syntax, or with an even simpler text-based syntax, you could make mach_commands.py files look like:

__mach__ = [{"command": "my-command",
             "description": "Some fancy command",
             "args": "func_returning_args",
             "conditions": [condition_func]}]

and the args function wouldn't even be evaluated unless the command itself was selected. True, argparse isn't designed to support this use case out of the box, but it doesn't seem like it would be especially complicated to add.
Assignee

Comment 53

5 years ago
Posted file wptrunner harness upstream (obsolete) —
Attachment #8348120 - Attachment is obsolete: true
Attachment #8449390 - Flags: review?(ahalberstadt)
Assignee

Comment 54

5 years ago
Posted patch wpt-integration-mach-2.diff (obsolete) — Splinter Review
Update to fix some of the mach integration issues.

I didn't remove the "import wptcommandline" since this is needed to compute the command line arguments. I did optimise that computation upstream so it became faster. Further optimisations can be made in mozlog.

I also didn't use *args with MozbuildObject because I replace one of the arguments passed in (log_manager). This might be unnecessary, but I think the work on improving the integration of mach and structured logging need not block this bug.
Attachment #8442209 - Attachment is obsolete: true
Attachment #8449453 - Flags: review?(ahalberstadt)
Assignee

Updated

5 years ago
Blocks: 996368
Comment on attachment 8449390 [details] [review]
wptrunner harness upstream

https://critic.hoppipolla.co.uk/r/1431
Attachment #8449390 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8449453 [details] [diff] [review]
wpt-integration-mach-2.diff

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

r+ with comments addressed

::: testing/web-platform/mach_commands.py
@@ +10,5 @@
> +import os
> +import shutil
> +import sys
> +
> +from StringIO import StringIO

mozpack.path, shutil and StringIO are all unused imports. For future you can run files through pyflakes to catch stuff like this.

@@ +19,5 @@
> +    MozbuildObject,
> +)
> +
> +from mach.decorators import (
> +    CommandArgument,

Also unused

@@ +68,5 @@
> +        self.setup_kwargs(kwargs)
> +
> +        kwargs["capture_stdio"] = True
> +        logger = wptrunner.setup_logging(kwargs, {})
> +        self.log_manager.register_structured_logger(wptrunner.logger)

Should this just be 'logger'? Otherwise the above logger variable is unused.

@@ +90,5 @@
> +        from mozlog.structured import structuredlog
> +
> +        log_manager = machlogging.StructuredLoggingManager()
> +        self._logger = structuredlog.StructuredLogger("web-platform-tests.update.mach")
> +        MozbuildObject.__init__(self, topsrcdir, settings, log_manager, topobjdir)

This seems mostly the same as above, could it be factored into a shared location? Feel free to ignore this.
Attachment #8449453 - Flags: review?(ahalberstadt) → review+
Assignee

Comment 57

5 years ago
Updates for non-mach parts of the integration to reflect upstream changes in wptrunner.
Attachment #8442207 - Attachment is obsolete: true
Assignee

Comment 58

5 years ago
Updates of mach integration for upstream changes and review comments.
Attachment #8449453 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Blocks: 1041535
Assignee

Updated

5 years ago
Blocks: 1041542
Assignee

Updated

5 years ago
Depends on: 1028304
Assignee

Comment 59

5 years ago
This includes a couple of somewhat-temporary bits; the machlogging.py file should go away once we have a better story for integrating structured logging with mach. The fetchlogs.py file should be generalised so that it's easy to retrieve structured log data from blobber for any testrun. However I think it makes more sense to land these bits than to wait for this cleanup work.
Attachment #8434127 - Attachment is obsolete: true
Attachment #8434129 - Attachment is obsolete: true
Attachment #8449390 - Attachment is obsolete: true
Attachment #8458738 - Attachment is obsolete: true
Attachment #8458740 - Attachment is obsolete: true
Attachment #8475845 - Flags: review?(ahalberstadt)
Assignee

Comment 60

5 years ago
Attachment #8475845 - Attachment is obsolete: true
Attachment #8475845 - Flags: review?(ahalberstadt)
Attachment #8475855 - Flags: review?(ahalberstadt)
Comment on attachment 8475855 [details] [diff] [review]
web-platform-tests changes excluding upstream harness, tests, metadata

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

r+ with some nits fixed and questions answered. You'll need to get build peer review for the Makefile changes though.

::: .hgignore
@@ +75,5 @@
>  # Unit tests for Loop
>  ^browser/components/loop/standalone/content/config\.js$
> +
> +# Ignore staging area for updating web-platform-tests
> +^testing/web-platform-tests/sync/

Shouldn't the staging directory be in the objdir somewhere?

::: testing/config/mozharness/web_platform_tests_config.py
@@ +13,5 @@
> +    MINIDUMP_STACKWALK_PATH = "%(abs_work_dir)s/tools/breakpad/win32/minidump_stackwalk.exe"
> +elif system() == "Darwin":
> +    MINIDUMP_STACKWALK_PATH = "%(abs_work_dir)s/tools/breakpad/osx64/minidump_stackwalk"
> +else:
> +    MINIDUMP_STACKWALK_PATH = None

Traditionally this just gets set up in the mozharness script. I guess this is ok too, but probably not much reason for it to live in-tree.

::: testing/web-platform/fetchlogs.py
@@ +1,1 @@
> +import argparse

missing mpl header

@@ +45,5 @@
> +    with open(name, "w") as f:
> +        resp = requests.get(url)
> +        #temp_f = cStringIO.StringIO(resp.text.encode(resp.encoding))
> +        #gzf  = gzip.GzipFile(fileobj=temp_f)
> +        #f.write(gzf.read())

nit: remove these commented out lines

::: testing/web-platform/machlogging.py
@@ +1,4 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

I remember gps was wondering why this file was being copied from python/mach and I don't remember what the answer was.

::: testing/web-platform/runtests.py
@@ +1,1 @@
> +#!/usr/bin/env python

missing mpl header

::: testing/web-platform/update.py
@@ +1,1 @@
> +#!/usr/bin/env python

missing mpl header
Attachment #8475855 - Flags: review?(ahalberstadt) → review+
Assignee

Comment 62

5 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #61)
> Comment on attachment 8475855 [details] [diff] [review]
> web-platform-tests changes excluding upstream harness, tests, metadata
> 
> Review of attachment 8475855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with some nits fixed and questions answered. You'll need to get build
> peer review for the Makefile changes though.
> 
> ::: .hgignore
> @@ +75,5 @@
> >  # Unit tests for Loop
> >  ^browser/components/loop/standalone/content/config\.js$
> > +
> > +# Ignore staging area for updating web-platform-tests
> > +^testing/web-platform-tests/sync/
> 
> Shouldn't the staging directory be in the objdir somewhere?

Perhaps? But it's "staging" in the sense of "temporary location to hold a clone of web-platform-tests when copying the files over". This operation doesn't require a build so it isn't obviously a good idea to require an object directory.

> ::: testing/config/mozharness/web_platform_tests_config.py
> @@ +13,5 @@
> > +    MINIDUMP_STACKWALK_PATH = "%(abs_work_dir)s/tools/breakpad/win32/minidump_stackwalk.exe"
> > +elif system() == "Darwin":
> > +    MINIDUMP_STACKWALK_PATH = "%(abs_work_dir)s/tools/breakpad/osx64/minidump_stackwalk"
> > +else:
> > +    MINIDUMP_STACKWALK_PATH = None
> 
> Traditionally this just gets set up in the mozharness script. I guess this
> is ok too, but probably not much reason for it to live in-tree.

Well yes, but I reasoned that there also wasn't that much reason for it to live out of tree e.g. if you want to experiment with a different stackwalk implementation this makes it possible without having to edit the mozharness code.
 
> ::: testing/web-platform/machlogging.py
> @@ +1,4 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> 
> I remember gps was wondering why this file was being copied from python/mach
> and I don't remember what the answer was.

Essentially that I was making some attempt to integrate structured logging with mach's concept of a logging manager. Since this just seems to be causing confusion I'll remove it for now and pretend that the mach stuff doesn't exist.
Assignee

Comment 63

5 years ago
Addressed review comments.

Ted: would you mind reviewing the build system changes?
Attachment #8475855 - Attachment is obsolete: true
Attachment #8476692 - Flags: review?(ted)
Assignee

Comment 64

5 years ago
Comment on attachment 8476692 [details] [diff] [review]
web-platform-tests changes excluding upstream harness, tests, metadata

Hedging my bets on build system peers ;)
Attachment #8476692 - Flags: review?(gps)
Comment on attachment 8476692 [details] [diff] [review]
web-platform-tests changes excluding upstream harness, tests, metadata

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

This gets r+ with use of INSTALL_TARGETS.

::: .hgignore
@@ +76,5 @@
>  ^browser/components/loop/standalone/content/config\.js$
>  ^browser/components/loop/standalone/node_modules/
> +
> +# Git clone directory for updating web-platform-tests
> +^testing/web-platform-tests/sync/

You'll want to update .gitignore as well.

::: testing/profiles/Makefile.in
@@ +13,5 @@
> +_DEST_DIR_MOCHITEST = $(DEPTH)/_tests/testing/mochitest/profile_data
> +_DEST_DIR_WPT = $(DEPTH)/_tests/web-platform/prefs
> +libs:: $(PROFILE_FILES)
> +	$(PYTHON) $(topsrcdir)/config/nsinstall.py $^ $(_DEST_DIR_MOCHITEST)
> +	$(PYTHON) $(topsrcdir)/config/nsinstall.py $^ $(_DEST_DIR_WPT)

Please switch this to use INSTALL_TARGETS.

::: testing/web-platform/Makefile.in
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +include $(topsrcdir)/config/rules.mk
> +
> +#ifneq ($(OS_ARCH),WINNT)

Is there a reason this is commented out?

@@ +24,5 @@
> +	$(INSTALL) $(topsrcdir)/testing/web-platform/harness/wptrunner $(_DEST_DIR)
> +libs:: $(WPT_TESTS)
> +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)
> +libs:: $(WPT_METADATA)
> +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)

Please use INSTALL_TARGETS for copying files.

::: testing/web-platform/fetchlogs.py
@@ +6,5 @@
> +import cStringIO
> +import gzip
> +import json
> +import os
> +import requests

Since you are using a module not available to the build system (requests), you should consider replacing all the "talk to Treeherder" code in this file with the existing Python package (https://github.com/mozilla/treeherder-client) that does that.

@@ +9,5 @@
> +import os
> +import requests
> +import urlparse
> +
> +treeherder_base = "http://treeherder.mozilla.org/"

https://

::: testing/web-platform/mach_commands.py
@@ +19,5 @@
> +    CommandProvider,
> +    Command,
> +)
> +
> +from wptrunner import wptcommandline

This should be deferred so the import isn't done until a command that needs it is executed.

(mach really needs a lazy module importer.)

::: testing/web-platform/runtests.py
@@ +39,5 @@
> +        module not in sys.modules):
> +        try:
> +            __import__(module)
> +        except ImportError:
> +            sys.path.insert(0, path)

This makes me somewhat sad. We really need to be executing things in proper virtualenvs so scripts don't have to muck about with sys.path.

I guess there's no good alternative here?
Attachment #8476692 - Flags: review?(ted)
Attachment #8476692 - Flags: review?(gps)
Attachment #8476692 - Flags: feedback+
Assignee

Comment 66

5 years ago
> This gets r+ with use of INSTALL_TARGETS.
> ::: testing/web-platform/Makefile.in 
> @@ +24,5 @@
> > +	$(INSTALL) $(topsrcdir)/testing/web-platform/harness/wptrunner $(_DEST_DIR)
> > +libs:: $(WPT_TESTS)
> > +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)
> > +libs:: $(WPT_METADATA)
> > +	$(INSTALL) $(foreach f,$^,"$f") $(_DEST_DIR)
> 
> Please use INSTALL_TARGETS for copying files.

In this case I want to copy entire directories that are either directly from upstream (harness/wptrunner, tests) or based on the layout of upstream (meta), so I don't have apriori knowledge of the exact directory structure. AFAIK, INSTALL_TARGETS doesn't make this kind of recursive copying easy (but I might be wrong).

> ::: testing/web-platform/fetchlogs.py
> @@ +6,5 @@
> > +import cStringIO
> > +import gzip
> > +import json
> > +import os
> > +import requests
> 
> Since you are using a module not available to the build system (requests),
> you should consider replacing all the "talk to Treeherder" code in this file
> with the existing Python package
> (https://github.com/mozilla/treeherder-client) that does that.

Yep, that seems like a sensible idea. This file is really only here so that it's committed somewhere for the moment; it's not clear to me where it should live in the long term (maybe in that library as a utility function, for example).

> ::: testing/web-platform/mach_commands.py
> @@ +19,5 @@
> > +    CommandProvider,
> > +    Command,
> > +)
> > +
> > +from wptrunner import wptcommandline
> 
> This should be deferred so the import isn't done until a command that needs
> it is executed.

All the top level code in wptcommandline is needed to build the command parsers for the mach commands in this file, so the import can't be deferred.

> ::: testing/web-platform/runtests.py
> @@ +39,5 @@
> > +        module not in sys.modules):
> > +        try:
> > +            __import__(module)
> > +        except ImportError:
> > +            sys.path.insert(0, path)
> 
> This makes me somewhat sad. We really need to be executing things in proper
> virtualenvs so scripts don't have to muck about with sys.path.
> 
> I guess there's no good alternative here?

Well I guess "don't support the use case where there isn't already a proper virtualenv set up" is an alternative. I'm not sure if that's a requirement anywhere important; I *think* mozharness should work without this but I would have to check to be sure.
Assignee

Comment 67

5 years ago
Updated patch to fix the review issues, apart from those with questions above.
Attachment #8476692 - Attachment is obsolete: true
Assignee

Comment 68

5 years ago
gps: Do you have an acceptable alternative to using INSTALL_TARGETS for the parts of the build that want to copy an entire directory rather than specific listed files?
Attachment #8479001 - Attachment is obsolete: true
Flags: needinfo?(gps)
Directory copying is annoying in today's world. I could tell you to write a custom Python script that used mozpack.files.FileFinder to discover files and mozpack.copier.FileCopier to copy them. But that's a lot of work. At this point, I don't want to be stop energy over a pattern that is used elsewhere in the build system.

Please convert file copies to INSTALL_TARGETS. It's OK to leave the custom $(INSTALL) rule for the directory copy.
Flags: needinfo?(gps)
Assignee

Comment 70

5 years ago
Updated with INSTALL_TARGETS used for individual file copies.
Attachment #8479738 - Attachment is obsolete: true
Attachment #8480558 - Flags: review?(gps)
Comment on attachment 8480558 [details] [diff] [review]
web-platform-tests changes excluding upstream harness, tests, metadata

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

::: testing/profiles/Makefile.in
@@ +4,5 @@
>  MOCHITEST_PROFILE_FILES = \
> +		prefs_general.js \
> +		prefs_b2g_unittest.js \
> +		webapps_mochitest.json \
> +		$(NULL)

We try to avoid tabs in Makefile.in unless they are part of a rule.

::: testing/web-platform/README
@@ +1,5 @@
> +web-platform-tests
> +==================
> +
> +This directory contains the W3C
> +[web-platform-tests](http://github.com/w3c/web-platform-tests). They

This file seems like some kind of mix of markdown and rst. Pick one and stick an extension on the file?

::: testing/web-platform/fetchlogs.py
@@ +45,5 @@
> +        counter += 1
> +        sep = "" if not prefix else "-"
> +        name = os.path.join(dest, prefix + sep + str(counter) + ".log")
> +
> +    with open(name, "w") as f:

open(name, 'wb') since you are writing str/bytes.

@@ +86,5 @@
> +                            download(url, prefix, None)
> +
> +def main():
> +    parser = create_parser()
> +    args = parser.parse_args()

Nit: I try to pass sys.argv through to parse_args() to facilitate unit testing. You don't need it here. But I encourage you to adopt this pattern going forward.
Attachment #8480558 - Flags: review?(gps) → review+
Assignee

Comment 72

5 years ago
Hopefully final version of glue patch, carrying forward r+.
Assignee

Updated

5 years ago
Attachment #8480558 - Attachment is obsolete: true
Assignee

Comment 73

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/891ab4bd814a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a75f9ad10f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8471b93cfa59
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3773aeab2f8

Assuming this doesn't actually break the build, it won't start testing until the next buildbot reconfig. At that point we may need to temporarily hide it until it's green.
Depends on: 1064145
Depends on: 1064153
Depends on: 1064154
Component: New Frameworks → General
Product: Testing → Testing
You need to log in before you can comment on or make changes to this bug.