889182 - mozapps/extensions xpcshell tests cannot be run concurrently

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mihnea Dobrescu-Balaur (:mihneadb)]

The main issue is that all the tests use a hardcoded 4444 port. What's worse is that the port is also hardcoded in the static files in the data/ folder.

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: what I've done so far

I tried to intercept the port numbers and either remove them when comparing two URLs and swap out the 4444 to the actual port when handling the RDFs.

Did not have much success. Any suggestion would be great!

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

I am still blocked on this, not sure what's the best solution to this problem.
Worst case we can just run these tests sequentially, but this should be avoided.

Comment 3

[Andrew Halberstadt [:ahal]]

Dave, do you have any ideas for getting the hardcoded port out of the xml/rdf files for these tests? Or could you direct us towards the person we should be talking to?

Much appreciated!

Comment 4

[Dave Townsend [:mossop]]

I guess you could create an sjs file that loads the rdf file, replaces %PORT% with the actual port and returns that.

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Dave Townsend (:Mossop) from comment #4)
> I guess you could create an sjs file that loads the rdf file, replaces
> %PORT% with the actual port and returns that.

Would this be the best way of doing this?

Also, I tried looking up on MDN "sjs" and found nothing, just a mention on the Mochitest wiki page.

Some more information (or maybe someone more experienced who could help out with this) would be great!


Thank you

Comment 6

[Robert Strong (they/them - no direct email)]

I think sjs would be the best way. There is info on sjs in (just search the page for sjs)
https://developer.mozilla.org/en-US/docs/Mochitest

and to see different uses of them you can search mxr
http://mxr.mozilla.org/mozilla-central/find?text=&string=\.sjs

Comment 7

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: interpolate static files

Work in progress. Managed to do it, this fixes the test_AddonsRepository.js test.
Did not use sjs files because getting the info of what static file to serve and
what port to use was too complicated.

This basically does the same thing, just that it uses a pathHandler instead of
an SJS file.

Comment 8

[cmtalbert]

Comment on attachment 775074 [details] [diff] [review]
interpolate static files

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

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js
@@ +1328,5 @@
> +      data += str.value;
> +    } while (read != 0);
> +  }
> +  cstream.close(); // this closes fstream as well
> +

A quick drive by comment:
I like the simplicity of this approach. Seems easier than dealing with a bunch of sjs files. This part here worries me, though. What if this throws? Shouldn't we use try/catch/finally to ensure this stream is closed regardless of whether this throws?

Comment 9

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Clint Talbert ( :ctalbert ) from comment #8)
> Comment on attachment 775074 [details] [diff] [review]
> interpolate static files
> 
> Review of attachment 775074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js
> @@ +1328,5 @@
> > +      data += str.value;
> > +    } while (read != 0);
> > +  }
> > +  cstream.close(); // this closes fstream as well
> > +
> 
> A quick drive by comment:
> I like the simplicity of this approach. Seems easier than dealing with a
> bunch of sjs files. This part here worries me, though. What if this throws?
> Shouldn't we use try/catch/finally to ensure this stream is closed
> regardless of whether this throws?

I was thinking that if something in there throws then the test fails and that's it. I will wrap that in a try/catch/finally statement.

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports

10 of the tests in the ini file have to be run sequentially. Will file a
bug about those after we land the parallel harness.

Comment 11

[Mihnea Dobrescu-Balaur (:mihneadb)]

:Mossop, should I flag you or :Unfocused for r? ?

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 775938 [details] [diff] [review]
use dynamic ports

Flagging both of you per review, as told in IRC by :Unfocused.

Thanks!

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #12)
> Comment on attachment 775938 [details] [diff] [review]
> use dynamic ports
> 
> Flagging both of you per review, as told in IRC by :Unfocused.
> 
> Thanks!

It's a lot of changes, but here's the summary (it applies to all files except for the ones that are related to the 10 tests that are run sequentially):

* changed 4444 to %PORT% in all static files
* for every test that uses a httpserver, start the server using a dynamic port
* register all the used static files with the server using our custom handler
* the custom handler respond with the static file over which it applied a match and replace regex that basically substitutes %PORT% with the value of gPort (found at runtime)

Comment 15

[Dave Townsend [:mossop]]

FYI I'm unlikely to get to this this week due to a work week

Comment 16

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports and interpolate static files

Rebased my patch so it would apply on the last m-c.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 775938 [details] [diff] [review]
use dynamic ports

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

There's a few files still referencing port 4444 related to tests that aren't run sequentially - ideally we should be able to verify that only files referencing port 4444 are related to tests we purposefully set to run sequentially. 

For the ones I looked at, some (nearly all?) of URLs aren't actually used, but they should be cleaned up anyway - otherwise it's going to cause confusion in the future. 
For example:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/addons/test_bug470377_1/install.rdf#16
Should be able to just remove the em:updateURL property, since the test doesn't use it (for that specific test, at least - the file it points to doesn't even exist).

test_bug299716_2.js should be skipped - it uses a hardcoded port in the install.rdf. The test does an update check and checks nothing changes - sadly, it looks like the test also passes if the update check fails, which seems to be what's happening. We should make that test more robust in a separate followup (it'll require an API change - out of scope here).

Ditto with test_bug335238.js

::: toolkit/mozapps/extensions/test/xpcshell/test_bug559800.js
@@ +42,1 @@
>  }

This should be:
  gServer.stop(do_test_finished);


Ditto for the same change elsewhere, like test_dss.js

::: toolkit/mozapps/extensions/test/xpcshell/test_mapURIToAddonID.js
@@ +76,5 @@
>  
>    // Create and configure the HTTP server.
>    testserver = new HttpServer();
>    testserver.registerDirectory("/addons/", do_get_file("addons"));
> +  testserver.start(-1);

Er, I don't think the HTTP server is even used in this test...

::: toolkit/mozapps/extensions/test/xpcshell/test_migrate3.js
@@ +121,5 @@
> +    homepageURL: "http://localhost:" + gPort + "/data/index.html",
> +    headerURL: "http://localhost:" + gPort + "/data/header.png",
> +    footerURL: "http://localhost:" + gPort + "/data/footer.png",
> +    previewURL: "http://localhost:" + gPort + "/data/preview.png",
> +    iconURL: "http://localhost:" + gPort + "/data/icon.png"

These URLs aren't requested - they just need to be valid URs. So don't need to create a server; don't even need to specify a port even.

::: toolkit/mozapps/extensions/test/xpcshell/test_migrateAddonRepository.js
@@ +75,5 @@
>  
>    stmt.params.addon_internal_id = 1;
>    stmt.params.num = 0;
> +  stmt.params.url = "http://localhost:" + gPort + "/full1-1.png";
> +  stmt.params.thumbnailURL = "http://localhost:" + gPort + "/thumbnail1-1.png";

Same as test_migrate3.js

::: toolkit/mozapps/extensions/test/xpcshell/test_theme.js
@@ +258,5 @@
> +    homepageURL: "http://localhost:" + gPort + "/data/index.html",
> +    headerURL: "http://localhost:" + gPort + "/data/header.png",
> +    footerURL: "http://localhost:" + gPort + "/data/footer.png",
> +    previewURL: "http://localhost:" + gPort + "/data/preview.png",
> +    iconURL: "http://localhost:" + gPort + "/data/icon.png"

Same as test_migrate3.js

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 778592 [details] [diff] [review]
use dynamic ports and interpolate static files

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

Seemed easier to just finish my original review of the old patch. Since this revision is just dealing with bitrot (I compared the two, and didn't see anything substantially different), the review in comment 17 applies equally here.

Comment 19

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports and interpolate static files

Comment 20

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Blair McBride [:Unfocused] from comment #17)
> Comment on attachment 775938 [details] [diff] [review]
> use dynamic ports
> 
> Review of attachment 775938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's a few files still referencing port 4444 related to tests that aren't
> run sequentially - ideally we should be able to verify that only files
> referencing port 4444 are related to tests we purposefully set to run
> sequentially. 

I grepped and couldn't find any such files after fixing the stuff you suggested in the review. Can you please give me a list of these files that you are talking about?


> 
> For the ones I looked at, some (nearly all?) of URLs aren't actually used,
> but they should be cleaned up anyway - otherwise it's going to cause
> confusion in the future. 
> For example:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/addons/test_bug470377_1/install.rdf#16
> Should be able to just remove the em:updateURL property, since the test
> doesn't use it (for that specific test, at least - the file it points to
> doesn't even exist).

I didn't really touch the files (other than swapping out 4444 to %PORT%) since I don't know all the code and its behaviour. Wouldn't it be better to file a follow up bug after this where someone who knows more about the extensions subtree can offer some guidance on this?

> 
> test_bug299716_2.js should be skipped - it uses a hardcoded port in the
> install.rdf. The test does an update check and checks nothing changes -
> sadly, it looks like the test also passes if the update check fails, which
> seems to be what's happening. We should make that test more robust in a
> separate followup (it'll require an API change - out of scope here).
> 
> Ditto with test_bug335238.js

So you mean skip these tests completely, or just run them seq?


// fixed the other quirks.

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #20)
> I grepped and couldn't find any such files after fixing the stuff you
> suggested in the review. Can you please give me a list of these files that you are talking about?

Just went through everything again - seems everything in tests/xpcshell is ok, so I must have just seen data files in tests/addons (which are only used by the xpcshell tests).

> I didn't really touch the files (other than swapping out 4444 to %PORT%)
> since I don't know all the code and its behaviour. Wouldn't it be better to
> file a follow up bug after this where someone who knows more about the
> extensions subtree can offer some guidance on this?

My concern is that without going through those cases, there could be other tests like test_bug299716_2.js that are passing but are actually broken, and we won't know about them. We need to avoid the tree ever being in that state.

For instance, looking at test_blocklistchange.js, the XPI files it uses all have hardcoded ports for the updateURL. Yet the test is passing. Looking at the test, it never checks for updates for those XPIs, so the update property should be removed.

I think most of the cases should be pretty simple to figure out. They'll be like:
* updateURL in install.rdf - check if the test uses the URL. If it does, test is probably broken. If it doesn't, the property should be removed.
* URL for icons, lightweight theme images, etc - if these tests are passing, they should be fine (nearly all are dummy URLs). Change the port to 5555, re-run test to confirm its still passing.

Whatever is left, I can help look through to see if the test is behaving correctly.

> So you mean skip these tests completely, or just run them seq?

Sorry, I mean run them sequentially.

Comment 22

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports and interpolate static files

Let me know if there's anything missing.

I also deleted some static files that weren't being used.

Comment 23

[Mihnea Dobrescu-Balaur (:mihneadb)]

https://tbpl.mozilla.org/?tree=Try&rev=c542d4341fa3

Comment 24

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports and interpolate static files

I found a few more dummy URLs. Let me know if there's others.

The ones that are still left are from tests that are run sequentially anyway and
if I delete them (or change the port) the test fails.

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 780802 [details] [diff] [review]
use dynamic ports and interpolate static files

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

r+ with the following fixup:

::: toolkit/mozapps/extensions/test/xpcshell/test_bug559800.js
@@ +39,5 @@
>  }
>  
>  function end_test() {
>    do_test_finished();
> +  gServer.stop(do_test_finished);

do_test_finished should only be called as a callback to stop() in these cases - not before that as well. ie, the whole function should be:

function end_test() {
  gServer.stop(do_test_finished);
}

Comment 26

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use dynamic ports and interpolate static files

Updated.

Comment 27

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 783838 [details] [diff] [review]
use dynamic ports and interpolate static files

keeping r+

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/408a7705c7a5

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/408a7705c7a5