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

Status: RESOLVED FIXED

Description

[William Lachance (:wlach) (use needinfo!)]

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.

Comment 1

[Anhad Jai Singh (:ffledgling)]

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?

Comment 2

[William Lachance (:wlach) (use needinfo!)]

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.

Comment 3

[William Lachance (:wlach) (use needinfo!)]

Oh, and no, there's no need to specify the port anymore.

Comment 4

[Anhad Jai Singh (:ffledgling)]

Created attachment 771379 [details] [diff] [review]
updated patch

Updated with fixes

Comment 5

[William Lachance (:wlach) (use needinfo!)]

Pushed: https://github.com/mozilla/mozbase/commit/582a2b3419ad53a4e9bc255b9e38b996c96cf8f8