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

RESOLVED FIXED in Firefox 51

Status

Testing
Lint
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: jgraham, Assigned: jgraham)

Tracking

(Depends on: 2 bugs)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

a year ago
This is intended to stop the common problem where people don't correctly build the manifest after making a wpt change.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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
(Assignee)

Comment 5

a year ago
mozreview-review-reply
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 6

a year ago
mozreview-review
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 7

a year ago
mozreview-review-reply
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 8

a year ago
mozreview-review
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 9

a year ago
mozreview-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 10

a year ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

a year ago
mozreview-review
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 19

a year ago
mozreview-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 20

a year ago
mozreview-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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

a year ago
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
(Assignee)

Comment 28

a year ago
Created attachment 8791931 [details] [diff] [review]
1302796.diff

Attached fix for the manifest.

Comment 29

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f80baed6263d
Remove some bogus changes in the wpt manifest, a=testonly

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d3fc2851c3fc
https://hg.mozilla.org/mozilla-central/rev/f06b81832d6f
https://hg.mozilla.org/mozilla-central/rev/efd117281a02
https://hg.mozilla.org/mozilla-central/rev/f80baed6263d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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)
Depends on: 1334811
Depends on: 1334297
You need to log in before you can comment on or make changes to this bug.