Closed Bug 887910 Opened 12 years ago Closed 12 years ago

mozfile's test_load.py:test_http shouldn't use hardcoded port for http server

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wlach, Assigned: ffledgling)

Details

Attachments

(1 file, 1 obsolete file)

https://github.com/mozilla/mozbase/blob/master/mozfile/tests/test_load.py#L28 By always instantiating the server with port 8888, we create two problems: 1. If another test wants to grab port 8888 at the same time, it will fail. 2. If you have a server listening on port 8888 when the test runs, it will fail. The preferred use of mozhttpd is to pass in port 0 (now the default if nothing else specified). Let's fix the test to do that, and use the new get_url method (http://mozbase.readthedocs.org/en/latest/mozhttpd.html#mozhttpd.MozHttpd.get_url) to get the file url below while we're at it.
I've removed the port parameter specified in the test entirely, since 0 is now the default and since we're using get_url we should not really be worried about the specified port, will that be okay or should I specify port=0 explicitly?
Attachment #771336 - Flags: review?(wlachance)
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 771336 [details] [diff] [review] Patch to remove hard-coded port usage Review of attachment 771336 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with some minor requests for changes. r+ with those applied. ::: mozfile/tests/test_load.py @@ +33,5 @@ > httpd.start(block=False) > + # Get the server's url > + server_url = httpd.get_url() > + # Fetch the content from the url > + content = load(server_url).read() I would just collapse the code + comments into: content = load(httpd.get_url()).read() It's pretty obvious what's going on without the comment. @@ +34,5 @@ > + # Get the server's url > + server_url = httpd.get_url() > + # Fetch the content from the url > + content = load(server_url).read() > + print content Remove this line.
Attachment #771336 - Flags: review?(wlachance) → review+
Oh, and no, there's no need to specify the port anymore.
Attached patch updated patchSplinter Review
Updated with fixes
Attachment #771336 - Attachment is obsolete: true
Attachment #771379 - Flags: review?(wlachance)
Attachment #771379 - Flags: review?(wlachance) → review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: