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

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: wlach, Assigned: ffledgling)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Comment 1

5 years ago
Created attachment 771336 [details] [diff] [review]
Patch to remove hard-coded port usage

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)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 771379 [details] [diff] [review]
updated patch

Updated with fixes
Attachment #771336 - Attachment is obsolete: true
Attachment #771379 - Flags: review?(wlachance)
Attachment #771379 - Flags: review?(wlachance) → review+
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.