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.
Created attachment 123509 [details] fixes the bug
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'?
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.
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*
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).
Created attachment 123611 [details] Fixed the bug pointed out by Tony
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>?
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.
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.
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>
All reviewed patches have been landed. Closing bug.