Closed
Bug 1466514
Opened 6 years ago
Closed 6 years ago
Download the uploaded wpt-manifest
Categories
(Testing :: web-platform-tests, enhancement)
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: cactusmachete, Assigned: cactusmachete)
Details
Attachments
(1 file)
No description provided.
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
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.
Attachment #8983035 -
Flags: review?(james) → review-
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
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) ```
Attachment #8983035 -
Flags: review?(james) → review+
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Pushed by james@hoppipolla.co.uk: https://hg.mozilla.org/integration/autoland/rev/e79ad2cef7ad download the uploaded wpt-manifest, r=jgraham
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e79ad2cef7ad
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•