Bug 62251 – webkitpy: add integration tests for new-run-webkit-httpd, stop calling shut_down_http_server

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Dirk Pranke]

webkitpy: add integration tests for new-run-webkit-httpd, stop calling shut_down_http_server

[Dirk Pranke]

Created attachment 96340 [details]
Patch

[Dirk Pranke]

Created attachment 96348 [details]
Patch

[Dirk Pranke]

Created attachment 96349 [details]
Patch

[Dirk Pranke]

Created attachment 96353 [details]
ready for review, I think

[Tony Chang]

Comment on attachment 96353 [details]
ready for review, I think

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

> Tools/Scripts/new-run-webkit-httpd:76
> +            # FIXME: This isn't ideal, since it could conflict with        
> +            # lighttpd processes not started by this script
> +            port_obj._executive.kill_all('lighttpd')
> +            port_obj._executive.kill_all('httpd')

What's the reason for using kill_all rather than the stop method here?

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 96353 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96353&action=review
> 
> > Tools/Scripts/new-run-webkit-httpd:76
> > +            # FIXME: This isn't ideal, since it could conflict with        
> > +            # lighttpd processes not started by this script
> > +            port_obj._executive.kill_all('lighttpd')
> > +            port_obj._executive.kill_all('httpd')
> 
> What's the reason for using kill_all rather than the stop method here?

In this context, start() has not been called, and so stop has no pid that it can use to kill. The older version of the code would call kill_all ; I'm just making this explicit since this was the only place that *was* calling kill_all, and it was forcing us to have a stupid hook into the port object.

A better fix would be for the script to be able to fetch the started pid from a pidfile and use that; that will come in a later patch once all of this stuff is cleaned up and consistent.

[Tony Chang]

Comment on attachment 96353 [details]
ready for review, I think

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

>>> Tools/Scripts/new-run-webkit-httpd:76
>>> +            port_obj._executive.kill_all('httpd')
>> 
>> What's the reason for using kill_all rather than the stop method here?
> 
> In this context, start() has not been called, and so stop has no pid that it can use to kill. The older version of the code would call kill_all ; I'm just making this explicit since this was the only place that *was* calling kill_all, and it was forcing us to have a stupid hook into the port object.
> 
> A better fix would be for the script to be able to fetch the started pid from a pidfile and use that; that will come in a later patch once all of this stuff is cleaned up and consistent.

It looks like on Linux, the binary is called apache2.  Do we need to add that here?  Also, it looks like on Windows you need the .exe or it doesn't kill the process.  This new code doesn't seem like it'll work on all platforms.

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 96353 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96353&action=review
> 
> >>> Tools/Scripts/new-run-webkit-httpd:76
> >>> +            port_obj._executive.kill_all('httpd')
> >> 
> >> What's the reason for using kill_all rather than the stop method here?
> > 
> > In this context, start() has not been called, and so stop has no pid that it can use to kill. The older version of the code would call kill_all ; I'm just making this explicit since this was the only place that *was* calling kill_all, and it was forcing us to have a stupid hook into the port object.
> > 
> > A better fix would be for the script to be able to fetch the started pid from a pidfile and use that; that will come in a later patch once all of this stuff is cleaned up and consistent.
> 
> It looks like on Linux, the binary is called apache2.  Do we need to add that here?  Also, it looks like on Windows you need the .exe or it doesn't kill the process.  This new code doesn't seem like it'll work on all platforms.

OK, I'll clean that up ... not sure how I missed that :(.

[Dirk Pranke]

Created attachment 96519 [details]
update w/ review feedback

[Dirk Pranke]

Revised patch uploaded.

Note that this whole area of code is so broken and inconsistent that it's hard to make interim patches actually look (or be) correct. The main reason I've broken them up into several bugs is to try and make the reviewing process easier. I'm planning to land them all in rapid succession, because I have much more faith that the end state is correct than I do that interim states are.

Note that this set of patches as a whole is a second attempt at the patch that was approved in bug 59977. There is still one patch remaining that I need to add that will actually clean up stray servers left over from previous runs, and if I land this series without that change, this fix will be no better than 59977 (although the actual code is cleaner in this set of patches, I think).

[Tony Chang]

Comment on attachment 96519 [details]
update w/ review feedback

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

> Tools/Scripts/webkitpy/layout_tests/port/http_server_integrationtest.py:1
> +# Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Tools/Scripts/webkitpy/layout_tests/port/http_server_integrationtest.py:44
> +    HTTP_PORTS = [8000, 8080, 8443]

Nit: Use a tuple instead of a list.

> Tools/Scripts/webkitpy/layout_tests/port/http_server_integrationtest.py:73
> +    def integration_test_http_server__normal(self):
> +        self.assert_servers_are_down('localhost', self.HTTP_PORTS)

Are these integration_tests only run when invoking the file directly?  Are they run on the bots?  Is anyone working on the FIXME in port_testcase.py to merge this into test-webkitpy?

> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:148
> +    def integration_test_lighttpd_server__normal(self):

We only use lighttpd on Chromium Win.  It looks like we only skip the test on chromium mac.  Is that intended?  Maybe the lighttpd tests should just move into chromium_win_unittest.py.

> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:189
> +    def integration_test_apache_server__fails(self):
> +        port = self.make_port(options=mocktool.MockOptions(configuration='Release', use_apache=True))

Do the apache tests pass on chromium windows?

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 96519 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96519&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/http_server_integrationtest.py:1
> > +# Copyright (C) 2010 Google Inc. All rights reserved.
> 
> 2011

Will do.

> 
> > Tools/Scripts/webkitpy/layout_tests/port/http_server_integrationtest.py:44
> > +    HTTP_PORTS = [8000, 8080, 8443]
> 
> Nit: Use a tuple instead of a list.
>

Ok.
 
> > Tools/Scripts/webkitpy/layout_tests/port/http_server_integrationtest.py:73
> > +    def integration_test_http_server__normal(self):
> > +        self.assert_servers_are_down('localhost', self.HTTP_PORTS)
> 
> Are these integration_tests only run when invoking the file directly?  Are they run on the bots?  Is anyone working on the FIXME in port_testcase.py to merge this into test-webkitpy?

Yes. No. Not yet. I suspect we need to reach some sort of agreement over what will be safe to run where.

Note that these tests won't actually pass, anyway, until the other patches land.

> 
> > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:148
> > +    def integration_test_lighttpd_server__normal(self):
> 
> We only use lighttpd on Chromium Win.  It looks like we only skip the test on chromium mac.  Is that intended?  Maybe the lighttpd tests should just move into chromium_win_unittest.py.

I need to re-run the tests and double check; I thought that the lighttpd tests actually ran successfully on all three platforms, which is why I did it this way. I don't recall why I stubbed them out here.

> 
> > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:189
> > +    def integration_test_apache_server__fails(self):
> > +        port = self.make_port(options=mocktool.MockOptions(configuration='Release', use_apache=True))
> 
> Do the apache tests pass on chromium windows?

No, which is why they're stubbed out there.

[Tony Chang]

Comment on attachment 96519 [details]
update w/ review feedback

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

>>> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:148
>>> +    def integration_test_lighttpd_server__normal(self):
>> 
>> We only use lighttpd on Chromium Win.  It looks like we only skip the test on chromium mac.  Is that intended?  Maybe the lighttpd tests should just move into chromium_win_unittest.py.
> 
> I need to re-run the tests and double check; I thought that the lighttpd tests actually ran successfully on all three platforms, which is why I did it this way. I don't recall why I stubbed them out here.

I see.  It may be that all the linux/mac bots still have lighttpd installed, but we might not want to depend on that.  It's probably a bit safer to just move the tests into chromium_win_unittest.py.

[Dirk Pranke]

(In reply to comment #13)
> (From update of attachment 96519 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96519&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:148
> >>> +    def integration_test_lighttpd_server__normal(self):
> >> 
> >> We only use lighttpd on Chromium Win.  It looks like we only skip the test on chromium mac.  Is that intended?  Maybe the lighttpd tests should just move into chromium_win_unittest.py.
> > 
> > I need to re-run the tests and double check; I thought that the lighttpd tests actually ran successfully on all three platforms, which is why I did it this way. I don't recall why I stubbed them out here.
> 
> I see.  It may be that all the linux/mac bots still have lighttpd installed, but we might not want to depend on that.  It's probably a bit safer to just move the tests into chromium_win_unittest.py.

Okay, you confused me, I think, with the earlier comment. The chromium ports all have lighttpd installed into third_party, and these tests pass on all of the ports.

As long as lighttpd can run everywhere, I feel like it makes sense to test it on all the platforms to get the increased coverage.

On the other hand, it seems a little goofy that we have a command line flag (--use-apache) that doesn't actually work on the one platform it is meant for, and no command line flags to turn off apache and use lighttpd on the others. 

So, I'd suggest that we land this patch as-is (with the nits fixed), and then add a separate patch that yanks the --use-apache flag and changes these test routines to just test whatever http server the port is configured for.

[Tony Chang]

(In reply to comment #14)
> Okay, you confused me, I think, with the earlier comment. The chromium ports all have lighttpd installed into third_party, and these tests pass on all of the ports.

lighttpd isn't installed on Linux.  It used to be pulled in by install-build-deps.sh, but we removed that a long time ago too.  We should probably remove it from mac DEPS as well since it's not used there.

[Dirk Pranke]

Created attachment 96839 [details]
update w/ more review feedback

[Dirk Pranke]

Created attachment 96842 [details]
fix naming in http_server_integrationtest.py

[Tony Chang]

Comment on attachment 96842 [details]
fix naming in http_server_integrationtest.py

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

> Tools/Scripts/webkitpy/layout_tests/port/http_server_integrationtest.py:39
> +import port_testcase

Nit: absolute import path?

[Dirk Pranke]

Created attachment 96987 [details]
upload w/ tony's last bit of feedback

[Dirk Pranke]

Committed r88671: <http://trac.webkit.org/changeset/88671>

[Dirk Pranke]

re-opening, I had to roll this out.

[Dirk Pranke]

finally fixed in bug 62829.

*** This bug has been marked as a duplicate of bug 62828 ***