Bug 1497898 - Fix the cache lifecycle, r=ato
authorJames Graham <james@hoppipolla.co.uk>
Fri, 16 Nov 2018 18:48:26 +0000
changeset 503254 fe2d962a8ed2e722266a417df17f7085aeabad3b
parent 503253 10e90d3295ee7a29b094b42912cdb584dfd47e98
child 503255 daf29e8abb1eb0f5685ed9d228afeb6b4a0c4327
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato
bugs1497898
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1497898 - Fix the cache lifecycle, r=ato The caches weren't being invalidated when the manifest itself changed. To fix this the manifest itself has to be written before the cache and the cache has to include data about the manifest that it's associated with (the mtime and path are used for this purpose). To make all this work requires a single method that can load the manifest, update it, write the manifest and write the caches. Therefore we introduce a single load_and_update method that is intended to replace all previous use of the load() or update() methods (and as a bonus handles manifest version mismatches in a single place). Depends on D8227 Differential Revision: https://phabricator.services.mozilla.com/D8228
testing/web-platform/tests/tools/gitignore/gitignore.py
testing/web-platform/tests/tools/manifest/item.py
testing/web-platform/tests/tools/manifest/manifest.py
testing/web-platform/tests/tools/manifest/update.py
testing/web-platform/tests/tools/manifest/vcs.py
testing/web-platform/tests/tools/wpt/testfiles.py
testing/web-platform/tests/tools/wptrunner/wptrunner/testloader.py
testing/web-platform/tests/tools/wptrunner/wptrunner/tests/test_update.py
--- a/testing/web-platform/tests/tools/gitignore/gitignore.py
+++ b/testing/web-platform/tests/tools/gitignore/gitignore.py
@@ -240,8 +240,12 @@ class PathFilter(object):
             assert ".git" not in dirnames
             yield orig_dirpath, dirnames, keep_files
 
     def __call__(self, iterator):
         if self.trivial:
             return iterator
 
         return self.filter(iterator)
+
+
+def has_ignore(dirpath):
+    return os.path.exists(os.path.join(dirpath, ".gitignore"))
--- a/testing/web-platform/tests/tools/manifest/item.py
+++ b/testing/web-platform/tests/tools/manifest/item.py
@@ -37,17 +37,16 @@ class ManifestItemMeta(ABCMeta):
 class ManifestItem(object):
     __metaclass__ = ManifestItemMeta
 
     item_type = None
 
     source_file_cache = SourceFileCache()
 
     def __init__(self, source_file, manifest=None):
-        self.manifest = manifest
         self.source_file = source_file
 
     @abstractproperty
     def id(self):
         """The test's id (usually its url)"""
         pass
 
     @property
--- a/testing/web-platform/tests/tools/manifest/manifest.py
+++ b/testing/web-platform/tests/tools/manifest/manifest.py
@@ -1,13 +1,14 @@
 import itertools
 import os
 from collections import defaultdict
 from six import iteritems, iterkeys, itervalues, string_types
 
+from . import vcs
 from .item import (ManualTest, WebDriverSpecTest, Stub, RefTestNode, RefTest,
                    TestharnessTest, SupportFile, ConformanceCheckerTest, VisualTest)
 from .log import get_logger
 from .utils import from_os_path, to_os_path
 
 try:
     import ujson as json
 except ImportError:
@@ -65,17 +66,20 @@ class TypeData(object):
         if key not in self.data:
             self.load(key)
         return self.data[key]
 
     def __bool__(self):
         return bool(self.data)
 
     def __len__(self):
-        return len(self.data)
+        rv = len(self.data)
+        if self.json_data is not None:
+            rv += len(self.json_data)
+        return rv
 
     def __delitem__(self, key):
         del self.data[key]
 
     def __setitem__(self, key, value):
         self.data[key] = value
 
     def __contains__(self, key):
@@ -104,27 +108,37 @@ class TypeData(object):
     def itervalues(self):
         self.load_all()
         return itervalues(self.data)
 
     def iteritems(self):
         self.load_all()
         return iteritems(self.data)
 
+    def values(self):
+        return self.itervalues()
+
+    def items(self):
+        return self.iteritems()
+
     def load(self, key):
         """Load a specific Item given a path"""
         if self.json_data is not None:
             data = set()
             path = from_os_path(key)
             for test in iterfilter(self.meta_filters, self.json_data.get(path, [])):
                 manifest_item = self.type_cls.from_json(self.manifest,
                                                         self.tests_root,
                                                         path,
                                                         test)
                 data.add(manifest_item)
+            try:
+                del self.json_data[path]
+            except KeyError:
+                pass
             self.data[key] = data
         else:
             raise ValueError
 
     def load_all(self):
         """Load all test items in this class"""
         if self.json_data is not None:
             for path, value in iteritems(self.json_data):
@@ -150,22 +164,23 @@ class TypeData(object):
     def paths(self):
         """Get a list of all paths containing items of this type,
         without actually constructing all the items"""
         rv = set(iterkeys(self.data))
         if self.json_data:
             rv |= set(to_os_path(item) for item in iterkeys(self.json_data))
         return rv
 
+
 class ManifestData(dict):
     def __init__(self, manifest, meta_filters=None):
         """Dictionary subclass containing a TypeData instance for each test type,
         keyed by type name"""
         self.initialized = False
-        for key, value in item_classes.iteritems():
+        for key, value in iteritems(item_classes):
             self[key] = TypeData(manifest, value, meta_filters=meta_filters)
         self.initialized = True
         self.json_obj = None
 
     def __setitem__(self, key, value):
         if self.initialized:
             raise AttributeError
         dict.__setitem__(self, key, value)
@@ -280,21 +295,32 @@ class Manifest(object):
 
                 if is_new or hash_changed:
                     changed = True
 
         deleted = prev_files - seen_files
         if deleted:
             changed = True
             for rel_path in deleted:
-                _, old_type = self._path_hash[rel_path]
-                if old_type in reftest_types:
-                    reftest_changes = True
-                del self._path_hash[rel_path]
-                del self._data[old_type][rel_path]
+                if rel_path in self._path_hash:
+                    _, old_type = self._path_hash[rel_path]
+                    if old_type in reftest_types:
+                        reftest_changes = True
+                    try:
+                        del self._path_hash[rel_path]
+                    except KeyError:
+                        pass
+                    try:
+                        del self._data[old_type][rel_path]
+                    except KeyError:
+                        pass
+                else:
+                    for test_data in itervalues(self._data):
+                        if rel_path in test_data:
+                            del test_data[rel_path]
 
         if reftest_changes:
             reftests, reftest_nodes, changed_hashes = self._compute_reftests(reftest_nodes)
             self._data["reftest"].data = reftests
             self._data["reftest_node"].data = reftest_nodes
             self._path_hash.update(changed_hashes)
 
         return changed
@@ -330,17 +356,17 @@ class Manifest(object):
 
     def to_json(self):
         out_items = {
             test_type: {
                 from_os_path(path):
                 [t for t in sorted(test.to_json() for test in tests)]
                 for path, tests in iteritems(type_paths)
             }
-            for test_type, type_paths in self._data.iteritems() if type_paths
+            for test_type, type_paths in iteritems(self._data) if type_paths
         }
         rv = {"url_base": self.url_base,
               "paths": {from_os_path(k): v for k, v in iteritems(self._path_hash)},
               "items": out_items,
               "version": CURRENT_VERSION}
         return rv
 
     @classmethod
@@ -365,16 +391,21 @@ class Manifest(object):
             self._data[test_type].set_json(tests_root, type_paths)
 
         return self
 
 
 def load(tests_root, manifest, types=None, meta_filters=None):
     logger = get_logger()
 
+    logger.warning("Prefer load_and_update instead")
+    return _load(logger, tests_root, manifest, types, meta_filters)
+
+
+def _load(logger, tests_root, manifest, types=None, meta_filters=None):
     # "manifest" is a path or file-like object.
     if isinstance(manifest, string_types):
         if os.path.exists(manifest):
             logger.debug("Opening manifest at %s" % manifest)
         else:
             logger.debug("Creating new manifest at %s" % manifest)
         try:
             with open(manifest) as f:
@@ -390,15 +421,57 @@ def load(tests_root, manifest, types=Non
         return rv
 
     return Manifest.from_json(tests_root,
                               json.load(manifest),
                               types=types,
                               meta_filters=meta_filters)
 
 
+def load_and_update(tests_root,
+                    manifest_path,
+                    url_base,
+                    update=True,
+                    rebuild=False,
+                    metadata_path=None,
+                    cache_root=None,
+                    working_copy=False,
+                    types=None,
+                    meta_filters=None,
+                    write_manifest=True):
+    logger = get_logger()
+
+    manifest = None
+    if not rebuild:
+        try:
+            manifest = _load(logger,
+                             tests_root,
+                             manifest_path,
+                             types=types,
+                             meta_filters=meta_filters)
+        except ManifestVersionMismatch:
+            logger.info("Manifest version changed, rebuilding")
+
+        if manifest is not None and manifest.url_base != url_base:
+            logger.info("Manifest url base did not match, rebuilding")
+
+    if manifest is None:
+        manifest = Manifest(url_base, meta_filters=meta_filters)
+        update = True
+
+    if update:
+        tree = vcs.get_tree(tests_root, manifest, manifest_path, cache_root,
+                            working_copy, rebuild)
+        changed = manifest.update(tree)
+        if write_manifest and changed:
+            write(manifest, manifest_path)
+        tree.dump_caches()
+
+    return manifest
+
+
 def write(manifest, manifest_path):
     dir_name = os.path.dirname(manifest_path)
     if not os.path.exists(dir_name):
         os.makedirs(dir_name)
     with open(manifest_path, "wb") as f:
         json.dump(manifest.to_json(), f, sort_keys=True, indent=1)
         f.write("\n")
--- a/testing/web-platform/tests/tools/manifest/update.py
+++ b/testing/web-platform/tests/tools/manifest/update.py
@@ -9,67 +9,45 @@ from .download import download_from_gith
 
 here = os.path.dirname(__file__)
 
 wpt_root = os.path.abspath(os.path.join(here, os.pardir, os.pardir))
 
 logger = get_logger()
 
 
-def update(tests_root, manifest, working_copy=False, cache_root=None, rebuild=False):
+def update(tests_root,
+           manifest,
+           manifest_path=None,
+           working_copy=False,
+           cache_root=None,
+           rebuild=False):
+    logger.warning("Deprecated; use manifest.load_and_update instead")
     logger.info("Updating manifest")
-    tree = None
-    if cache_root is None:
-        cache_root = os.path.join(tests_root, ".cache")
-    if not os.path.exists(cache_root):
-        try:
-            os.makedirs(cache_root)
-        except IOError:
-            cache_root = None
 
-    if not working_copy:
-        tree = vcs.Git.for_path(tests_root, manifest.url_base,
-                                cache_path=cache_root, rebuild=rebuild)
-    if tree is None:
-        tree = vcs.FileSystem(tests_root, manifest.url_base,
-                              cache_path=cache_root, rebuild=rebuild)
-
-    try:
-        return manifest.update(tree)
-    finally:
-        tree.dump_caches()
+    tree = vcs.get_tree(tests_root, manifest, manifest_path, cache_root,
+                        working_copy, rebuild)
+    return manifest.update(tree)
 
 
 def update_from_cli(**kwargs):
     tests_root = kwargs["tests_root"]
     path = kwargs["path"]
     assert tests_root is not None
 
-    m = None
-
     if kwargs["download"]:
         download_from_github(path, tests_root)
 
-    if not kwargs.get("rebuild", False):
-        try:
-            m = manifest.load(tests_root, path)
-        except manifest.ManifestVersionMismatch:
-            logger.info("Manifest version changed, rebuilding")
-            m = None
-
-    if m is None:
-        m = manifest.Manifest(kwargs["url_base"])
-
-    changed = update(tests_root,
-                     m,
-                     working_copy=kwargs["work"],
-                     cache_root=kwargs["cache_root"],
-                     rebuild=kwargs["rebuild"])
-    if changed:
-        manifest.write(m, path)
+    manifest.load_and_update(tests_root,
+                             path,
+                             kwargs["url_base"],
+                             update=True,
+                             rebuild=kwargs["rebuild"],
+                             cache_root=kwargs["cache_root"],
+                             working_copy=kwargs["work"])
 
 
 def abs_path(path):
     return os.path.abspath(os.path.expanduser(path))
 
 
 def create_parser():
     parser = argparse.ArgumentParser()
--- a/testing/web-platform/tests/tools/manifest/vcs.py
+++ b/testing/web-platform/tests/tools/manifest/vcs.py
@@ -3,19 +3,46 @@ import os
 import platform
 import stat
 import subprocess
 from collections import deque
 
 from .sourcefile import SourceFile
 
 
+def get_tree(tests_root, manifest, manifest_path, cache_root,
+             working_copy=False, rebuild=False):
+    tree = None
+    if cache_root is None:
+        cache_root = os.path.join(tests_root, ".wptcache")
+    if not os.path.exists(cache_root):
+        try:
+            os.makedirs(cache_root)
+        except IOError:
+            cache_root = None
+
+    if not working_copy:
+        tree = Git.for_path(tests_root,
+                            manifest.url_base,
+                            manifest_path=manifest_path,
+                            cache_path=cache_root,
+                            rebuild=rebuild)
+    if tree is None:
+        tree = FileSystem(tests_root,
+                          manifest.url_base,
+                          manifest_path=manifest_path,
+                          cache_path=cache_root,
+                          rebuild=rebuild)
+    return tree
+
+
 class Git(object):
-    def __init__(self, repo_root, url_base, cache_path, rebuild=False):
-        self.root = os.path.abspath(repo_root)
+    def __init__(self, repo_root, url_base, cache_path, manifest_path=None,
+                 rebuild=False):
+        self.root = repo_root
         self.git = Git.get_func(repo_root)
         self.url_base = url_base
         # rebuild is a noop for now since we don't cache anything
 
     @staticmethod
     def get_func(repo_path):
         def git(cmd, *args):
             full_cmd = ["git", cmd] + list(args)
@@ -25,21 +52,21 @@ class Git(object):
                 if platform.uname()[0] == "Windows" and isinstance(e, WindowsError):
                         full_cmd[0] = "git.bat"
                         return subprocess.check_output(full_cmd, cwd=repo_path, stderr=subprocess.STDOUT)
                 else:
                     raise
         return git
 
     @classmethod
-    def for_path(cls, path, url_base, cache_path, rebuild=False):
+    def for_path(cls, path, url_base, cache_path, manifest_path=None, rebuild=False):
         git = Git.get_func(path)
         try:
             return cls(git("rev-parse", "--show-toplevel").rstrip(), url_base, cache_path,
-                       rebuild=rebuild)
+                       manifest_path=manifest_path, rebuild=rebuild)
         except subprocess.CalledProcessError:
             return None
 
     def _local_changes(self):
         changes = {}
         cmd = ["status", "-z", "--ignore-submodules=all"]
         data = self.git(*cmd)
 
@@ -81,57 +108,57 @@ class Git(object):
                                  hash,
                                  contents=contents), True
 
     def dump_caches(self):
         pass
 
 
 class FileSystem(object):
-    def __init__(self, root, url_base, cache_path, rebuild=False):
-        self.root = root
+    def __init__(self, root, url_base, cache_path, manifest_path=None, rebuild=False):
+        from gitignore import gitignore
+        self.root = os.path.abspath(root)
         self.url_base = url_base
+        self.ignore_cache = None
+        self.mtime_cache = None
         if cache_path is not None:
-            self.mtime_cache = MtimeCache(cache_path, rebuild)
-            self.ignore_cache = GitIgnoreCache(cache_path, root, rebuild)
-        else:
-            self.mtime_cache = None
-            self.ignore_cache = None
-        from gitignore import gitignore
+            if manifest_path is not None:
+                self.mtime_cache = MtimeCache(cache_path, root, manifest_path, rebuild)
+            if gitignore.has_ignore(root):
+                self.ignore_cache = GitIgnoreCache(cache_path, root, rebuild)
         self.path_filter = gitignore.PathFilter(self.root,
                                                 extras=[".git/"],
                                                 cache=self.ignore_cache)
 
     def __iter__(self):
         mtime_cache = self.mtime_cache
-        for dirpath, dirnames, filenames in self.path_filter(walk(".")):
+        for dirpath, dirnames, filenames in self.path_filter(walk(self.root)):
             for filename, path_stat in filenames:
-                # We strip the ./ prefix off the path
                 path = os.path.join(dirpath, filename)
                 if mtime_cache is None or mtime_cache.updated(path, path_stat):
                     yield SourceFile(self.root, path, self.url_base), True
                 else:
                     yield path, False
-        self.ignore_cache.dump()
 
     def dump_caches(self):
         for cache in [self.mtime_cache, self.ignore_cache]:
             if cache is not None:
                 cache.dump()
 
 
 class CacheFile(object):
     file_name = None
 
-    def __init__(self, cache_root, rebuild=False):
+    def __init__(self, cache_root, tests_root, rebuild=False):
+        self.tests_root = tests_root
         if not os.path.exists(cache_root):
             os.makedirs(cache_root)
         self.path = os.path.join(cache_root, self.file_name)
+        self.modified = False
         self.data = self.load(rebuild)
-        self.modified = False
 
     def dump(self):
         if not self.modified:
             return
         with open(self.path, 'w') as f:
             json.dump(self.data, f, indent=1)
 
     def load(self, rebuild=False):
@@ -149,41 +176,67 @@ class CacheFile(object):
         """Check if the cached data is valid and return an updated copy of the
         cache containing only data that can be used."""
         return data
 
 
 class MtimeCache(CacheFile):
     file_name = "mtime.json"
 
+    def __init__(self, cache_root, tests_root, manifest_path, rebuild=False):
+        self.manifest_path = manifest_path
+        super(MtimeCache, self).__init__(cache_root, tests_root, rebuild=False)
+
     def updated(self, rel_path, stat):
         """Return a boolean indicating whether the file changed since the cache was last updated.
 
         This implicitly updates the cache with the new mtime data."""
         mtime = stat.st_mtime
         if mtime != self.data.get(rel_path):
             self.modified = True
             self.data[rel_path] = mtime
             return True
         return False
 
+    def check_valid(self, data):
+        if data.get("/tests_root") != self.tests_root:
+            self.modified = True
+        else:
+            if self.manifest_path is not None and os.path.exists(self.manifest_path):
+                mtime = os.path.getmtime(self.manifest_path)
+                if data.get("/manifest_path") != [self.manifest_path, mtime]:
+                    self.modified = True
+            else:
+                self.modified = True
+        if self.modified:
+            data = {}
+            data["/tests_root"] = self.tests_root
+        return data
+
+    def dump(self):
+        if self.manifest_path is None:
+            raise ValueError
+        if not os.path.exists(self.manifest_path):
+            return
+        mtime = os.path.getmtime(self.manifest_path)
+        self.data["/manifest_path"] = [self.manifest_path, mtime]
+        self.data["/tests_root"] = self.tests_root
+        super(MtimeCache, self).dump()
+
 
 class GitIgnoreCache(CacheFile):
     file_name = "gitignore.json"
 
-    def __init__(self, cache_root, ignore_path, rebuild=False):
-        self.ignore_path = ignore_path
-        super(GitIgnoreCache, self).__init__(cache_root, rebuild=False)
-
     def check_valid(self, data):
-        mtime = os.path.getmtime(self.ignore_path)
-        if data.get("/gitignore_file") != [self.ignore_path, mtime]:
+        ignore_path = os.path.join(self.tests_root, ".gitignore")
+        mtime = os.path.getmtime(ignore_path)
+        if data.get("/gitignore_file") != [ignore_path, mtime]:
             self.modified = True
             data = {}
-            data["/gitignore_file"] = [self.ignore_path, mtime]
+            data["/gitignore_file"] = [ignore_path, mtime]
         return data
 
     def __contains__(self, key):
         return key in self.data
 
     def __getitem__(self, key):
         return self.data[key]
 
@@ -238,9 +291,9 @@ def walk(root):
                 dirs.append((name, path_stat))
             else:
                 non_dirs.append((name, path_stat))
 
         yield rel_path, dirs, non_dirs
         for name, path_stat in dirs:
             new_path = join(dir_path, name)
             if not is_link(path_stat.st_mode):
-                stack.append((new_path, relpath(new_path)))
+                stack.append((new_path, relpath(new_path, root)))
--- a/testing/web-platform/tests/tools/wpt/testfiles.py
+++ b/testing/web-platform/tests/tools/wpt/testfiles.py
@@ -185,20 +185,18 @@ def _init_manifest_cache():
 
     def load(manifest_path=None):
         if manifest_path is None:
             manifest_path = os.path.join(wpt_root, "MANIFEST.json")
         if c.get(manifest_path):
             return c[manifest_path]
         # cache at most one path:manifest
         c.clear()
-        wpt_manifest = manifest.load(wpt_root, manifest_path)
-        if wpt_manifest is None:
-            wpt_manifest = manifest.Manifest()
-        update.update(wpt_root, wpt_manifest)
+        wpt_manifest = manifest.load_and_update(wpt_root, manifest_path, "/",
+                                                update=True)
         c[manifest_path] = wpt_manifest
         return c[manifest_path]
     return load
 
 
 load_manifest = _init_manifest_cache()
 
 
--- a/testing/web-platform/tests/tools/wptrunner/wptrunner/testloader.py
+++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/testloader.py
@@ -360,111 +360,57 @@ class TestFilter(object):
             include_tests = set()
             for test in tests:
                 if self.manifest.include(test):
                     include_tests.add(test)
 
             if include_tests:
                 yield test_type, test_path, include_tests
 
+
 class TagFilter(object):
     def __init__(self, tags):
         self.tags = set(tags)
 
     def __call__(self, test_iter):
         for test in test_iter:
             if test.tags & self.tags:
                 yield test
 
 
 class ManifestLoader(object):
-    def __init__(self, test_paths, force_manifest_update=False, manifest_download=False, types=None, meta_filters=None):
+    def __init__(self, test_paths, force_manifest_update=False, manifest_download=False,
+                 types=None, meta_filters=None):
         do_delayed_imports()
         self.test_paths = test_paths
         self.force_manifest_update = force_manifest_update
         self.manifest_download = manifest_download
         self.types = types
-        self.meta_filters = meta_filters or []
         self.logger = structured.get_default_logger()
+        self.meta_filters = meta_filters
         if self.logger is None:
             self.logger = structured.structuredlog.StructuredLogger("ManifestLoader")
 
     def load(self):
         rv = {}
         for url_base, paths in self.test_paths.iteritems():
             manifest_file = self.load_manifest(url_base=url_base,
                                                **paths)
             path_data = {"url_base": url_base}
             path_data.update(paths)
             rv[manifest_file] = path_data
         return rv
 
-    def create_manifest(self, manifest_path, tests_path, url_base="/"):
-        self.update_manifest(manifest_path, tests_path, url_base, recreate=True,
-                             download=self.manifest_download)
-
-    def update_manifest(self, manifest_path, tests_path, url_base="/",
-                        meta_filters=None, recreate=False, download=False):
-        self.logger.info("Updating test manifest %s" % manifest_path)
-
-        json_data = None
-        if download:
-            # TODO: make this not github-specific
+    def load_manifest(self, tests_path, manifest_path, metadata_path, url_base="/", **kwargs):
+        cache_root = os.path.join(metadata_path, ".cache")
+        if self.manifest_download:
             download_from_github(manifest_path, tests_path)
-
-        if not recreate:
-            try:
-                with open(manifest_path) as f:
-                    json_data = json.load(f)
-            except IOError:
-                self.logger.info("Unable to find test manifest")
-            except ValueError:
-                self.logger.info("Unable to parse test manifest")
-
-        if not json_data:
-            self.logger.info("Creating test manifest")
-            manifest_file = manifest.Manifest(url_base)
-        else:
-            try:
-                manifest_file = manifest.Manifest.from_json(tests_path,
-                                                            json_data,
-                                                            meta_filters=meta_filters)
-            except manifest.ManifestVersionMismatch:
-                manifest_file = manifest.Manifest(url_base, meta_filters=meta_filters)
-
-        manifest_update.update(tests_path, manifest_file, True)
-
-        manifest.write(manifest_file, manifest_path)
-        return manifest_file
-
-    def load_manifest(self, tests_path, manifest_path, url_base="/", **kwargs):
-        try:
-            if (not os.path.exists(manifest_path) or
-                self.force_manifest_update):
-                manifest_file = self.update_manifest(manifest_path,
-                                                     tests_path,
-                                                     url_base,
-                                                     meta_filters=self.meta_filters,
-                                                     download=self.manifest_download)
-
-            else:
-                manifest_file = manifest.load(tests_path,
-                                              manifest_path,
-                                              types=self.types,
-                                              meta_filters=self.meta_filters)
-        except manifest.ManifestVersionMismatch:
-            manifest_file = manifest.Manifest(url_base)
-
-        if manifest_file.url_base != url_base:
-            self.logger.info("Updating url_base in manifest from %s to %s" % (manifest_file.url_base,
-                                                                              url_base))
-            manifest_file.url_base = url_base
-            manifest.write(manifest_file, manifest_path)
-
-        return manifest_file
+        return manifest.load_and_update(tests_path, manifest_path, url_base,
+                                        cache_root=cache_root, update=self.force_manifest_update,
+                                        meta_filters=self.meta_filters)
 
 
 def iterfilter(filters, iter):
     for f in filters:
         iter = f(iter)
     for item in iter:
         yield item
 
@@ -542,27 +488,29 @@ class TestLoader(object):
     def load_metadata(self, test_manifest, metadata_path, test_path):
         inherit_metadata = self.load_dir_metadata(test_manifest, metadata_path, test_path)
         test_metadata = manifestexpected.get_manifest(
             metadata_path, test_path, test_manifest.url_base, self.run_info)
         return inherit_metadata, test_metadata
 
     def iter_tests(self):
         manifest_items = []
+        manifests_by_url_base = {}
 
         for manifest in sorted(self.manifests.keys(), key=lambda x:x.url_base):
             manifest_iter = iterfilter(self.manifest_filters,
                                        manifest.itertypes(*self.test_types))
             manifest_items.extend(manifest_iter)
+            manifests_by_url_base[manifest.url_base] = manifest
 
         if self.chunker is not None:
             manifest_items = self.chunker(manifest_items)
 
         for test_type, test_path, tests in manifest_items:
-            manifest_file = iter(tests).next().manifest
+            manifest_file = manifests_by_url_base[iter(tests).next().url_base]
             metadata_path = self.manifests[manifest_file]["metadata_path"]
             inherit_metadata, test_metadata = self.load_metadata(manifest_file, metadata_path, test_path)
 
             for test in iterfilter(self.meta_filters,
                                    self.iter_wpttest(inherit_metadata, test_metadata, tests)):
                 yield test_path, test_type, test
 
     def iter_wpttest(self, inherit_metadata, test_metadata, tests):
--- a/testing/web-platform/tests/tools/wptrunner/wptrunner/tests/test_update.py
+++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/tests/test_update.py
@@ -93,17 +93,17 @@ def suite_log(entries, run_info=None):
             entries +
             [("suite_end", {})])
 
 
 def create_test_manifest(tests, url_base="/"):
     source_files = []
     for i, (test, _, test_type, _) in enumerate(tests):
         if test_type:
-            source_files.append(SourceFileWithTest(test, str(i) * 40, item_classes[test_type]))
+            source_files.append((SourceFileWithTest(test, str(i) * 40, item_classes[test_type]), True))
     m = manifest.Manifest()
     m.update(source_files)
     return m
 
 
 def test_update_0():
     tests = [("path/to/test.htm", ["/path/to/test.htm"], "testharness",
               """[test.htm]