888544 - use a dynamic port in jsdownloads/ xpcshell tests so they can be run in parallel

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use -1 as httpd port

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 769275 [details] [diff] [review]
use -1 as httpd port

Review of attachment 769275 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +50,5 @@
>                                        "@mozilla.org/binaryoutputstream;1",
>                                        "nsIBinaryOutputStream",
>                                        "setOutputStream")
>  
> +let serverSocket = null;

I don't think you need this complicate change that may easily create shadow variables in tests (indeed you had to unshadow a couple, that's confusing).

@@ +60,4 @@
>  
> +XPCOMUtils.defineLazyGetter(this, "FAKE_BASE", function() {
> +  return "http://localhost:" + serverSocket.port;
> +});

we generate a new fake server everytime we call startFakeServer, but since all of these are lazy getters, they will use the proper port only for the first generated server.
I suspect this works just by accident, likely the httpd server returns always the same free port, but that's fragile.
You need an actual getter here, but not a lazy one, you may create those with Object.defineProperty on the global object, I guess.

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +628,5 @@
>   * Ensures download error details are reported on network failures.
>   */
>  add_task(function test_download_error_source()
>  {
> +  serverSocket = startFakeServer();

please no

::: toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js
@@ +275,5 @@
>   * Ensures download error details are reported for legacy downloads.
>   */
>  add_task(function test_error()
>  {
> +  serverSocket = startFakeServer();

ditto

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

ah, rather than storing gServerSocket I think you may just store gFakeServerPort

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

Ok, I understand what the problems are, and I tried to do what was possible without changing the structure of the tests themselves (the fact that they all share those 'constants' in the head file while starting their own servers is a pain).

I'm not sure I fully understand what you suggest I should do.

Are you saying I should use defineProperty for all the URLs in the head file and inside the defProp function just use socketServer "blindly" (knowing that it will be defined by the time the getters are called)?


Thanks

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #4)
> Are you saying I should use defineProperty for all the URLs in the head file
> and inside the defProp function just use socketServer "blindly" (knowing
> that it will be defined by the time the getters are called)?

I think that's fine, we use one server at a time. The alternative would be to provide an object that given a server returns urls, but that looks like an overkill for the current usage.

Comment 6

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports

I hope I understood correctly what you suggested.

Comment 7

[Mihnea Dobrescu-Balaur (:mihneadb)]

try run: https://tbpl.mozilla.org/?tree=Try&rev=ef4fd93a637b

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 773742 [details] [diff] [review]
use dynamic ports

Review of attachment 773742 [details] [diff] [review]:
-----------------------------------------------------------------

well, ideally only the FAKE_BASE getters should not be lazy, but actually I think it's ok as it is, so all of them are coherent, there's less risk of using the wrong getter type.

Comment 9

[:Paolo Amadini]

This looks good. For the record, I noticed we started to have many repetitions of
the same pattern, in the future we might refactor that part a little bit (a good
moment might be while solving the intermittent failure in bug 865364).

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d578673157a5

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d578673157a5

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

:mak77, this is not entirely fixed. Every once in a while I encounter this:

http://pastebin.mozilla.org/2796790

At first I thought it was the getTempDir function but that's not the problem. I'll run these sequentially for now (it's no big win, 4s vs 5s on my machine), marked in bug 898658.

Comment 13

[:Paolo Amadini]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #12)
> :mak77, this is not entirely fixed. Every once in a while I encounter this:
> 
> http://pastebin.mozilla.org/2796790

This looks like the intermittent failure filed as bug 901017, do you think it
can be related to running the tests concurrently?

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

:paolo, you are absolutely right. I just assumed it was my fault, haha :).