Bug 48892 – Rebaseline server: initial framework

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

[Mihai Parparita]

Rebaseline server: initial framework

[Mihai Parparita]

Created attachment 72780 [details]
Patch

[Mihai Parparita]

Tony, taking you up on your offer to start reviewing this.

[Tony Chang]

Comment on attachment 72780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72780&action=review

Mostly small style nits.

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:37
>  \ No newline at end of file

Nit: New line

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:46
> +class RebaselineHTTPServer(BaseHTTPServer.HTTPServer):
> +    def __init__(self, httpd_port, results_directory):

Nit: Can you add a module level docstring explaining what is in this file and what this file is for?

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:53
> +    STATIC_FILE_NAMES = [
> +        'index.html',

Nit: Make this a set.

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:69
> +            (path, query_string) = self.path.split('?', 1)

Nit: Don't need the () on the left side

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:89
> +    def serve_static_file(self, static_path):

Nit: Can this method be protected?

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:107
> +        static_file = file(file_path, 'rb')

The preferred way in webkit to open files seems to be:
  with codecs.open(file_path, 'rb') as static_file:
The 'with' automatically closes the file at the end of the scope (which includes exceptions being raised).

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:111
> +        (mime_type, encoding) = mimetypes.guess_type(file_path)

Nit: no () on the left

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:117
> +            expires_time = \
> +                datetime.datetime.now() + datetime.timedelta(0, cacheable_seconds)

Nit: I prefer () for implicit line continuation instead of \, but I realize that's not in PEP8.  I think Eric just prefers to have long lines :)

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:129
> +    help_text = 'Given a test results directory generated by (new-)run-webkit-tests, starts a local HTTP server that displays unexpected failures and allows rebaselining.'

Tears, we could have just used the doc string for this, but I guess that boat has sailed.

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:145
> +        print 'Starting server at http://localhost:%d/' % options.httpd_port
> +

Nit: Maybe add text saying to browse to /quitquitquit to shutdown the server?  I guess you can also use ctrl+c to shutdown, right?

[Tony Chang]

Just to make sure, you guys are OK with adding this as a webkit-patch command (rather than a new file in WebKitTools/Scripts), right?

[Eric Seidel (no email)]

I think I should bite the bullet and finally fix https://bugs.webkit.org/show_bug.cgi?id=45838 so that writing this outside of the webkit-patch command would be easy.  I'm not sure this (or a lot of things) really need to be in webkit-patch anymore.  NOt that it really hurts anything to have them there, but I think smaller units of complexity are probably always better.

[Mihai Parparita]

Created attachment 72998 [details]
Patch

[Mihai Parparita]

(In reply to comment #3)
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:46
> > +class RebaselineHTTPServer(BaseHTTPServer.HTTPServer):
> > +    def __init__(self, httpd_port, results_directory):
> 
> Nit: Can you add a module level docstring explaining what is in this file and what this file is for?

Done.
 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:53
> > +    STATIC_FILE_NAMES = [
> > +        'index.html',
> 
> Nit: Make this a set.

Done.

> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:69
> > +            (path, query_string) = self.path.split('?', 1)
> 
> Nit: Don't need the () on the left side.

Fixed.

> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:89
> > +    def serve_static_file(self, static_path):
> 
> Nit: Can this method be protected?

Done.

> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:107
> > +        static_file = file(file_path, 'rb')
> 
> The preferred way in webkit to open files seems to be:
>   with codecs.open(file_path, 'rb') as static_file:
> The 'with' automatically closes the file at the end of the scope (which includes exceptions being raised).

Neat, changed.
> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:111
> > +        (mime_type, encoding) = mimetypes.guess_type(file_path)
> 
> Nit: no () on the left

Fixed.

> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:117
> > +            expires_time = \
> > +                datetime.datetime.now() + datetime.timedelta(0, cacheable_seconds)
> 
> Nit: I prefer () for implicit line continuation instead of \, but I realize that's not in PEP8.  I think Eric just prefers to have long lines :)

Switched to parentheses.

> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:129
> > +    help_text = 'Given a test results directory generated by (new-)run-webkit-tests, starts a local HTTP server that displays unexpected failures and allows rebaselining.'
> 
> Tears, we could have just used the doc string for this, but I guess that boat has sailed.

Actually, help_text = __doc__ does what you want.

> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:145
> > +        print 'Starting server at http://localhost:%d/' % options.httpd_port
> > +
> 
> Nit: Maybe add text saying to browse to /quitquitquit to shutdown the server?  I guess you can also use ctrl+c to shutdown, right?

Added some help text.

[Adam Barth]

Comment on attachment 72998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72998&action=review

Thanks for posting this in small pieces.  Also, can we test this code?  For example, we could test the command by having a mock HTTPServer so we didn't have to spin up a real server.

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/index.html:38
> +    <a href="/quitquitquit">Exit</a>

:)

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:50
> +class RebaselineHTTPServer(BaseHTTPServer.HTTPServer):

I wonder if these classes should go in a different package.  We don't really have anything similar to this now, so we might need to invent something (like we invented the bot package to hold the bot-related support classes).  Having this code here seems like it will make this file grow to infinite size.

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:87
> +        # See if a class method matches.
> +        func_name = func_or_file_name.replace('.', '_')

Should we prevent calls to function names that start with _ ?  Also, WebKit doesn't like abbreviations in variable names, so this would be better as function_name.

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:132
> +    help_text = __doc__

Should we actually add this __doc__ text?

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:143
> +            print 'Path to results directory is required.'

If it's required, why does its description have [ ] around it?  I think our argument parser is smart enough to do this checking for you if you used argument_names = "PATH_TO_RESULTS_DIRECTORY".  Also, we're trying to use " consistently instead of '.  Yes, we're terrible at that, but I mention it anyway.

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 72780 [details])
> Tears, we could have just used the doc string for this, but I guess that boat has sailed.

Someone needs to explain to me sometime why docstrings are useful.  As far as I can tell, they are exclusively harmful to the code.  They're routinely abused to add useless, redundant comments to code, or to excuse poor function naming or design.

I think lacking such an explanation I may start being more agressive about removing so many of the bad docstrings in our python code.

When can one even access a doc string?

I notice that the unit testing framework will use them instead of the test name (which is kinda cool).  But I've not seen any other part of python exploit this semantic data.

I expect the main problem with doc strings is that they don't have a clear common use case (or that they have to many things that might like to use them), so they end up trying to be everything to everyone and thus nothing.

[Mihai Parparita]

Created attachment 73004 [details]
Patch

[Mihai Parparita]

(In reply to comment #8)
> Thanks for posting this in small pieces.  Also, can we test this code?  For example, we could test the command by having a mock HTTPServer so we didn't have to spin up a real server.

I was hoping to move the actual rebaseline/copying logic into a separate class that the server/handler would use, and just unittest that.

> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:50
> > +class RebaselineHTTPServer(BaseHTTPServer.HTTPServer):
> 
> I wonder if these classes should go in a different package.  We don't really have anything similar to this now, so we might need to invent something (like we invented the bot package to hold the bot-related support classes).  Having this code here seems like it will make this file grow to infinite size.

I don't have a strong opinion here (the final file would probably be ~500 lines). If bug 45838 is fixed, where do you expect standalone tools to end up?
 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:87
> > +        # See if a class method matches.
> > +        func_name = func_or_file_name.replace('.', '_')
> 
> Should we prevent calls to function names that start with _ ?

Good point, made that return a 401.

>  Also, WebKit doesn't like abbreviations in variable names, so this would be better as function_name.

Changed.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:132
> > +    help_text = __doc__
>
> Should we actually add this __doc__ text?

The triple-quoted string at the top of the file ends up as the __doc__ value.

> > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:143
> > +            print 'Path to results directory is required.'
> 
> If it's required, why does its description have [ ] around it?  I think our argument parser is smart enough to do this checking for you if you used argument_names = "PATH_TO_RESULTS_DIRECTORY".  

Ah, hadn't realized that. Removed the [] and the checking.

> Also, we're trying to use " consistently instead of '.  Yes, we're terrible at that, but I mention it anyway.

Switched to ".

[Tony Chang]

(In reply to comment #9)
> (In reply to comment #3)
> > (From update of attachment 72780 [details] [details])
> > Tears, we could have just used the doc string for this, but I guess that boat has sailed.
> 
> Someone needs to explain to me sometime why docstrings are useful.  As far as I can tell, they are exclusively harmful to the code.  They're routinely abused to add useless, redundant comments to code, or to excuse poor function naming or design.
> 
> When can one even access a doc string?

__doc__ accesses the doc string.  In this case, instead of having a help_text member variable, you could have help just print __doc__.  This is commonly done to print the usage of a script.  For example:

class Command(object):
  """This command does ...""" 
  def help(self):
      # This method would actually be in an abstract base class.
      print self.__doc__

c = Command()
c.help()


> I think lacking such an explanation I may start being more agressive about removing so many of the bad docstrings in our python code.
> 

I think the google python style guide asks for overly verbose doc strings on functions, so I'm ok with removing (or condensing) what we have in the current code.

[Tony Chang]

Comment on attachment 73004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73004&action=review

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:150
> +        print ("Use the 'Exit' link in the UI, http://localhost:%d/" +

Nit: The + is not necessary (string literals are automatically joined).

[Mihai Parparita]

Committed r71369: <http://trac.webkit.org/changeset/71369>

[Mihai Parparita]

Comment on attachment 73004 [details]
Patch

Landed by hand (fixed Tony's nit before submitting).

[Eric Seidel (no email)]

This broke all the ews bots. :(  Clearly we're missing some testing somewhere.

ValueError: max() arg is an empty sequence.