From c6a4bcb4b43bdd82cfac805591b08141778137a8 Mon Sep 17 00:00:00 2001 From: Sam Sneddon Date: Fri, 17 May 2019 11:01:42 +0000 Subject: [PATCH] Bug 1541707 [wpt PR 16135] - Fix manifest regeneration differences, a=testonly Automatic update from web-platform-tests Don't bother downloading manifest if we're rebuilding -- Ensure items are removed from json_data when set -- Bump manifest version to v6 This is needed as we need to force regeneration to fix the bugs fixed in cb839d72f8dbb28f10775377771e4799deb79928 and f759f0b3a8cb7457adbf30dc0734d3a158ce4990. -- Get rid of special-casing for old manifests URLs are guaranteed to be relative paths, and script_metadata is guaranteed to exist if it's non-empty and never needs recomputed. -- __bool__ doesn't exist; we want __nonzero__ -- wpt-commits: 55b021a2529c0bde4e488b89af7fdaa63a3037ff, 3f250e3a22634ba043320b562cf3bc27413afdc7, 398466b252b3da92204001db8c37b452cb22e3c8, fc73997d2bc182c7d806fc8cd73eabc19fe466bb, ab9bf5190a3152cef6eda56404ba623356268ea7 wpt-pr: 16135 --- testing/web-platform/tests/tools/manifest/item.py | 23 ++----- .../web-platform/tests/tools/manifest/manifest.py | 10 ++- .../tests/tools/manifest/tests/test_item.py | 4 +- .../tests/tools/manifest/tests/test_manifest.py | 79 +++++++++++++--------- .../web-platform/tests/tools/manifest/update.py | 2 +- 5 files changed, 62 insertions(+), 56 deletions(-) diff --git a/testing/web-platform/tests/tools/manifest/item.py b/testing/web-platform/tests/tools/manifest/item.py index 5e313faa751e..b6d33e8452cd 100644 --- a/testing/web-platform/tests/tools/manifest/item.py +++ b/testing/web-platform/tests/tools/manifest/item.py @@ -63,28 +63,19 @@ class URLManifestItem(ManifestItem): def __init__(self, tests_root, path, url_base, url, **extras): super(URLManifestItem, self).__init__(tests_root, path) + assert url_base[0] == "/" self.url_base = url_base + assert url[0] != "/" self._url = url self._extras = extras @property - def _source_file(self): - """create a SourceFile for the item""" - from .sourcefile import SourceFile - return SourceFile(self._tests_root, self.path, self.url_base) - - @property def id(self): return self.url @property def url(self): # we can outperform urljoin, because we know we just have path relative URLs - if self._url[0] == "/": - # TODO: MANIFEST6 - # this is actually a bug in older generated manifests, _url shouldn't - # be an absolute path - return self._url if self.url_base == "/": return "/" + self._url return urljoin(self.url_base, self._url) @@ -125,12 +116,7 @@ class TestharnessTest(URLManifestItem): @property def script_metadata(self): - if "script_metadata" in self._extras: - return self._extras["script_metadata"] - else: - # TODO: MANIFEST6 - # this branch should go when the manifest version is bumped - return self._source_file.script_metadata + return self._extras.get("script_metadata") def to_json(self): rv = super(TestharnessTest, self).to_json() @@ -140,8 +126,7 @@ class TestharnessTest(URLManifestItem): rv[-1]["testdriver"] = self.testdriver if self.jsshell: rv[-1]["jsshell"] = True - if self.script_metadata is not None: - # we store this even if it is [] to avoid having to read the source file + if self.script_metadata: rv[-1]["script_metadata"] = self.script_metadata return rv diff --git a/testing/web-platform/tests/tools/manifest/manifest.py b/testing/web-platform/tests/tools/manifest/manifest.py index afbc92ae8ab2..bfe57c7823b2 100644 --- a/testing/web-platform/tests/tools/manifest/manifest.py +++ b/testing/web-platform/tests/tools/manifest/manifest.py @@ -15,7 +15,7 @@ try: except ImportError: fast_json = json -CURRENT_VERSION = 5 +CURRENT_VERSION = 6 class ManifestError(Exception): @@ -68,8 +68,8 @@ class TypeData(object): self.load(key) return self.data[key] - def __bool__(self): - return bool(self.data) + def __nonzero__(self): + return bool(self.data) or bool(self.json_data) def __len__(self): rv = len(self.data) @@ -86,6 +86,10 @@ class TypeData(object): raise KeyError def __setitem__(self, key, value): + if self.json_data is not None: + path = from_os_path(key) + if path in self.json_data: + del self.json_data[path] self.data[key] = value def __contains__(self, key): diff --git a/testing/web-platform/tests/tools/manifest/tests/test_item.py b/testing/web-platform/tests/tools/manifest/tests/test_item.py index a73d46be6857..385180b25ce1 100644 --- a/testing/web-platform/tests/tools/manifest/tests/test_item.py +++ b/testing/web-platform/tests/tools/manifest/tests/test_item.py @@ -14,7 +14,7 @@ from ..item import URLManifestItem "a.b.serviceworker.c.d", ]) def test_url_https(path): - m = URLManifestItem("/foobar", "/" + path, "/", "/foo.bar/" + path) + m = URLManifestItem("/foo", "bar/" + path, "/", "bar/" + path) assert m.https is True @@ -36,6 +36,6 @@ def test_url_https(path): "a.serviceworkerb.c", ]) def test_url_not_https(path): - m = URLManifestItem("/foobar", "/" + path, "/", "/foo.bar/" + path) + m = URLManifestItem("/foo", "bar/" + path, "/", "bar/" + path) assert m.https is False diff --git a/testing/web-platform/tests/tools/manifest/tests/test_manifest.py b/testing/web-platform/tests/tools/manifest/tests/test_manifest.py index 4a56b633f562..22335564b7c8 100644 --- a/testing/web-platform/tests/tools/manifest/tests/test_manifest.py +++ b/testing/web-platform/tests/tools/manifest/tests/test_manifest.py @@ -15,7 +15,7 @@ def SourceFileWithTest(path, hash, cls, **kwargs): if cls == item.SupportFile: test = cls("/foobar", path, **kwargs) else: - test = cls("/foobar", path, "/", utils.rel_path_to_url(path), **kwargs) + test = cls("/foobar", path, "/", utils.from_os_path(path), **kwargs) s.manifest_items = mock.Mock(return_value=(cls.item_type, [test])) return s @@ -58,11 +58,11 @@ def sourcefile_strategy(draw): ref_path = draw(rel_dir_file_path()) h.assume(path != ref_path) ref_eq = draw(hs.sampled_from(["==", "!="])) - test = cls("/foobar", path, "/", utils.rel_path_to_url(path), references=[(utils.rel_path_to_url(ref_path), ref_eq)]) + test = cls("/foobar", path, "/", utils.from_os_path(path), references=[(utils.from_os_path(ref_path), ref_eq)]) elif cls is item.SupportFile: test = cls("/foobar", path) else: - test = cls("/foobar", path, "/", utils.rel_path_to_url(path)) + test = cls("/foobar", path, "/", utils.from_os_path(path)) s.manifest_items = mock.Mock(return_value=(cls.item_type, [test])) return s @@ -111,42 +111,36 @@ def test_manifest_to_json_forwardslash(): 'paths': { 'a/b': ('0000000000000000000000000000000000000000', 'testharness') }, - 'version': 5, + 'version': 6, 'url_base': '/', 'items': { 'testharness': { - 'a/b': [['/a/b', {}]] + 'a/b': [['a/b', {}]] } } } +@pytest.mark.skipif(os.sep != "\\", reason="backslash path") def test_manifest_to_json_backslash(): m = manifest.Manifest() s = SourceFileWithTest("a\\b", "0"*40, item.TestharnessTest) - if os.path.sep == "\\": - assert m.update([(s, True)]) is True - - assert m.to_json() == { - 'paths': { - 'a/b': ('0000000000000000000000000000000000000000', 'testharness') - }, - 'version': 5, - 'url_base': '/', - 'items': { - 'testharness': { - 'a/b': [['/a/b', {}]] - } + assert m.update([(s, True)]) is True + + assert m.to_json() == { + 'paths': { + 'a/b': ('0000000000000000000000000000000000000000', 'testharness') + }, + 'version': 6, + 'url_base': '/', + 'items': { + 'testharness': { + 'a/b': [['a/b', {}]] } } - else: - with pytest.raises(ValueError): - # one of these must raise ValueError - # the first must return True if it doesn't raise - assert m.update([(s, True)]) is True - m.to_json() + } def test_manifest_from_json_backslash(): @@ -154,11 +148,11 @@ def test_manifest_from_json_backslash(): 'paths': { 'a\\b': ('0000000000000000000000000000000000000000', 'testharness') }, - 'version': 5, + 'version': 6, 'url_base': '/', 'items': { 'testharness': { - 'a\\b': [['/a/b', {}]] + 'a\\b': [['a/b', {}]] } } } @@ -294,8 +288,8 @@ def test_iterpath(): m = manifest.Manifest() sources = [SourceFileWithTest("test1", "0"*40, item.RefTestNode, references=[("/test1-ref", "==")]), - SourceFileWithTests("test2", "1"*40, item.TestharnessTest, [("/test2-1.html",), - ("/test2-2.html",)]), + SourceFileWithTests("test2", "1"*40, item.TestharnessTest, [("test2-1.html",), + ("test2-2.html",)]), SourceFileWithTest("test3", "0"*40, item.TestharnessTest)] m.update([(s, True) for s in sources]) @@ -308,8 +302,8 @@ def test_filter(): m = manifest.Manifest() sources = [SourceFileWithTest("test1", "0"*40, item.RefTestNode, references=[("/test1-ref", "==")]), - SourceFileWithTests("test2", "0"*40, item.TestharnessTest, [("/test2-1.html",), - ("/test2-2.html",)]), + SourceFileWithTests("test2", "0"*40, item.TestharnessTest, [("test2-1.html",), + ("test2-2.html",)]), SourceFileWithTest("test3", "0"*40, item.TestharnessTest)] m.update([(s, True) for s in sources]) @@ -317,7 +311,7 @@ def test_filter(): def filter(it): for test in it: - if test[0] in ["/test2-2.html", "/test3"]: + if test[0] in ["test2-2.html", "test3"]: yield test filtered_manifest = manifest.Manifest.from_json("/", json, types=["testharness"], meta_filters=[filter]) @@ -407,3 +401,26 @@ def test_update_from_json(): test1 = s1.manifest_items()[1][0] assert list(m) == [("testharness", test1.path, {test1})] + + +def test_update_from_json_modified(): + # Create the original manifest + m = manifest.Manifest() + s1 = SourceFileWithTest("test1", "0"*40, item.TestharnessTest) + m.update([(s1, True)]) + json_str = m.to_json() + + # Reload it from JSON + m = manifest.Manifest.from_json("/", json_str) + + # Update it with timeout="long" + s2 = SourceFileWithTest("test1", "1"*40, item.TestharnessTest, timeout="long") + m.update([(s2, True)]) + json_str = m.to_json() + assert json_str == { + 'items': {'testharness': {'test1': [['test1', {"timeout": "long"}]]}}, + 'paths': {'test1': ('1111111111111111111111111111111111111111', + 'testharness')}, + 'url_base': '/', + 'version': 6 + } diff --git a/testing/web-platform/tests/tools/manifest/update.py b/testing/web-platform/tests/tools/manifest/update.py index 217d767cece2..321cfebe2a60 100755 --- a/testing/web-platform/tests/tools/manifest/update.py +++ b/testing/web-platform/tests/tools/manifest/update.py @@ -33,7 +33,7 @@ def update_from_cli(**kwargs): path = kwargs["path"] assert tests_root is not None - if kwargs["download"]: + if not kwargs["rebuild"] and kwargs["download"]: download_from_github(path, tests_root) manifest.load_and_update(tests_root, -- 2.11.4.GIT