use a dynamic port in downloads/ xpcshell tests so they can be run in parallel

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mihneadb, Assigned: mihneadb)

Tracking

unspecified
mozilla25
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Blocks: 887064
(Assignee)

Comment 1

5 years ago
Created attachment 769266 [details] [diff] [review]
use -1 as httpd port
Assignee: nobody → mihneadb
(Assignee)

Updated

5 years ago
Attachment #769266 - Flags: review?(mak77)
Comment on attachment 769266 [details] [diff] [review]
use -1 as httpd port

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

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +92,5 @@
> +  var port;
> +  if (!server)
> +    port = 4444;
> +  else
> +    port = server.identity.primaryPort;

I'm not sure I understand this, is not the scope of these changes to stop using fixed ports?
So why are you making the server an optional parameter? Should not it be mandatory to avoid exactly what you are fixing here?

moreover, I think you should update the javadoc explaining the new param.

::: toolkit/components/downloads/test/unit/test_privatebrowsing.js
@@ +89,2 @@
>  
> +  let tmpDir = do_get_profile();

I actually think it's not a very wise idea to use the temporary profile dir as a trashcan for anything, why not adding a do_get_tempDir() to xpcshell that generates a uniquely named (nsIFile can do that) subdir in TmpD? That would be much better.

::: toolkit/components/downloads/test/unit/test_privatebrowsing_cancel.js
@@ +109,5 @@
>      response.write("foo");
>    });
> +  httpserv.start(-1);
> +
> +  let port = httpserv.identity.primaryPort;

please move this closer to its usage and make it a const PORT
Attachment #769266 - Flags: review?(mak77)
(Assignee)

Comment 3

5 years ago
(In reply to Marco Bonardo [:mak] from comment #2)
> Comment on attachment 769266 [details] [diff] [review]
> use -1 as httpd port
> 
> Review of attachment 769266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/downloads/test/unit/head_download_manager.js
> @@ +92,5 @@
> > +  var port;
> > +  if (!server)
> > +    port = 4444;
> > +  else
> > +    port = server.identity.primaryPort;
> 
> I'm not sure I understand this, is not the scope of these changes to stop
> using fixed ports?
> So why are you making the server an optional parameter? Should not it be
> mandatory to avoid exactly what you are fixing here?

This is because test_guid.js does not actually create/start a server. The others do. I felt the least intrusive way to fix this was to add the server as a parameter and where there was no server just keep the old behaviour (or return a random port that is not used by a server, could do that).

I'm not sure how to do this in a better way, maybe you have a suggestion.

> 
> moreover, I think you should update the javadoc explaining the new param.

You are right, I missed that.

> 
> ::: toolkit/components/downloads/test/unit/test_privatebrowsing.js
> @@ +89,2 @@
> >  
> > +  let tmpDir = do_get_profile();
> 
> I actually think it's not a very wise idea to use the temporary profile dir
> as a trashcan for anything, why not adding a do_get_tempDir() to xpcshell
> that generates a uniquely named (nsIFile can do that) subdir in TmpD? That
> would be much better.

Great idea, I'll open a new bug for this and Cc you.

> 
> ::: toolkit/components/downloads/test/unit/test_privatebrowsing_cancel.js
> @@ +109,5 @@
> >      response.write("foo");
> >    });
> > +  httpserv.start(-1);
> > +
> > +  let port = httpserv.identity.primaryPort;
> 
> please move this closer to its usage and make it a const PORT

Ok.
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #3)
> This is because test_guid.js does not actually create/start a server. The
> others do. I felt the least intrusive way to fix this was to add the server
> as a parameter and where there was no server just keep the old behaviour (or
> return a random port that is not used by a server, could do that).
> 
> I'm not sure how to do this in a better way, maybe you have a suggestion.

Just create a server and pass it in, make the server parameter mandatory and throw if it's not defined. It's just one test, the overhead is minimal.
(Assignee)

Comment 5

5 years ago
(In reply to Marco Bonardo [:mak] from comment #4)
> (In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #3)
> > This is because test_guid.js does not actually create/start a server. The
> > others do. I felt the least intrusive way to fix this was to add the server
> > as a parameter and where there was no server just keep the old behaviour (or
> > return a random port that is not used by a server, could do that).
> > 
> > I'm not sure how to do this in a better way, maybe you have a suggestion.
> 
> Just create a server and pass it in, make the server parameter mandatory and
> throw if it's not defined. It's just one test, the overhead is minimal.

If the server is not started, getting server.identity.primaryPort just blocks. And that test does not *start* a server, since it (seems to) tries to reach an URL that does not exist.
the scope of the test is not to test a url that does not exist, I'm quite confident you can reuse a valid url from some other test in the same folder.
(Assignee)

Updated

5 years ago
Depends on: 892021
(Assignee)

Comment 7

5 years ago
Created attachment 773713 [details] [diff] [review]
use a dynamic port

Not sure about the javadoc for the server param.
Attachment #773713 - Flags: review?(mak77)
(Assignee)

Updated

5 years ago
Attachment #769266 - Attachment is obsolete: true
Comment on attachment 773713 [details] [diff] [review]
use a dynamic port

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

just some minor indentation fixes.

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +77,5 @@
>  
>  var gDownloadCount = 0;
>  /**
>   * Adds a download to the DM, and starts it.
> + * @param server: a HttpServer used to build the sourceURI

rather than "to build" I'd say "to serve"

@@ +92,3 @@
>  {
> +  if (!server)
> +    do_throw("server undefined");

"Must provide a valid server."

::: toolkit/components/downloads/test/unit/test_bug_401430.js
@@ +111,1 @@
>  			targetFile: do_get_file(resultFileName, true)});

reindent second line

::: toolkit/components/downloads/test/unit/test_cancel_download_files_removed.js
@@ +89,5 @@
>    currentTest++;
>  
> +  let channel = NetUtil.newChannel("http://localhost:" +
> +                                  httpserver.identity.primaryPort +
> +                                  set.serverPath);

bad indentation

::: toolkit/components/downloads/test/unit/test_private_resume.js
@@ +96,5 @@
>  
>    downloadUtils.downloadManager.addPrivacyAwareListener(listener);
>  
> +  const downloadCSource = "http://localhost:" +
> +        httpserv.identity.primaryPort + "/head_download_manager.js";

just indent by 2 spaces or align after =

::: toolkit/components/downloads/test/unit/test_privatebrowsing.js
@@ +255,5 @@
>  
>    // properties of Download-C
>    let downloadC = -1;
> +  const downloadCSource = "http://localhost:" +
> +        httpserv.identity.primaryPort + "/head_download_manager.js";

just indent by 2 spaces or align after =
Attachment #773713 - Flags: review?(mak77) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 774139 [details] [diff] [review]
use a dynamic port

Not sure if I properly fixed the indentation issues, r?'ing again.
Attachment #774139 - Flags: review?(mak77)
(Assignee)

Updated

5 years ago
Attachment #773713 - Attachment is obsolete: true

Updated

5 years ago
Attachment #774139 - Flags: review?(mak77) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eae43bdf03c2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.