Bug 50581 - Make TestResultsServer return empty JSON or 404 for non-existing results files
Summary: Make TestResultsServer return empty JSON or 404 for non-existing results files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-06 12:21 PST by Kinuko Yasuda
Modified: 2010-12-06 16:07 PST (History)
1 user (show)

See Also:


Attachments
Patch (1.88 KB, patch)
2010-12-06 12:23 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (1.25 KB, patch)
2010-12-06 15:38 PST, Kinuko Yasuda
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 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).
Comment 1 Kinuko Yasuda 2010-12-06 12:23:33 PST
Created attachment 75726 [details]
Patch
Comment 2 Ojan Vafai 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?
Comment 3 Kinuko Yasuda 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?
Comment 4 Ojan Vafai 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/
Comment 5 Kinuko Yasuda 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
Comment 6 Ojan Vafai 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.
Comment 7 Ojan Vafai 2010-12-06 14:22:31 PST
Bug 50589 for the onerror bug.
Comment 8 Kinuko Yasuda 2010-12-06 15:38:26 PST
Created attachment 75742 [details]
Patch
Comment 9 Kinuko Yasuda 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.
Comment 10 Kinuko Yasuda 2010-12-06 16:07:22 PST
Committed r73409: <http://trac.webkit.org/changeset/73409>