Bug 50305 – Rebaseline server: add updating of baselines

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Mihai Parparita]

Rebaseline server: add updating of baselines

[Mihai Parparita]

Created attachment 75239 [details]
Patch

[Mihai Parparita]

+Dirk since I'm extending (Mock)FileSystem slightly.

[Dirk Pranke]

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.

[Tony Chang]

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 =

[Dirk Pranke]

(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.

[Mihai Parparita]

(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.

[Mihai Parparita]

Created attachment 75340 [details]
Patch

[Tony Chang]

Comment on attachment 75340 [details]
Patch

LGTM.  Maybe dirk wants to make another pass at it.

[Dirk Pranke]

(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)

[Dirk Pranke]

(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.

[Mihai Parparita]

(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.

[Mihai Parparita]

Committed r73204: <http://trac.webkit.org/changeset/73204>