Bug 76680 – run-perf-tests should support --test-results-server option

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

[Ryosuke Niwa]

I've considered doing this using curl but hard-coding the exact URL, format, etc... into master.cfg seemed inflexible and fragile at best. So I'm adding --upload-json option to run-perf-test instead just like run-webkit-tests supports --test-results-server.

[Dirk Pranke]

If they do the same thing, it seems like the options should probably have the same name?

[Ryosuke Niwa]

(In reply to comment #1)
> If they do the same thing, it seems like the options should probably have the same name?

I guess we can do that though webkit-perf is not exactly "test result server".

[Dirk Pranke]

Feel free to change the name in run-webkit-tests to something better instead; I have no preference here.

[Ryosuke Niwa]

Created attachment 123244 [details]
adds the option

[Dirk Pranke]

Comment on attachment 123244 [details]
adds the option

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

> Tools/Scripts/webkitpy/common/net/file_uploader.py:92
> +        self._uplad_data(content_type, filesystem.read_text_file(filename))

typo: "uplad_data".

> Tools/Scripts/webkitpy/common/net/file_uploader.py:98
> +            file_objs.append(('file', filename, filesystem.read_text_file(filename)))

The previous code was opened as binary. Was that a bug? Seems like it probably was since we're uploading json data ...

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:324
> +        # Set uploading timeout in case appengine server is having problem.

Nit: "having problems"

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:118
> +            self._printer.write("Build not up to date for %s" % self._port._path_to_driver())

why are you changing the _log calls to self._printer calls? If the printer was initialized properly, the log messages should show up there anyway, and you're losing the different log levels this way.

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:138
> +                return -2

Is there significance to these values? Should you be logging messages or something?

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:157
>              except:

I realize you didn't change this, but bare exception catches are usually a bad idea. Please make this more specific while you're in here.

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:172
> +        except Exception, error:

same comment, except you did write this one :).

[Ryosuke Niwa]

Comment on attachment 123244 [details]
adds the option

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

>> Tools/Scripts/webkitpy/common/net/file_uploader.py:92
>> +        self._uplad_data(content_type, filesystem.read_text_file(filename))
> 
> typo: "uplad_data".

Fixed.

>> Tools/Scripts/webkitpy/common/net/file_uploader.py:98
>> +            file_objs.append(('file', filename, filesystem.read_text_file(filename)))
> 
> The previous code was opened as binary. Was that a bug? Seems like it probably was since we're uploading json data ...

Yeah, it was probably a bug.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:324
>> +        # Set uploading timeout in case appengine server is having problem.
> 
> Nit: "having problems"

Fixed.

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:118
>> +            self._printer.write("Build not up to date for %s" % self._port._path_to_driver())
> 
> why are you changing the _log calls to self._printer calls? If the printer was initialized properly, the log messages should show up there anyway, and you're losing the different log levels this way.

The use of _log was an error. I never intended to use _log anywhere in the code.

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:138
>> +                return -2
> 
> Is there significance to these values? Should you be logging messages or something?

No, I just want to make the debugging easier when failures happen on a bot. _generate_json and _upload_json should each log some error when they return False.

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:157
>>              except:
> 
> I realize you didn't change this, but bare exception catches are usually a bad idea. Please make this more specific while you're in here.

Sure, fixed.

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:172
>> +        except Exception, error:
> 
> same comment, except you did write this one :).

I don't think I can make this one more specific. We need to catch any error here in order to avoid blowing up inside _upload_json. It's also too verbose to list all exceptions urllib, socket, etc... may throw.

[Ryosuke Niwa]

Created attachment 123249 [details]
Fixed per Dirk's comment

[Ryosuke Niwa]

Comment on attachment 123249 [details]
Fixed per Dirk's comment

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

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:51
> +    _default_branch = 'WebKit trunk'

I'm going to change this to webkit-trunk per corresponding AppEngine change.

[Adam Barth]

Comment on attachment 123249 [details]
Fixed per Dirk's comment

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

> Tools/Scripts/webkitpy/common/net/file_uploader.py:86
>  class FileUploader(object):

Were there no file_uploader unit tests to update?  If not, please add some before landing.

> Tools/Scripts/webkitpy/common/net/file_uploader.py:92
> +    def upload_single_file(self, filesystem, content_type, filename):
> +        self._upload_data(content_type, filesystem.read_text_file(filename))

upload_single_file -> upload_single_text_file ?

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:138
> +                return -2

These numbers should probably be named constants.  Even better: just log the problem rather than using error codes.

[Ryosuke Niwa]

Comment on attachment 123249 [details]
Fixed per Dirk's comment

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

>> Tools/Scripts/webkitpy/common/net/file_uploader.py:86
>>  class FileUploader(object):
> 
> Were there no file_uploader unit tests to update?  If not, please add some before landing.

Unfortunately, it's hard to unit-test these functions because we can't really stub/mock urllib2/socket/NetworkTransaction.

[Ryosuke Niwa]

(In reply to comment #9)
> (From update of attachment 123249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123249&action=review
>
> > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:138
> > +                return -2
> 
> These numbers should probably be named constants.  Even better: just log the problem rather than using error codes.

Calles emit a error message so we should be able to see them in the console.

[Dirk Pranke]

> >> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:118
> >> +            self._printer.write("Build not up to date for %s" % self._port._path_to_driver())
> > 
> > why are you changing the _log calls to self._printer calls? If the printer was initialized properly, the log messages should show up there anyway, and you're losing the different log levels this way.
> 
> The use of _log was an error. I never intended to use _log anywhere in the code.
> 

Generally, we use _log to print things in python code. Further, the printer class is a confusing, obscure class that probably shouldn't be public and should only be used when you need to print things that aren't a line-at-a-time.

Please change these back to _log statements unless there's some other reason you really need access to the printer directly.

[Ryosuke Niwa]

Comment on attachment 123249 [details]
Fixed per Dirk's comment

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

>> Tools/Scripts/webkitpy/common/net/file_uploader.py:92
>> +        self._upload_data(content_type, filesystem.read_text_file(filename))
> 
> upload_single_file -> upload_single_text_file ?

Done.

>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:138
>>> +                return -2
>> 
>> These numbers should probably be named constants.  Even better: just log the problem rather than using error codes.
> 
> Calles emit a error message so we should be able to see them in the console.

I've changed calls to printer.write by log.error in the callees and replaced these by _EXIT_CODE_* constants.

[Ryosuke Niwa]

Committed r105543: <http://trac.webkit.org/changeset/105543>

[Dirk Pranke]

(In reply to comment #13)
> I've changed calls to printer.write by log.error in the callees and replaced these by _EXIT_CODE_* constants.

Looks great. Thank you!