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

RESOLVED FIXED in mozilla25

Status

()

Core
Networking
RESOLVED FIXED
4 years ago
4 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, 7 obsolete attachments)

(Assignee)

Description

4 years ago
This means basically starting httpd.js with -1 as a port and then using the correct port in the tests.
(Assignee)

Updated

4 years ago
Blocks: 887706
(Assignee)

Comment 1

4 years ago
Created attachment 769008 [details] [diff] [review]
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
Assignee: nobody → mihneadb
(Assignee)

Comment 2

4 years ago
Created attachment 770607 [details] [diff] [review]
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"
Attachment #769008 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
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).
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 887064
Depends on: 887054
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 :)
(Assignee)

Updated

4 years ago
Attachment #770607 - Flags: review?(honzab.moz)
(Assignee)

Comment 8

4 years ago
Created attachment 774056 [details] [diff] [review]
use dynamic ports

Added commit metdata.

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

Updated

4 years ago
Attachment #770607 - Attachment is obsolete: true
Attachment #770607 - Flags: review?(honzab.moz)
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Comment 11

4 years ago
Created attachment 774078 [details] [diff] [review]
use a dynamic port in network/ xpcshell tests so they can be run in parallel

added a PORT constant for test_traceable_channel.js
Attachment #774078 - Flags: review?(honzab.moz)
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 13

4 years ago
(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
(Assignee)

Comment 14

4 years ago
(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
(Assignee)

Comment 17

4 years ago
Created attachment 775796 [details] [diff] [review]
use dynamic ports
Attachment #775796 - Flags: review?(honzab.moz)
(Assignee)

Updated

4 years ago
Attachment #774078 - Attachment is obsolete: true
Attachment #774078 - Flags: review?(honzab.moz)
(Assignee)

Comment 18

4 years ago
(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.
(Assignee)

Comment 19

4 years ago
Created attachment 776051 [details] [diff] [review]
use dynamic ports

Updated according to feedback. Used qdiff -w.

Let me know if I should change something else.

Thanks!
Attachment #776051 - Flags: review?(honzab.moz)
(Assignee)

Updated

4 years ago
Attachment #775796 - Attachment is obsolete: true
Attachment #775796 - Flags: review?(honzab.moz)
(Assignee)

Updated

4 years ago
No longer depends on: 887054
(Assignee)

Comment 20

4 years ago
try run: https://tbpl.mozilla.org/?tree=Try&rev=3cc3d0e1e923
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
(Assignee)

Comment 22

4 years ago
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?
(Assignee)

Comment 24

4 years ago
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?
(Assignee)

Comment 26

4 years ago
(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+
(Assignee)

Comment 28

4 years ago
Thanks! Fixing those asap.
(Assignee)

Comment 29

4 years ago
Created attachment 778544 [details] [diff] [review]
use dynamic ports

Fixed indentation issues.
(Assignee)

Updated

4 years ago
Attachment #776051 - Attachment is obsolete: true
(Assignee)

Comment 30

4 years ago
Created attachment 778546 [details] [diff] [review]
use dyamic ports

Changed commit msg to r=honzab
(Assignee)

Updated

4 years ago
Attachment #778544 - Attachment is obsolete: true
(Assignee)

Comment 31

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

keeping r+
Attachment #778546 - Flags: review+
(Assignee)

Updated

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