Bug 76802 – run-perf-tests should report server-side errors

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

[Ryosuke Niwa]

webkit-perf.appspot.com may reject uploaded jsons for reasons other than network errors. e.g. malformed data. run-perf-tests should report those errors.

[Ryosuke Niwa]

Created attachment 123509 [details]
fixes the bug

[Tony Chang]

Comment on attachment 123509 [details]
fixes the bug

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

> Tools/Scripts/webkitpy/common/net/file_uploader.py:116
> -            NetworkTransaction(timeout_seconds=self._timeout_seconds).run(callback)
> +            response = NetworkTransaction(timeout_seconds=self._timeout_seconds).run(callback)
>          finally:
>              socket.setdefaulttimeout(orig_timeout)
> +            return response

Do we need to define response outside of the try?  Otherwise if response doesn't get set, you'll get an UnboundLocalError exception.

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:183
> +        response_body = [line.strip('\n') for line in response]
> +        if response_body != ['OK']:

Can you just check for response.strip('\n') == 'OK'?

[Ryosuke Niwa]

Comment on attachment 123509 [details]
fixes the bug

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

>> Tools/Scripts/webkitpy/common/net/file_uploader.py:116
>> +            return response
> 
> Do we need to define response outside of the try?  Otherwise if response doesn't get set, you'll get an UnboundLocalError exception.

Oops, yes, I should set it to None. It's very annoying that we can't create a unit test for this function :(

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:183
>> +        if response_body != ['OK']:
> 
> Can you just check for response.strip('\n') == 'OK'?

response is a file-like object so not sure if we can do that. I was initially fetching only the first line but then it didn't really matter because the for-loop terminates after one iteration when the response is just 'OK' and we have to read the entire response otherwise anyways.

[Tony Chang]

Comment on attachment 123509 [details]
fixes the bug

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

>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:183
>>> +        if response_body != ['OK']:
>> 
>> Can you just check for response.strip('\n') == 'OK'?
> 
> response is a file-like object so not sure if we can do that. I was initially fetching only the first line but then it didn't really matter because the for-loop terminates after one iteration when the response is just 'OK' and we have to read the entire response otherwise anyways.

You could do response.read().strip('\n') and store that in a variable.  I guess that's about the same amount of code.  *shrug*

[Ryosuke Niwa]

Comment on attachment 123509 [details]
fixes the bug

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

>>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:183
>>>> +        if response_body != ['OK']:
>>> 
>>> Can you just check for response.strip('\n') == 'OK'?
>> 
>> response is a file-like object so not sure if we can do that. I was initially fetching only the first line but then it didn't really matter because the for-loop terminates after one iteration when the response is just 'OK' and we have to read the entire response otherwise anyways.
> 
> You could do response.read().strip('\n') and store that in a variable.  I guess that's about the same amount of code.  *shrug*

But then I'd have to store that value when it doesn't match 'OK' or else I'd have to manually seek back (not even sure if we can do that on the response).

[Ryosuke Niwa]

Created attachment 123611 [details]
Fixed the bug pointed out by Tony

[Dirk Pranke]

Comment on attachment 123611 [details]
Fixed the bug pointed out by Tony

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

change looks fine to me but I'll let tony r+ it.

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:183
> +        if response_body != ['OK']:

Minor nit: why not just delete 182 and change 183 to <code>if response_body != ['OK\n']:</code>?

[Ryosuke Niwa]

Comment on attachment 123611 [details]
Fixed the bug pointed out by Tony

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

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:183
>> +        if response_body != ['OK']:
> 
> Minor nit: why not just delete 182 and change 183 to <code>if response_body != ['OK\n']:</code>?

If I do that, then I'd have to call stip on each line when we output the error.

[Dirk Pranke]

Comment on attachment 123611 [details]
Fixed the bug pointed out by Tony

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

>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:183
>>> +        if response_body != ['OK']:
>> 
>> Minor nit: why not just delete 182 and change 183 to <code>if response_body != ['OK\n']:</code>?
> 
> If I do that, then I'd have to call stip on each line when we output the error.

Ah, fair enough.

[Ryosuke Niwa]

Comment on attachment 123611 [details]
Fixed the bug pointed out by Tony

Clearing flags on attachment: 123611

Committed r105650: <http://trac.webkit.org/changeset/105650>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.