Bug 62027 – nrwt: handle missing httpd cleanly

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

[Dirk Pranke]

See 61939 for the change to ORWT; NRWT needs something similar.

[Kristóf Kosztyó]

Created attachment 97244 [details]
proposed fix

[Csaba Osztrogonác]

Comment on attachment 97244 [details]
proposed fix

LGTM, but I'm not so familiar with NRWT. Dirk?

[Dirk Pranke]

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.

[Kristóf Kosztyó]

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.

[Dirk Pranke]

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.

[Kristóf Kosztyó]

Created attachment 97786 [details]
proposed fix

[Peter Gal]

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

[Kristóf Kosztyó]

Created attachment 97798 [details]
proposed fix

thank you for noticed that

[Dirk Pranke]

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.

[Kristóf Kosztyó]

Created attachment 97952 [details]
proposed fix

thank you for your forbearance

[Dirk Pranke]

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.

[Kristóf Kosztyó]

Created attachment 98128 [details]
proposed fix

[Dirk Pranke]

Comment on attachment 98128 [details]
proposed fix

excellent. thanks for the work!

[WebKit Review Bot]

Comment on attachment 98128 [details]
proposed fix

Clearing flags on attachment: 98128

Committed r89414: <http://trac.webkit.org/changeset/89414>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it was rolled out by http://trac.webkit.org/changeset/89421

http://build.webkit.org/builders/Qt%20Linux%20Release/builds/34459/steps/webkitpy-test/logs/stdio

[Dirk Pranke]

Created attachment 98203 [details]
stub out check_sys_deps() on the test port.

[Dirk Pranke]

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.

[Tony Chang]

Comment on attachment 98203 [details]
stub out check_sys_deps() on the test port.

What is the change in base.py for?

[Dirk Pranke]

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

[Tony Chang]

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

[Dirk Pranke]

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

[Dirk Pranke]

Created attachment 98219 [details]
update ChangeLog

[WebKit Review Bot]

Comment on attachment 98219 [details]
update ChangeLog

Clearing flags on attachment: 98219

Committed r89502: <http://trac.webkit.org/changeset/89502>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.