Create a lint that checks whether the wpt manifest has unexpected changes.

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jgraham, Assigned: jgraham)

Tracking

(Depends on 2 bugs)

unspecified
mozilla51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(4 attachments)

This is intended to stop the common problem where people don't correctly build the manifest after making a wpt change.
Comment on attachment 8791309 [details]
Bug 1302796 - Add integration between structured logging and lints,

https://reviewboard.mozilla.org/r/78756/#review77424

This is clever! Didn't have time to finish looking at everything today, but wanted to publish the comments I had. Will pick it up again tomorrow.

::: python/mozlint/mozlint/types.py:10
(Diff revision 1)
>  from __future__ import unicode_literals
>  
>  import re
>  from abc import ABCMeta, abstractmethod
>  
> +from mozlog.reader import LogHandler

Maybe this should go in the \_lint function it's being used in to avoid a hard dependency on mozlog.

::: python/mozlint/mozlint/types.py:119
(Diff revision 1)
> +
> +class StructuredLogType(ExternalType):
> +    def _lint(self, files, linter, **lintargs):
> +        payload = linter['payload']
> +        handler = LintHandler(linter)
> +        payload(files, handler, **lintargs)

If you catch KeyboardInterrupt and return handler.results anyway, mozlint will be able to display the partial results up to that point. I've found this feature to be very nice with flake8.

::: python/mozlint/mozlint/types.py:126
(Diff revision 1)
> +
>  supported_types = {
>      'string': StringType(),
>      'regex': RegexType(),
>      'external': ExternalType(),
> +    'structured_log': StructuredLogType()

Please add a test in mozlint/test/test_types.py. Note there's a patch on autoland currently that re-writes that test to use pytest, so make sure you either wait for it to merge or pull in that change locally.

::: testing/mozbase/mozlog/mozlog/logtypes.py:189
(Diff revision 1)
> +    def convert(self, data):
> +        if len(data) != len(self.item_types):
> +            raise ValueError("Expected %i items got %i" % (len(self.item_types), len(data)))
> +        return tuple(item_type.convert(value)
> +                     for item_type, value in zip(self.item_types, data))
> +            

nit: whitespace
Comment on attachment 8791309 [details]
Bug 1302796 - Add integration between structured logging and lints,

https://reviewboard.mozilla.org/r/78756/#review77424

> Maybe this should go in the \_lint function it's being used in to avoid a hard dependency on mozlog.

What's the reason to avoid such a dependency? Is this used outside of gecko?

In the longer term I would like mozlint to always output structured logs so there would be a dependency there anyway.
Comment on attachment 8791309 [details]
Bug 1302796 - Add integration between structured logging and lints,

https://reviewboard.mozilla.org/r/78756/#review77556

::: python/mozlint/mozlint/types.py:115
(Diff revision 1)
> +
> +    def lint(self, data):
> +        self.results.append(result.from_linter(self.linter, data))
> +
> +
> +class StructuredLogType(ExternalType):

Please add this to the documentation:
https://dxr.mozilla.org/mozilla-central/source/tools/lint/docs/create.rst#24

::: python/mozlint/mozlint/types.py:116
(Diff revision 1)
> +    def _lint(self, files, linter, **lintargs):
> +        payload = linter['payload']
> +        handler = LintHandler(linter)
> +        payload(files, handler, **lintargs)
> +        return handler.results

You could stuff 'handler' into lintargs and call ExternalType._lint(). Not sure if that's much cleaner or not though...
Comment on attachment 8791309 [details]
Bug 1302796 - Add integration between structured logging and lints,

https://reviewboard.mozilla.org/r/78756/#review77424

> What's the reason to avoid such a dependency? Is this used outside of gecko?
> 
> In the longer term I would like mozlint to always output structured logs so there would be a dependency there anyway.

Fair enough, it's not used outside gecko. And agree that it would be nice to output with mozlog eventually. At least add mozlog to the setup.py though.
Comment on attachment 8791309 [details]
Bug 1302796 - Add integration between structured logging and lints,

https://reviewboard.mozilla.org/r/78756/#review77560

Setting r- for now to get re-flagged later.
Attachment #8791309 - Flags: review?(ahalberstadt) → review-
Comment on attachment 8791310 [details]
Bug 1302796 - Add --check-clean flag to mach manifest update and mozlint integration,

https://reviewboard.mozilla.org/r/78758/#review77558

::: tools/lint/wpt_manifest.lint:17
(Diff revision 1)
> +top_src_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir, os.pardir))
> +wpt_dir = os.path.join(top_src_dir, "testing", "web-platform")
> +print wpt_dir
> +manifestupdate = imp.load_source("manifestupdate",
> +                                 os.path.join(wpt_dir, "manifestupdate.py"))

topsrcdir lives in kwargs['root'] below, so you can move all this logic under the 'lint' function and use that.

Also leftover debug statement.

::: tools/lint/wpt_manifest.lint:24
(Diff revision 1)
> +def lint(files, handler, **kwargs):
> +    logger = wptlogging.setup({}, {"mach": sys.stdout})
> +    logger.add_handler(handler)
> +    manifestupdate.update(logger, wpt_dir, True)

What if the StructuredLogType either recieves a structured logger instance or creates one automatically if none specified:

    def lint(files, logger, **kwargs):
        ...
        
    LINTER = {
        'payload': lint,
        'logger': wptlogging.setup(...) # optional
    }
    
Seems a bit cleaner, but feel free to close this issue if you disagree.
Attachment #8791310 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8791311 [details]
Bug 1302796 - Add wpt manifest lint to taskcluster,

https://reviewboard.mozilla.org/r/78760/#review77564

::: taskcluster/ci/source-check/mozlint.yml:114
(Diff revision 1)
> +    when:
> +        files-changed:
> +            - 'testing/web-platform/tests/**'
> +            - 'testing/web-platform/mozilla/tests/**'
> +            - 'testing/web-platform/meta/MANIFEST.json'
> +            - 'testing/web-platform/meta/MANIFEST.json'

I see that this runs on a superset of files to the W task. Would it be better to merge these two tasks with:

    ./mach lint -l wpt -l wpt_manifest ?

The downside is that the wpt linter will now get run when 'testing/web-platform/mozilla/tests' change. Though it won't actually lint those files, it'll just run unecessarily. Feel free to close this if you prefer to keep them separate.
Attachment #8791311 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8791309 [details]
Bug 1302796 - Add integration between structured logging and lints,

https://reviewboard.mozilla.org/r/78756/#review77556

> You could stuff 'handler' into lintargs and call ExternalType._lint(). Not sure if that's much cleaner or not though...

So I eventually just decided that inheritance here isn't really worthwhile.
Comment on attachment 8791309 [details]
Bug 1302796 - Add integration between structured logging and lints,

https://reviewboard.mozilla.org/r/78756/#review77624

Thanks, looks good!

::: python/mozlint/test/linters/structured.lint:10
(Diff revision 3)
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import mozlog
> +
> +
> +def lint(files, handler, **kwargs):

Doesn't this pass in a logger now?

::: python/mozlint/test/linters/structured.lint:12
(Diff revision 3)
> +    print dir(mozlog.structuredlog.StructuredLogger)
> +    print(dir(logger))

Leftover debug statements?

::: python/mozlint/test/linters/structured.lint:19
(Diff revision 3)
> +    logger.add_handler(handler)
> +    for path in files:
> +        with open(path, 'r') as fh:
> +            for i, line in enumerate(fh.readlines()):
> +                if 'foobar' in line:
> +                    logger.info("foo")

ditto
Attachment #8791309 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8791310 [details]
Bug 1302796 - Add --check-clean flag to mach manifest update and mozlint integration,

https://reviewboard.mozilla.org/r/78758/#review77754

Looks fine.

::: tools/lint/wpt_manifest.lint:7
(Diff revision 3)
> +import json
> +import os
> +import imp
> +import sys

Sort imports
Attachment #8791310 - Flags: review?(Ms2ger) → review+
Comment on attachment 8791311 [details]
Bug 1302796 - Add wpt manifest lint to taskcluster,

https://reviewboard.mozilla.org/r/78760/#review77758

::: python/mozlint/mozlint/types.py:114
(Diff revision 3)
>      def __init__(self, linter):
>          self.linter = linter
>          self.results = []
>  
>      def lint(self, data):
> -        self.results.append(result.from_linter(self.linter, data))
> +        self.results.append(result.from_linter(self.linter, **data))

This should perhaps be in the other commit.

::: taskcluster/ci/source-check/mozlint.yml:93
(Diff revision 3)
> +            - 'testing/web-platform/meta/MANIFEST.json'
> +            - 'testing/web-platform/meta/MANIFEST.json'

Duplicate line
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/d3fc2851c3fc
Add integration between structured logging and lints, r=ahal
https://hg.mozilla.org/integration/autoland/rev/f06b81832d6f
Add --check-clean flag to mach manifest update and mozlint integration, r=ahal,Ms2ger
https://hg.mozilla.org/integration/autoland/rev/efd117281a02
Add wpt manifest lint to taskcluster, r=ahal
Posted patch 1302796.diffSplinter Review
Attached fix for the manifest.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f80baed6263d
Remove some bogus changes in the wpt manifest, a=testonly
Duplicate of this bug: 1272276
Assignee: nobody → james
It looks like this broke "mach web-platform-tests-create".  See bug 1333838 comment 12.
Flags: needinfo?(james)
I filed bug 1334297 on the web-platform-tests-create breakage.
Flags: needinfo?(james)
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.