Bug 62027 - nrwt: handle missing httpd cleanly
Summary: nrwt: handle missing httpd cleanly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kristóf Kosztyó
URL:
Keywords:
Depends on: 63130
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-03 09:37 PDT by Dirk Pranke
Modified: 2011-06-22 17:18 PDT (History)
8 users (show)

See Also:


Attachments
proposed fix (1.83 KB, patch)
2011-06-15 00:01 PDT, Kristóf Kosztyó
dpranke: review-
Details | Formatted Diff | Diff
proposed fix (3.04 KB, patch)
2011-06-16 01:28 PDT, Kristóf Kosztyó
dpranke: review-
Details | Formatted Diff | Diff
proposed fix (3.18 KB, patch)
2011-06-20 05:42 PDT, Kristóf Kosztyó
no flags Details | Formatted Diff | Diff
proposed fix (3.16 KB, patch)
2011-06-20 07:14 PDT, Kristóf Kosztyó
dpranke: review-
Details | Formatted Diff | Diff
proposed fix (1.88 KB, patch)
2011-06-21 01:58 PDT, Kristóf Kosztyó
no flags Details | Formatted Diff | Diff
proposed fix (1.76 KB, patch)
2011-06-22 00:27 PDT, Kristóf Kosztyó
no flags Details | Formatted Diff | Diff
stub out check_sys_deps() on the test port. (2.54 KB, patch)
2011-06-22 11:16 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
update ChangeLog (2.76 KB, patch)
2011-06-22 12:42 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-06-03 09:37:38 PDT
See 61939 for the change to ORWT; NRWT needs something similar.
Comment 1 Kristóf Kosztyó 2011-06-15 00:01:29 PDT
Created attachment 97244 [details]
proposed fix
Comment 2 Csaba Osztrogonác 2011-06-15 00:07:26 PDT
Comment on attachment 97244 [details]
proposed fix

LGTM, but I'm not so familiar with NRWT. Dirk?
Comment 3 Dirk Pranke 2011-06-15 12:51:51 PDT
Comment on attachment 97244 [details]
proposed fix

The idea is good, but this is is the wrong place to check for this ... this check would be done in a worker thread after the main test suite has already started running. 

There is a check_sys_deps() function on the Port object that is called prior to running the tests. It is passed a flag indicating whether we need an http server to run the tests or not. You should probably move this check to that routine instead. See  Tools/Scripts/webkitpy/layout_tests/port/base.py:185. Bonus points for modifying the chromium.py implementation as well :)


Also, NRWT doesn't actually support the --no-http flag yet, so we should either add that, file a bug to fix that, or not mention it in this error message.
Comment 4 Kristóf Kosztyó 2011-06-16 01:28:51 PDT
Created attachment 97420 [details]
proposed fix

I deleted the flag from the message, and it now test in the right place.
But I didn't modify the chromium.py because I can't test it at now.
Comment 5 Dirk Pranke 2011-06-16 14:35:32 PDT
Comment on attachment 97420 [details]
proposed fix

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

Almost there.

> Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:210
> +        if not(os.system(self._start_cmd + " -v >" + os.devnull)):

Generally speaking we try to avoid calling routines in os.* directly where possible, and this will also have the side effect of printing stuff to stderr, which I try to avoid.

Can you change this to:

try:
     return self._port_obj._executive(self._start_cmd + " -v", return_error_code=True):
except OSError, e:
     return False

instead?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:191
> +        self.check_httpd()

This check only needs to be done if needs_http is true.
Comment 6 Kristóf Kosztyó 2011-06-20 05:42:43 PDT
Created attachment 97786 [details]
proposed fix
Comment 7 Peter Gal 2011-06-20 06:32:30 PDT
(In reply to comment #6)
> Created an attachment (id=97786) [details]
> proposed fix

> Tools/Scripts/webkitpy/layout_tests/port/base.py:244
> +        print cmd

I don't think you wanted this 'print' in the patch :)

Otherwise LGTM.
Comment 8 Kristóf Kosztyó 2011-06-20 07:14:18 PDT
Created attachment 97798 [details]
proposed fix

thank you for noticed that
Comment 9 Dirk Pranke 2011-06-20 11:27:21 PDT
Comment on attachment 97798 [details]
proposed fix

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

> Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:211
> +

You don't need this function. Use self._port_obj._path_to_apache() (see line 78 for an example).

> Tools/Scripts/webkitpy/layout_tests/port/base.py:192
> +            self.check_httpd()

This should just return False if there's no httpd, not raise an Exception.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:240
> +            raise Exception('No httpd found. Cannot run http tests.')

This should not raise an exception. Just return the result of has_httpd(). In fact, I would probably just inline that method, which made more sense as a separate method when it was on the http_server object.

> Tools/Scripts/webkitpy/layout_tests/port/http_server.py:107
> +

Same comment as above.
Comment 10 Kristóf Kosztyó 2011-06-21 01:58:05 PDT
Created attachment 97952 [details]
proposed fix

thank you for your forbearance
Comment 11 Dirk Pranke 2011-06-21 12:51:13 PDT
Comment on attachment 97952 [details]
proposed fix

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

Change basically looks fine but I'm cq-'ing so you can clean up the nits.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:192
> +            if not self.check_httpd():

Minor nit:

if needs_http:
  return self.check_httpd()
return True

You don't need the print (which should be _log.error() anyway) since check_httpd() will log messages.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:193
> +                print 'No httpd found. Cannot run http tests.'

Nit. Change this to _log.error()

> Tools/Scripts/webkitpy/layout_tests/port/base.py:244
> +            if logging:

I don't know if you were copying code from check_image_diff, but logging isn't passed in to this method, so you're testing against the imported module, which will always be true. Just remove the if.
Comment 12 Kristóf Kosztyó 2011-06-22 00:27:34 PDT
Created attachment 98128 [details]
proposed fix
Comment 13 Dirk Pranke 2011-06-22 00:35:00 PDT
Comment on attachment 98128 [details]
proposed fix

excellent. thanks for the work!
Comment 14 WebKit Review Bot 2011-06-22 01:22:45 PDT
Comment on attachment 98128 [details]
proposed fix

Clearing flags on attachment: 98128

Committed r89414: <http://trac.webkit.org/changeset/89414>
Comment 15 WebKit Review Bot 2011-06-22 01:22:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Dirk Pranke 2011-06-22 11:16:02 PDT
Created attachment 98203 [details]
stub out check_sys_deps() on the test port.
Comment 18 Dirk Pranke 2011-06-22 11:16:52 PDT
Whoops. I should've realized that we needed to run test-webkitpy and make sure nothing broke. We needed to stub out the check_sys_deps() call (which used to be a no-op) on the test port.
Comment 19 Tony Chang 2011-06-22 11:30:09 PDT
Comment on attachment 98203 [details]
stub out check_sys_deps() on the test port.

What is the change in base.py for?
Comment 20 Dirk Pranke 2011-06-22 11:38:26 PDT
(In reply to comment #19)
> (From update of attachment 98203 [details])
> What is the change in base.py for?

That's the actual fix ... it modifies the default check_sys_deps() code to ensure that we have an http server. This only happens on the non-chromium ports at the moment. I will add this to chromium in a separate path.

We could put this change only in webkit.py, but I'm planning to merge the two classes shortly anyway, so I didn't see much point.
Comment 21 Tony Chang 2011-06-22 11:44:45 PDT
Comment on attachment 98203 [details]
stub out check_sys_deps() on the test port.

Oh, I see, there are two fixes here: (1) handle missing httpd cleanly and (2) fixing unit tests.  The ChangeLog didn't make that clear (normally the text below the bug number expands on the bug description).
Comment 22 Dirk Pranke 2011-06-22 12:40:07 PDT
(In reply to comment #21)
> (From update of attachment 98203 [details])
> Oh, I see, there are two fixes here: (1) handle missing httpd cleanly and (2) fixing unit tests.  The ChangeLog didn't make that clear (normally the text below the bug number expands on the bug description).

Sorry, I had tried to make the ChangeLog describe that, but I probably could've described the original change better. I will update the ChangeLog.
Comment 23 Dirk Pranke 2011-06-22 12:42:31 PDT
Created attachment 98219 [details]
update ChangeLog
Comment 24 WebKit Review Bot 2011-06-22 17:18:52 PDT
Comment on attachment 98219 [details]
update ChangeLog

Clearing flags on attachment: 98219

Committed r89502: <http://trac.webkit.org/changeset/89502>
Comment 25 WebKit Review Bot 2011-06-22 17:18:58 PDT
All reviewed patches have been landed.  Closing bug.