Rebaseline server: add updating of baselines
Created attachment 75239 [details] Patch
+Dirk since I'm extending (Mock)FileSystem slightly.
Comment on attachment 75239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75239&action=review I only stared in detail at the filesystem stuff. The non-filesystem stuff looked plausibly correct and stylistically fine, but someone else should probably give it a solid review (or let me know if there's a desire for me to review it more carefully). > WebKitTools/Scripts/webkitpy/common/system/filesystem.py:120 > + def copy(self, source, destination): Is your intent for this to be copy() or copyfile() ? The mock version below only does copyfile() > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:44 > + self.files = files or {} I'm not seeing a reason for this change (or the change to the signature) over the previous version. Am I missing something? > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:62 > + I think this will fail if you list a directory containing files in subdirectories. (e.g. files={'foo/bar/baz.html'} fs.listdir('foo') will return 'baz.html'. In the patch to 48072 that I still haven't had time to break up and land, I have: def listdir(self, path): if not self.isdir(path): raise OSError("%s is not a directory" % path) if not path.endswith('/'): path += '/' dirs = [] files = [] for f in self.files: if self.exists(f) and f.startswith(path): remaining = f[len(path):] if '/' in remaining: dir = remaining[:remaining.index('/')] if not dir in dirs: dirs.append(dir) else: files.append(remaining) return dirs + files which is probably better. > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:85 > + self.files[destination] = self.files[source] This will not do the right thing if source is a directory (suggest adding: if self.isdir(source): raise IOError(errno.EISDIR, source, os.strerror(errno.ISDIR)) Of course, you'd need isdir() for that to work ... : def isfile(self, path): return path in self.files and self.files[path] is not None def isdir(self, path): return self.exists(path) and not self.isfile(path) Also, in 48072 I called this copyfile() for consistency w/ shutils (and to be explicit), but I don't feel strongly about the name.
Comment on attachment 75239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75239&action=review >> WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:44 >> + self.files = files or {} > > I'm not seeing a reason for this change (or the change to the signature) over the previous version. Am I missing something? class Foo(object): def __init__(self, files={}): self.files = files a = Foo() a.files['test'] = 1 b = Foo() print b.files # prints {'test': 1} > WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/queue.js:176 > + if (xhr.status < 400) { > + handleSuccess(); > + } else { > + handleFailure(); > + } Nit: 4 space indents > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:141 > + self.send_response(success and 200 or 500) I think with python 2.4+, you can do "200 if success else 500", which seems clearer, but I would probably just merge with the if statement above. > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:242 > + (test_name, test_extension) = os.path.splitext(test_file) Nit: drop () on the left side of =
(In reply to comment #4) > (From update of attachment 75239 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75239&action=review > > >> WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:44 > >> + self.files = files or {} > > > > I'm not seeing a reason for this change (or the change to the signature) over the previous version. Am I missing something? > > class Foo(object): > def __init__(self, files={}): > self.files = files > > a = Foo() > a.files['test'] = 1 > b = Foo() > print b.files # prints {'test': 1} > Ah. I thought that object literals were safe for default values, but I guess this is actually assigning to an anonymous object instead.
(In reply to comment #3) > > WebKitTools/Scripts/webkitpy/common/system/filesystem.py:120 > > + def copy(self, source, destination): > > Is your intent for this to be copy() or copyfile() ? The mock version below only does copyfile() copyfile is all I need, I changed its name (and the shutil function that's used) to that. > I think this will fail if you list a directory containing files in subdirectories. (e.g. files={'foo/bar/baz.html'} fs.listdir('foo') will return 'baz.html'. > > In the patch to 48072 that I still haven't had time to break up and land, I have: <snip> > which is probably better. Switched to your implementation. > > > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:85 > > + self.files[destination] = self.files[source] > > This will not do the right thing if source is a directory (suggest adding: > > if self.isdir(source): > raise IOError(errno.EISDIR, source, os.strerror(errno.ISDIR)) > > Of course, you'd need isdir() for that to work ... : > > def isfile(self, path): > return path in self.files and self.files[path] is not None > > def isdir(self, path): > return self.exists(path) and not self.isfile(path) Not sure I understand this, given that your isfile() implementation is the same as the current exists(), so I don't see how isdir could ever return true. (In reply to comment #4) > > WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/queue.js:176 > > + if (xhr.status < 400) { > > + handleSuccess(); > > + } else { > > + handleFailure(); > > + } > > Nit: 4 space indents Fixed. > > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:141 > > + self.send_response(success and 200 or 500) > > I think with python 2.4+, you can do "200 if success else 500", which seems clearer, but I would probably just merge with the if statement above. Merged with the if statement above. > > > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:242 > > + (test_name, test_extension) = os.path.splitext(test_file) > > Nit: drop () on the left side of = Done. Also renamed test_extension to _ since it's not actually used.
Created attachment 75340 [details] Patch
Comment on attachment 75340 [details] Patch LGTM. Maybe dirk wants to make another pass at it.
(In reply to comment #6) > > > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:85 > > > + self.files[destination] = self.files[source] > > > > This will not do the right thing if source is a directory (suggest adding: > > > > if self.isdir(source): > > raise IOError(errno.EISDIR, source, os.strerror(errno.ISDIR)) > > > > Of course, you'd need isdir() for that to work ... : > > > > def isfile(self, path): > > return path in self.files and self.files[path] is not None > > > > def isdir(self, path): > > return self.exists(path) and not self.isfile(path) > > Not sure I understand this, given that your isfile() implementation is the same as the current exists(), so I don't see how isdir could ever return true. > ah, sorry, I forgot that I had changed the exists() logic in that patch as well: def exists(self, path): if path in self.files: return self.files[path] is not None # Check to see if path refers to a "directory" in self.files. if not path.endswith('/'): path = path + '/' return any([f.startswith(path) for f in self.files]) I think this code is correct but is perhaps a bit confusing. It might be better to rewrite this as: def exists(self, path): return self.isfile(path) or self.isdir(path) def isfile(self, path): return path in self.files and self.files[path] is not None def isdir(self, path): if path in self.files: return False if not path.endswith('/'): path += '/' return any(f.startswith(path) for f in self.files)
(In reply to comment #8) > (From update of attachment 75340 [details]) > LGTM. Maybe dirk wants to make another pass at it. At some point the code in filesystem_mock / in_memory_filesystem should validate its arguments more carefully, but it doesn't have to be now. Patch LGTM otherwise.
(In reply to comment #9) > I think this code is correct but is perhaps a bit confusing. It might be better to rewrite this as: > > def exists(self, path): > return self.isfile(path) or self.isdir(path) > > def isfile(self, path): > return path in self.files and self.files[path] is not None > > def isdir(self, path): > if path in self.files: > return False > if not path.endswith('/'): > path += '/' > return any(f.startswith(path) for f in self.files) Seems straightforward enough. I'll incorporate this and land.
Committed r73204: <http://trac.webkit.org/changeset/73204>