Closed Bug 888350 Opened 6 years ago Closed 6 years ago

use a dynamic port in network/test/unit* xpcshell tests so they can be run in parallel

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mihneadb, Assigned: mihneadb)

References

Details

Attachments

(1 file, 7 obsolete files)

This means basically starting httpd.js with -1 as a port and then using the correct port in the tests.
Blocks: 887706
I still need to figure out why the tests that have something to do with the cache work only using port 4444.

One such test is netwerk/test/unit/test_bug767025.js
Assignee: nobody → mihneadb
Attached patch use -1 as httpd port (obsolete) — Splinter Review
I cannot get test_bug455311.js to pass locally even without any patches. Other than that all tests pass.

The two cache-related tests have been marked as "run sequentially"
Attachment #769008 - Attachment is obsolete: true
No longer blocks: 887706
When is this planned to land?  I have a lot of changes in network xpcshell tests right now on gum (new http cache work).
(In reply to Honza Bambas (:mayhemer) from comment #3)
> When is this planned to land?  I have a lot of changes in network xpcshell
> tests right now on gum (new http cache work).

This should land as soon as possible, since it does not break anything. Would you like to review this or should I ask someone else?

As for the actual parallel harness, I'm not sure when that will land. I guess as soon as all the fixed tests patches land.
Blocks: 887064
Depends on: parxpc
honza, can we have adrian do the work of merging to gum after this lands?
(In reply to Patrick McManus [:mcmanus] from comment #5)
> honza, can we have adrian do the work of merging to gum after this lands?

I'll rather do it.  I plat to backout all changes to files we've ever modified and re-apply on top of our patches.  It will be simpler.
(In reply to Honza Bambas (:mayhemer) from comment #6)
> I plat to backout 

I plan :)
Attachment #770607 - Flags: review?(honzab.moz)
Attached patch use dynamic ports (obsolete) — Splinter Review
Added commit metdata.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=71e15bfbd6c9
Attachment #770607 - Attachment is obsolete: true
Attachment #770607 - Flags: review?(honzab.moz)
Attachment #774056 - Flags: review?(honzab.moz)
Comment on attachment 770607 [details] [diff] [review]
use -1 as httpd port

Following test have not been fixed by this patch:

test_bug767025.js
test_offlinecache_custom-directory.js
test_traceable_channel.js
(In reply to Honza Bambas (:mayhemer) from comment #9)
> Comment on attachment 770607 [details] [diff] [review]
> use -1 as httpd port
> 
> Following test have not been fixed by this patch:
> 
> test_bug767025.js
> test_offlinecache_custom-directory.js
> test_traceable_channel.js

Yes, I added "run-sequentially" for those (hm, I missed test_traceable channel, will get back with the new patch) because the cache uses the hardcoded ports in the generated hash key.
added a PORT constant for test_traceable_channel.js
Attachment #774078 - Flags: review?(honzab.moz)
Attachment #774056 - Attachment is obsolete: true
Attachment #774056 - Flags: review?(honzab.moz)
Please just check these are also either excluded from parallel run or fixed:

test_basic_functionality.js
test_body_length.js
test_byte_range.js
test_cern_meta.js
test_default_index_handler.js
test_empty_body.js
test_errorhandler_exception.js
test_header_array.js
test_host.js
test_load_module.js
test_name_scheme.js
test_processasync.js
test_qi.js
test_registerdirectory.js
test_registerfile.js
test_registerprefix.js
test_request_line_split_in_two_packets.js
test_response_write.js
test_seizepower.js
test_setindexhandler.js
test_setstatusline.js
test_sjs.js
test_sjs_object_state.js
test_sjs_state.js
test_sjs_throwing_exceptions.js
test_start_stop.js
test_bug561042.js
test_bug767025.js
test_bug856978.js
test_offlinecache_custom-directory.js
test_protocolproxyservice.js
(In reply to Honza Bambas (:mayhemer) from comment #12)
> Please just check these are also either excluded from parallel run or fixed:
> 
> test_basic_functionality.js
> test_body_length.js
> test_byte_range.js
> test_cern_meta.js
> test_default_index_handler.js
> test_empty_body.js
> test_errorhandler_exception.js
> test_header_array.js
> test_host.js
> test_load_module.js
> test_name_scheme.js
> test_processasync.js
> test_qi.js
> test_registerdirectory.js
> test_registerfile.js
> test_registerprefix.js
> test_request_line_split_in_two_packets.js
> test_response_write.js
> test_seizepower.js
> test_setindexhandler.js
> test_setstatusline.js
> test_sjs.js
> test_sjs_object_state.js
> test_sjs_state.js
> test_sjs_throwing_exceptions.js
> test_start_stop.js
> test_bug561042.js
> test_bug767025.js
> test_bug856978.js
> test_offlinecache_custom-directory.js
> test_protocolproxyservice.js

Those are httpd.js's tests, covered in bug 887706
Summary: use a dynamic port in network/ xpcshell tests so they can be run in parallel → use a dynamic port in network/test xpcshell tests so they can be run in parallel
Summary: use a dynamic port in network/test xpcshell tests so they can be run in parallel → use a dynamic port in network/test/unit* xpcshell tests so they can be run in parallel
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #8)
> Created attachment 774056 [details] [diff] [review]
> use dynamic ports
> 
> Added commit metdata.
> 
> Try run: https://tbpl.mozilla.org/?tree=Try&rev=71e15bfbd6c9

So I don't know about the test_resumable_truncate.js failure on WinXP. Any suggestions?

I'm thinking httpd's choose a dynamic port thing might have a bug?
Comment on attachment 774078 [details] [diff] [review]
use a dynamic port in network/ xpcshell tests so they can be run in parallel

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

First, sorry for the delay, this is a large patch.

Thanks for the effort.


We have a general rule not to add random whitespace removal changes to patches.  Only modify lines that you need to modify to fix the bug.  It makes reviewing much harder (distinguish change in the code or just a whitespace change) and also merging any pending patches over your changes is harder.  Now I'm not referring to my pending changes but anyone's.

Please try to remove the whitespace only changes by doing hg qdiff -w.  It should produce a patch that ignores it.  You just may need to check on few files that your are changing indention of existing lines at and fix them.  If that doesn't work for you, then leave the whitespace changes in...

You are kinda inconsistent in a way you fix the stuff, but that is OK.

I'll run tests locally and push this to try to give final r+.

::: netwerk/test/unit/test_authentication.js
@@ +12,5 @@
> +  return "http://localhost:" + httpserv.identity.primaryPort;
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "PORT", function() {
> + return httpserv.identity.primaryPort;

bad indent

@@ +413,5 @@
>    do_test_pending();
>  }
>  
>  function test_digest_bogus_user() {
> +  var chan = makeChan("http://localhost:" + PORT + "/auth/digest");

All usages of "..." + PORT + "path.." could be turned to URL + "/path.." instead.

::: netwerk/test/unit/test_cookie_header.js
@@ +62,5 @@
>  
>  function makeChan() {
>    var ios = Components.classes["@mozilla.org/network/io-service;1"]
>                        .getService(Components.interfaces.nsIIOService);
> +  var chan = ios.newChannel(URL, null, null)

URL is missing the trailing slash here.  Please check other tests too

::: netwerk/test/unit/test_fallback_request-error_canceled.js
@@ +94,5 @@
>  
>    cacheUpdateObserver = {observe: function() {
>      dump("got offline-cache-update-completed\n");
>      // offline cache update completed.
> +    var _x = randomURI; // doing this to eval the lazy getter

if you just put a line

randomURI;

alone, doesn't it work already?

And why is it needed at all? (Add a comment why you do it at least.)  because of the server.stop call bellow?

::: netwerk/test/unit/test_freshconnection.js
@@ +30,5 @@
> +  server.start(-1);
> +  var port = server.identity.primaryPort;
> +  server.stop(function(){});
> +
> +  var chan = ios.newChannel("http://localhost:" + port, "", null);

Not sure you need this...

::: netwerk/test/unit/test_header_Accept-Language.js
@@ +91,5 @@
> +  srv.start(-1);
> +  var port = srv.identity.primaryPort;
> +  srv.stop(function(){});
> +
> +  let chan = ios.newChannel("http://localhost:" + port + path, "", null);

Not needed IMO :)

::: netwerk/test/unit/test_redirect_baduri.js
@@ +9,5 @@
>   * Test whether we fail bad URIs in HTTP redirect as CORRUPTED_CONTENT.
>   */
>  
> +var httpServer = new HttpServer();
> +httpServer.start(-1);

why not a lazy getter in this too?

::: netwerk/test/unit/test_redirect_from_script.js
@@ +195,5 @@
>    };
>    return test;
>  }
>  
> +// will be defined in run_test because of the lazy getters

// because port is defined dynamically

?
:mihneadb, what all patches I have to apply to make this patch work?  apppplying parxpc2.diff + netwerk_restof_tests.diff gives me:

 0:16.68 INFO | Result summary:
 0:16.69 INFO | Passed: 209
 0:16.69 INFO | Failed: 19
 0:16.69 INFO | Todo: 0
Attached patch use dynamic ports (obsolete) — Splinter Review
Attachment #775796 - Flags: review?(honzab.moz)
Attachment #774078 - Attachment is obsolete: true
Attachment #774078 - Flags: review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #16)
> :mihneadb, what all patches I have to apply to make this patch work? 
> apppplying parxpc2.diff + netwerk_restof_tests.diff gives me:
> 
>  0:16.68 INFO | Result summary:
>  0:16.69 INFO | Passed: 209
>  0:16.69 INFO | Failed: 19
>  0:16.69 INFO | Todo: 0

So, parxpc2, the patch in bug 887706 (for the httpd.js tests) and the new "netwerk2" updated patch which I just uploaded here.
Attached patch use dynamic ports (obsolete) — Splinter Review
Updated according to feedback. Used qdiff -w.

Let me know if I should change something else.

Thanks!
Attachment #776051 - Flags: review?(honzab.moz)
Attachment #775796 - Attachment is obsolete: true
Attachment #775796 - Flags: review?(honzab.moz)
No longer depends on: parxpc
Hmm, so I've updated to the current mozilla-central tip where bug 887706 is fixed, applied the patch, rebuild netwerk/test.  Though, mach xpcshell-test netwerk/test are running sequentially.  What's wrong?
Depends on: 887706
Did you apply the parxpc patch as well?
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #22)
> Did you apply the parxpc patch as well?

Where is "parxpc" patch?
My bad, I was referring to the patch in bug 887054. That has not landed yet. There's no need to bother running them concurrently, the big issue was to fix them and not break current behavior. 


Thanks!
Thanks for explanation.  So, to make it clear: parallel run of network xpcshell tests is not covered by this bug.  Only purpose is to prepare the network tests for it, right?
(In reply to Honza Bambas (:mayhemer) from comment #25)
> Thanks for explanation.  So, to make it clear: parallel run of network
> xpcshell tests is not covered by this bug.  Only purpose is to prepare the
> network tests for it, right?


Sorry for the confusion! 


Yes, that is correct. However, I did check that the tests pass successfully when run on parallel with this patch applied and they do.
Comment on attachment 776051 [details] [diff] [review]
use dynamic ports

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

r=honzab

Thanks!  This is it.

mach xpcshell-test network/test runs correctly with this patch on my local build.

::: netwerk/test/unit/test_assoc.js
@@ +37,5 @@
>  
>               // this is invalid because the space between method and URL is missing
>               {url: "/assoc/assoctest?invalid2",
> +        responseheader: [ "Assoc-Req: GEThttp://localhost:" + port +
> +            "/assoc/assoctest?invalid2",

Fix indention

::: netwerk/test/unit/test_bug586908.js
@@ +28,5 @@
>      throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>    },
>    
>    mainThreadOnly: true,
> +    PACURI: "http://localhost:" + httpserv.identity.primaryPort + "/redirect",

Indention

::: netwerk/test/unit/test_mismatch_last-modified.js
@@ +75,5 @@
>  	var channel = request.QueryInterface(Ci.nsIHttpChannel);
>  
> +	    var chan = ios.newChannel("http://localhost:" +
> +				      httpserver.identity.primaryPort +
> +				      "/test1", "", null);

Fix indention.

@@ +107,5 @@
>  	var channel = request.QueryInterface(Ci.nsIHttpChannel);
>  
> +	    var chan = ios.newChannel("http://localhost:" +
> +				      httpserver.identity.primaryPort +
> +				      "/test1", "", null);

Indention
Attachment #776051 - Flags: review?(honzab.moz) → review+
Thanks! Fixing those asap.
Attached patch use dynamic ports (obsolete) — Splinter Review
Fixed indentation issues.
Attachment #776051 - Attachment is obsolete: true
Attached patch use dyamic portsSplinter Review
Changed commit msg to r=honzab
Attachment #778544 - Attachment is obsolete: true
Comment on attachment 778546 [details] [diff] [review]
use dyamic ports

keeping r+
Attachment #778546 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9d9e46a514de
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.