Closed Bug 888537 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Downloads API, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mihneadb, Assigned: mihneadb)

References

Details

Attachments

(1 file, 2 obsolete files)

      No description provided.
Blocks: 887064
Attached patch use -1 as httpd port (obsolete) — Splinter Review
Assignee: nobody → mihneadb
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)
(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.
(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.
Depends on: 892021
Attached patch use a dynamic port (obsolete) — Splinter Review
Not sure about the javadoc for the server param.
Attachment #773713 - Flags: review?(mak77)
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+
Not sure if I properly fixed the indentation issues, r?'ing again.
Attachment #774139 - Flags: review?(mak77)
Attachment #773713 - Attachment is obsolete: true
Attachment #774139 - Flags: review?(mak77) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eae43bdf03c2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: