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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mihnea Dobrescu-Balaur (:mihneadb)]

This means basically starting httpd.js with -1 as a port and then using the correct port in the tests.

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use -1 as httpd port; work in progress

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

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use -1 as httpd port

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"

Comment 3

[Honza Bambas (:mayhemer)]

When is this planned to land?  I have a lot of changes in network xpcshell tests right now on gum (new http cache work).

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 5

[Patrick McManus [:mcmanus]]

honza, can we have adrian do the work of merging to gum after this lands?

Comment 6

[Honza Bambas (:mayhemer)]

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

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> I plat to backout 

I plan :)

Comment 8

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports

Added commit metdata.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=71e15bfbd6c9

Comment 9

[Honza Bambas (:mayhemer)]

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

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 11

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use a dynamic port in network/ xpcshell tests so they can be run in parallel

added a PORT constant for test_traceable_channel.js

Comment 12

[Honza Bambas (:mayhemer)]

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

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

[Honza Bambas (:mayhemer)]

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

?

Comment 16

[Honza Bambas (:mayhemer)]

: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

Comment 17

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports

Comment 18

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 19

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports

Updated according to feedback. Used qdiff -w.

Let me know if I should change something else.

Thanks!

Comment 20

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 21

[Honza Bambas (:mayhemer)]

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?

Comment 22

[Mihnea Dobrescu-Balaur (:mihneadb)]

Did you apply the parxpc patch as well?

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #22)
> Did you apply the parxpc patch as well?

Where is "parxpc" patch?

Comment 24

[Mihnea Dobrescu-Balaur (:mihneadb)]

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!

Comment 25

[Honza Bambas (:mayhemer)]

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?

Comment 26

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

[Honza Bambas (:mayhemer)]

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

Comment 28

[Mihnea Dobrescu-Balaur (:mihneadb)]

Thanks! Fixing those asap.

Comment 29

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports

Fixed indentation issues.

Comment 30

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dyamic ports

Changed commit msg to r=honzab

Comment 31

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 778546 [details] [diff] [review]
use dyamic ports

keeping r+

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9e46a514de

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9d9e46a514de