Bug 50581 – Make TestResultsServer return empty JSON or 404 for non-existing results files

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

[Kinuko Yasuda]

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).

[Kinuko Yasuda]

Created attachment 75726 [details]
Patch

[Ojan Vafai]

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 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]

(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]

(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]

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]

Bug 50589 for the onerror bug.

[Kinuko Yasuda]

Created attachment 75742 [details]
Patch

[Kinuko Yasuda]

(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]

Committed r73409: <http://trac.webkit.org/changeset/73409>