762143 - instantiation of mozhttpd should go in its own method

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

run_tests.run_tests is a long method that does far too much.  We
should separate the discrete bits of functionality into standalone
functions (or objects).

One of the things that it does is to start the mozhttpd server for the
develop case:

http://hg.mozilla.org/build/talos/file/170c100911b6/talos/run_tests.py#l548

lines 548-568 should be moved to their own method to start the server
and return the httpd object.  Stopping can be handled as it is done
currently:

http://hg.mozilla.org/build/talos/file/170c100911b6/talos/run_tests.py#l597

Comment 1

[Eric Millin]

I'll take care of this.

Comment 2

[Eric Millin]

Attachment: run_test refactoring

Comment 3

[Eric Millin]

I tested this patch using the start time tests, and it seemed to work fine.  The code reflects what I think you want, but it does not reflect what you said you want--it is, as it were, unsolicited advice ;p  Seriously tho, I tried to "separate the discrete bits of functionality into standalone functions (or objects)," which took me beyond run_tests.  If want just the httpd method changes, let me know.  

In regards to the changes in ffprocess.py, Py threw an error whenever the code tried to access an urlparser attribute.  Afaik, the port and hostname attributes don't exist in Py 2.4, but I'm using 2.4.4 and I don't know very much ;) 

Also, I'm not sure how to squash changesets in hg.  Please let me know how to handle multiple changesets in the future.

The attached code really needs another couple rounds of refactoring, but I think it's a good start.  I look forward to your comments.

Comment 4

[Jeff Hammel]

Comment on attachment 639996 [details] [diff] [review]
run_test refactoring

Thanks for the very extensive patch!

This is a much larger than the scope of the bug, but I'll look at it when I can.

Comment 5

[Eric Millin]

Sorry about the size.  If it's better for you to review just the initial method, and then work from there, I'd be happy to resubmit.

Comment 6

[Jeff Hammel]

Comment on attachment 639996 [details] [diff] [review]
run_test refactoring

+        hostname,port = url[1].split(':')
So this won't work if there is no ':' in the domain :( (It could also not work for urls of form user:[e-mail]:80, but I don't care about that)

Probably most/all of the stuff in BrowserInterpolator should be in PerfConfigurator

+   ## if sys.maxsize > 2**32:  # is 64bits?
+   ##     platform_name = 'win-amd64'
+   ## else:
+    platform_name = 'win32'

Why this change?


This is also a diff vs the last diff.  In general, uploading the entireity of the proposed change is preferable

These look like mostly good changes, but its a bit hard to parse since the diff isn't applyable:

curl https://bug762143.bugzilla.mozilla.org/attachment.cgi?id=639996 | patch -p1 --dry-run

Could you upload a new diff that contains all changes from the current tip?

Comment 7

[jdev005]

Attachment: tested patch

Comment 8

[jdev005]

Attachment: better 1 don c the pervious

Comment 9

[Jeff Hammel]

Comment on attachment 649733 [details] [diff] [review]
better 1 don c the pervious

This mostly looks good.  However, let's not pass in all of browser_config here, just the wbserver.  The `if browser_config['develop']` check can be done in the run_tests function

Comment 10

[jdev005]

Attachment: this should do it

Comment 11

[Jeff Hammel]

Comment on attachment 650181 [details] [diff] [review]
this should do it

+def setup_webserver(browser_config_webserver):

I can't say I'm a fan of the name browser_config_webserver.  How about just webserver?

+# setup a webserver, if --develop is specified to PerfConfigurator.py

indentation

Other than that, works for me.  Do you mind if I make these changes on checkin?

Comment 12

[jdev005]

sure no problem :)

Comment 13

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1bdefb6a312e

Comment 14

[Mozilla RelEng Bot]

Try run for 1bdefb6a312e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1bdefb6a312e
Results (out of 69 total builds):
    success: 62
    failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1bdefb6a312e

Comment 15

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/19fb848f813e