Bug 50581

Summary: Make TestResultsServer return empty JSON or 404 for non-existing results files
Product: WebKit Reporter: Kinuko Yasuda <kinuko>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch ojan: review+

Kinuko Yasuda
Reported 2010-12-06 12:21:18 PST
Make TestResultsServer return empty JSON or 404 for non-existing results files. For non-layout tests some builders do not have complete test results but only for a specific set of tests (e.g. only ui_tests and net_unittests for "XP Tests (dbg)(2)" builder etc). We should return empty json or 404 for such cases (otherwise the dashboard frontend won't run as intended).
Attachments
Patch (1.88 KB, patch)
2010-12-06 12:23 PST, Kinuko Yasuda
no flags
Patch (1.25 KB, patch)
2010-12-06 15:38 PST, Kinuko Yasuda
ojan: review+
Kinuko Yasuda
Comment 1 2010-12-06 12:23:33 PST
Ojan Vafai
Comment 2 2010-12-06 12:32:44 PST
Comment on attachment 75726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75726&action=review > WebKitTools/TestResultServer/handlers/testfilehandler.py:118 > + return EMPTY_JSON_FORMAT % builder Why do we want to do this instead of handling None at the calling location? Don't we want to 404 in this case as well?
Kinuko Yasuda
Comment 3 2010-12-06 12:42:58 PST
Comment on attachment 75726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75726&action=review >> WebKitTools/TestResultServer/handlers/testfilehandler.py:118 >> + return EMPTY_JSON_FORMAT % builder > > Why do we want to do this instead of handling None at the calling location? Don't we want to 404 in this case as well? I want to do so as well, simply returning 404 somehow didn't get the script.onerror function called in my local env (unlike normal 404 error). I haven't investigated it yet -- do you know how to make it work?
Ojan Vafai
Comment 4 2010-12-06 12:46:52 PST
(In reply to comment #3) > I want to do so as well, simply returning 404 somehow didn't get the script.onerror function called in my local env (unlike normal 404 error). I haven't investigated it yet -- do you know how to make it work? Hm...should work. Maybe it's some issue with serving off localhost. Maybe try updating the testing server and see if it works there? http://test-results-test.appspot.com/
Kinuko Yasuda
Comment 5 2010-12-06 13:01:33 PST
(In reply to comment #4) > (In reply to comment #3) > > I want to do so as well, simply returning 404 somehow didn't get the script.onerror function called in my local env (unlike normal 404 error). I haven't investigated it yet -- do you know how to make it work? > > Hm...should work. Maybe it's some issue with serving off localhost. Maybe try updating the testing server and see if it works there? > > http://test-results-test.appspot.com/ I updated the test-results-test server but getting the same result. (The console shows 'Failed to load resource: the server responded with a status of 404 (Not Found)' error messages as well as for normal 404 but the error function isn't being hooked up.) Will you be able to take a look at it? On the testing server now URLs like following returns 404. http://test-results-test.appspot.com/testfile?builder=Linux%20Builder%20(Views%20dbg)&master=Chromium&testtype=unit_tests&name=results.json
Ojan Vafai
Comment 6 2010-12-06 14:05:47 PST
Looks like a webkit bug. At first glance, it seems we don't fire the error event for a 404 if the file contents are empty. Firefox fires the error event in this case. As a workaround, on the JS side, we can listen to the load event as well and check if the script is empty, e.g., <script id="foo" src="http://test-results-test.appspot.com/testfile?builder=Linux%20Builder%20(Views%20dbg)&master=Chromium&testtype=unit_tests&name=results.json" onload="alert('innerHTML=\'' + document.getElementById('foo').innerHTML + '\'');" onerror="alert('error')"></script> Eventually, we can fix the webkit bug and won't need this anymore.
Ojan Vafai
Comment 7 2010-12-06 14:22:31 PST
Bug 50589 for the onerror bug.
Kinuko Yasuda
Comment 8 2010-12-06 15:38:26 PST
Kinuko Yasuda
Comment 9 2010-12-06 15:42:54 PST
(In reply to comment #6) > Looks like a webkit bug. At first glance, it seems we don't fire the error event for a 404 if the file contents are empty. Firefox fires the error event in this case. As a workaround, on the JS side, we can listen to the load event as well and check if the script is empty, e.g., > > <script > id="foo" > src="http://test-results-test.appspot.com/testfile?builder=Linux%20Builder%20(Views%20dbg)&master=Chromium&testtype=unit_tests&name=results.json" > onload="alert('innerHTML=\'' + document.getElementById('foo').innerHTML + '\'');" > onerror="alert('error')"></script> > > Eventually, we can fix the webkit bug and won't need this anymore. Interesting. onload work-around seemed to work. I updated the patch just to return 404.
Kinuko Yasuda
Comment 10 2010-12-06 16:07:22 PST
Note You need to log in before you can comment on or make changes to this bug.