1466514 - Download the uploaded wpt-manifest

Status: RESOLVED FIXED

(Testing :: web-platform-tests, enhancement)

Comment 1

[Ahilya Sinha [:cactusmachete]]

Attachment: Bug 1466514 - download the uploaded wpt-manifest,

Review commit: https://reviewboard.mozilla.org/r/248898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248898/

Comment 2

[James Graham [:jgraham]]

Comment on attachment 8983035 [details]
Bug 1466514 - download the uploaded wpt-manifest,

https://reviewboard.mozilla.org/r/248898/#review255018

This generally looks very good, just some minor issues.

::: testing/web-platform/mach_commands.py:373
(Diff revision 1)
>      @Command("web-platform-tests-update",
>               category="testing",
>               parser=create_parser_update)
>      def update_web_platform_tests(self, **params):
>          self.setup()
> -        self.virtualenv_manager.install_pip_package('html5lib==1.0.1')
> +        self.virtualenv_manager.install_pip_package('html5lib==0.99')

Looks like you reverted some changes here.

::: testing/web-platform/manifestdownload.py:17
(Diff revision 1)
> +from six.moves.urllib.request import urlopen
> +
> +def abs_path(path):
> +    return os.path.abspath(os.path.expanduser(path))
> +
> +def hg_commits(repo_root):

Two blank lines between functions (I know the rest of the code isn't great about this, but we are slowly movin toward PEP8 style everywhere).

::: testing/web-platform/manifestdownload.py:44
(Diff revision 1)
> +
> +    for revision in commits:
> +        req = requests.get(cset_url.format(changeset=revision),
> +                           headers={'Accept': 'application/json'})
> +
> +        if req.status_code not in range(200, 300):

You can use `req.raise_for_status()` here.

::: testing/web-platform/manifestdownload.py:71
(Diff revision 1)
> +    url = url_func(logger, commits) + "/artifacts/public/"
> +
> +    man_url= url + "manifest.json.gz"
> +    moz_man_url= url + "moz_manifest.json.gz"
> +
> +    return ( _download(logger, os.path.join(wpt_dir, "meta", "MANIFEST.json"), man_url) and

So, this is OK for now, but eventually we'll want to put this somewhere in the objdir under self.top_obj_dir/testing/web-platform, and teach wptrunner to look there for a manifest (there could be a little complexity there; we currently have a .ini file with the per-manifest configuration, but there isn't any way to pass the obj dir root in to that file, and the command line only allows per-manifest configuration).

::: testing/web-platform/manifestdownload.py:92
(Diff revision 1)
> +    if resp.code != 200:
> +        logger.warning("Downloading pregenerated manifest failed; got HTTP status %d" %
> +                       resp.code)
> +        return False
> +
> +    gzf = gzip.GzipFile(fileobj=io.BytesIO(resp.read()))

Does it work to pass `resp` directly into `GzipFile`? I guess possibly not if it requires arbitary seeks, in which case buffering the whole thing in a `BytesIO` is probably necessary.

::: testing/web-platform/meta/MANIFEST.json:519956
(Diff revision 1)
>    "css/css-scoping/shadow-assign-dynamic-001.html": [
>     "c57e0fd5aa5be63e1cadf65a4e382798c5e05ec4",
>     "reftest"
>    ],
>    "css/css-scoping/shadow-at-import.html": [
> -   "40f2606177ad3143774d97060ac1bbfa9743801f",
> +   "67295000ad3c24c2d9ab0ac556d34758f3ce654c",

Let's skip these changes in this patch.

Comment 3

[Ahilya Sinha [:cactusmachete]]

Comment on attachment 8983035 [details]
Bug 1466514 - download the uploaded wpt-manifest,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248898/diff/1-2/

Comment 4

[James Graham [:jgraham]]

Comment on attachment 8983035 [details]
Bug 1466514 - download the uploaded wpt-manifest,

https://reviewboard.mozilla.org/r/248898/#review255804

I think this is fine, just some indent weirdness to sort out.

Thanks for the fixes!

::: testing/web-platform/manifestdownload.py:21
(Diff revisions 1 - 2)
>  
> +
>  def hg_commits(repo_root):
>      hg = Mercurial.get_func(repo_root)
> -    return [item for item in hg("log", "-l50", "--template={node}\n", "testing/web-platform/tests/",
> -                "testing/web-platform/mozilla/tests").split("\n") if item]
> +    return [item for item in hg("log", "-fl50", "--template={node}\n",
> +                "testing/web-platform/tests/", "testing/web-platform/mozilla/tests").split("\n")

This indent looks a bit weird, I think it should be
```
return [item for item in hg("log", "-fl50", "--template={node}\n",
        "testing/web-platform/tests/", "testing/web-platform/mozilla/tests").split("\n")
        if item]
```

so that the subsequent lines align with the first character after the opening `[`

(good catch adding `-f` btw).

::: testing/web-platform/manifestdownload.py:59
(Diff revisions 1 - 2)
>              return tc_url.format(changeset=cset)
>  
> -    # TODO: using fallback can cause issues if we change the manifest upload format, till the next latest source is released.
> -    # Can just use a fallback from some other, more frequent release, though. Or check in a list of releases for most recent?
> -    logger.info("Can't find a commit-specific manifest so just using the most recent one")
> -    return "https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central.latest.source.manifest-upload"
> +    logger.info("Can't find a commit-specific manifest so just using the most"
> +                        "recent one")
> +    return ("https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central."
> +                        "latest.source.manifest-upload")

Again, the indent is a little odd; generally continuation lines should align with the first line.

::: testing/web-platform/manifestdownload.py:124
(Diff revisions 1 - 2)
>      return parser
>  
>  
>  def download_from_taskcluster(logger, wpt_dir, repo_root, force=False):
> -    return download_manifest(logger, wpt_dir, lambda: hg_commits(repo_root), taskcluster_url, force)
> +    return download_manifest(logger, wpt_dir, lambda: hg_commits(repo_root),
> +                        taskcluster_url, force)

Here too, I would expect
```
return download_manifest(logger, wpt_dir, lambda: hg_commits(repo_root),
                         taskcluster_url, force)
```

Comment 5

[Ahilya Sinha [:cactusmachete]]

Comment on attachment 8983035 [details]
Bug 1466514 - download the uploaded wpt-manifest,

https://reviewboard.mozilla.org/r/248898/#review255804

> This indent looks a bit weird, I think it should be
> ```
> return [item for item in hg("log", "-fl50", "--template={node}\n",
>         "testing/web-platform/tests/", "testing/web-platform/mozilla/tests").split("\n")
>         if item]
> ```
> 
> so that the subsequent lines align with the first character after the opening `[`
> 
> (good catch adding `-f` btw).

Oh, sorry. 
Not used to PEP8 and the things that come along with it.

Comment 6

[Ahilya Sinha [:cactusmachete]]

Comment on attachment 8983035 [details]
Bug 1466514 - download the uploaded wpt-manifest,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248898/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e79ad2cef7ad
download the uploaded wpt-manifest, r=jgraham

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e79ad2cef7ad