Closed Bug 1334767 Opened 3 years ago Closed 3 years ago

Make wpt lint ignore hash-only changes in the manifest

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jgraham, Unassigned)

Details

Attachments

(1 file)

Jobs keep getting backed out for unimportant lint failures. Although this is a thing that we should fix in some better way, a good first step is to only fail the lint if something that will actually affect tests changed.
Comment on attachment 8831411 [details]
Bug 1334767 - Make wpt manifest lint only flag changes that will affect the tests,

https://reviewboard.mozilla.org/r/107992/#review109288

::: testing/web-platform/manifestupdate.py:42
(Diff revision 1)
> +    do_delayed_imports(wpt_dir)
> +
>      if check_clean:
> -        old_manifests = {}
> -        for data in test_paths.itervalues():
> -            path = os.path.join(data["metadata_path"], "MANIFEST.json")
> +        return _check_clean(logger, test_paths)
> +
> +    else:

I don't like else-after-return.

(It also makes me wonder if there's a point in threading those through the same function.)

::: testing/web-platform/manifestupdate.py:63
(Diff revision 1)
> +    for url_base, paths in test_paths.iteritems():
> +        manifest_path = os.path.join(paths["metadata_path"], "MANIFEST.json")
> +        old_manifest = manifest.manifest.load(paths["tests_path"], manifest_path)
> +        new_manifest = manifest.manifest.Manifest.from_json(paths["tests_path"],
> +                                                            old_manifest.to_json())
> +        manifest.update.update(paths["tests_path"], new_manifest, working_copy=True)

You've used `paths["tests_path"]` thrice; introduce a local?

::: testing/web-platform/manifestupdate.py:67
(Diff revision 1)
> +                                                            old_manifest.to_json())
> +        manifest.update.update(paths["tests_path"], new_manifest, working_copy=True)
> +        manifests_by_path[manifest_path] = (old_manifest, new_manifest)
>  
> -        if check_clean:
> -            clean = diff_manifests(logger, old_manifests)
> +    for manifest_path, manifests in manifests_by_path.iteritems():
> +        if not diff_manifests(logger, manifest_path, *manifests):

Using `*args` seems unnecessarily cryptic. Use `for manifest_path, (old_manifest, new_manifest) in ...`?

::: testing/web-platform/manifestupdate.py:75
(Diff revision 1)
>  
> -def diff_manifests(logger, old_manifests):
> -    logger.info("Diffing old and new manifests")
> -    import difflib
>  
> +def diff_manifests(logger, manifest_path, old_manifest, new_manifest):

Needs a doc comment.

::: testing/web-platform/manifestupdate.py:78
(Diff revision 1)
> -    import difflib
>  
> +def diff_manifests(logger, manifest_path, old_manifest, new_manifest):
> +    logger.info("Diffing old and new manifests %s" % manifest_path)
> +    old_items, new_items = defaultdict(set), defaultdict(set)
>      clean = True

Move this down to just before the `added_paths` loop?

::: testing/web-platform/manifestupdate.py:79
(Diff revision 1)
>  
> +def diff_manifests(logger, manifest_path, old_manifest, new_manifest):
> +    logger.info("Diffing old and new manifests %s" % manifest_path)
> +    old_items, new_items = defaultdict(set), defaultdict(set)
>      clean = True
> -    for path, old in old_manifests.iteritems():
> +    for manifest, items in zip((old_manifest, new_manifest), (old_items, new_items)):

How about `[(old_manifest, old_items), (new_manifest, new_items)]`?

::: testing/web-platform/manifestupdate.py:111
(Diff revision 1)
> -                        logged = True
> -                        logger.lint_error(path=path,
> -                                          message="Manifest changed",
> -                                          lineno=(old_0 + 1),
> -                                          source="\n".join(old[old_0:old_1]),
> -                                          linter="wpt-manifest")
> +        old_tests = old_items[path]
> +        new_tests = new_items[path]
> +        added_tests = new_tests - old_tests
> +        removed_tests = old_tests - new_tests
> +        if added_tests or removed_tests:
> +            log_error(logger, manifest_path, "%s changed test types or metadata" % path)

You forgot `clean`?
Attachment #8831411 - Flags: review?(Ms2ger) → review+
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/a16de69b0207
Make wpt manifest lint only flag changes that will affect the tests, r=Ms2ger
https://hg.mozilla.org/mozilla-central/rev/a16de69b0207
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.