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

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

Not sure about the javadoc for the server param.

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 =

Not sure if I properly fixed the indentation issues, r?'ing again.