772272 - Remove do_load_httpd_js from xpcshell tests

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Add deprecation notice

Now that we can load testing-only JS modules and can load httpd.js as one (bug 755196), we have no need for the hack that is do_load_httpd_js() in xpcshell's head.js.

This trivial patch adds a deprecation notice. We can follow-up with actual removal. In the mean time, hopefully this will prevent people from introducing new usages that need to be cleaned later.

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 640408 [details] [diff] [review]
Add deprecation notice

This isn't Core :: Build

Comment 2

[(not currently active) Ted Mielczarek]

I don't see that this is going to do anything useful, you don't even get stdout for tests unless you run check-one or your test fails. I'd rather we just remove it and replace with the C.u.import or whatever.

Comment 3

[Gregory Szorc [:gps]]

Ted: That's fair. Shifting bug appropriately.

To whoever picks this up, the steps that need to be performed are to identify every usage of do_load_httpd_js(). See https://mxr.mozilla.org/mozilla-central/search?string=do_load_httpd_js

For each, at the top of the file, add:

Components.utils.import("resource://testing-common/httpd.js");
or
Cu.import("resource://testing-common/httpd.js");

Then, change every instance of "new nsHttpServer" with "new HttpServer".

It is possible some tests may break while refactoring because they are accessing symbols not exported from the httpd.js module (they could access them before because httpd.js was included as a file and thus all symbols were available). I'm fine with filing follow-up bugs for these.

Since there are 157 usages of do_load_httpd_js (quite a lot), if you only want to tackle a subset in this bug, that's fine: we can always file a follow-up to address the remaining ones. I'd suggest the toolkit/mozapps/extensions as a good starting batch, as that one is less likely to break than netwerk.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 640408 [details] [diff] [review]
Add deprecation notice

Clearing review per discussion.

Comment 5

[Mark Capella [:capella]]

I'm already working with toolkit/mozapps/extensions to remove nsILocalFiles references, so this sounds like a good followup bug.

I'll start there with it's 57 bugs, and expand as I go to get the last 100 or so in the other folders.

I'm interested in seeing what "tests may break while refactoring because they are accessing symbols not exported from the httpd.js module", and might like to also help clean that up.

fyi - I've completed a bunch of c++ bugs but have just started working with JS / XPCOM / XPCShell etc, so trivial bug for me is still a learning opportunity -- mark

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v1) - Extensions

This patch removes all references in the extensions folder. Looking for feedback from the mentor as a first step. Then we can either expand the patch for the rest of the occurrences or submit for approval for partial move with the rest to follow on.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 649102 [details] [diff] [review]
Patch (v1) - Extensions

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

This approach looks good to me!

I haven't looked at every file, but assuming it passes Try and the rest of the patch doesn't contain crazy things I haven't seen yet, this could probably be a r+.

If making additional changes, please keep patches more or less separated according to components they touch. I don't want to review a monolithic patch ;)

Comment 8

[Mark Capella [:capella]]

Attachment: Patch - Extensions

Requesting approval for the first patch.

Rebased, tested on TRY:
https://tbpl.mozilla.org/?tree=Try&rev=40b012cb80e2

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 649623 [details] [diff] [review]
Patch - Extensions

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

I skimmed this patch, it seems mostly mechanical. As long as it passes Try then r=me.

My only real concern is that it might make backporting tests to other branches harder. I guess we could always backport the testing module change if it's an issue.

::: toolkit/mozapps/extensions/test/unit/test_bug463819.js
@@ +18,5 @@
>  // This allows the EM to attempt to display errors to the user without failing
>  var promptService = {
>    alert: function(aParent, aDialogTitle, aText) {
>    },
>    

Can you kill the whitespace on this line while you're here? There's existing trailing whitespace in a few other spots as well. If it's not too much trouble to clean those up that'd be nice, but don't kill yourself over it. (Splinter shows these now and now it drives me nuts!)

Comment 10

[Mark Capella [:capella]]

"Kill whitespace" ... not a problem!

I tweaked 7 or 8 files where trailing whitespace shows up in the DIFF. The first example was tricky, as fixing the line that showed up in the DIFF, expanded the size of the DIFF, which then showed another line further down with trailing whitespace, and fixing it then expands .... blah, blah, blah :P

I have a utility I wrote to identify and optionally remove those buggers, but it operates file-wide and then the review is even worse.

The first TRY push worked fine, but I'm going to test this locally again, re-post it here and push to TRY again just to be safe, and pending that move it to inbound keeping the bug open for the rest of the (folder based) patches to follow (with additional review requests of course).

Let me know if you have any further requests -- mark

Comment 11

[Mark Capella [:capella]]

Push to TRY with witespace nits
https://tbpl.mozilla.org/?tree=Try&rev=efdb841ac0e9

Comment 12

[Mark Capella [:capella]]

First patch extensions directory
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3029842b3204

Comment 13

[Mark Capella [:capella]]

Attachment: Patch - Extensions and nits

Comment 14

[Ed Morley [:emorley]]

Adjusting whiteboard to [leave open] rather than [do not close], since the merge tool will otherwise close the bug. See:
https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound
or
https://groups.google.com/d/topic/mozilla.dev.platform/DQyQ56DgmhA/discussion

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3029842b3204

Comment 16

[Mark Capella [:capella]]

Attachment: Patch - Toolkit

This patch completes changes for the toolkits folder, and corrects an overlooked symbol definition (Cr / Components.results) for a couple cases from the previous patch that snuck past TRY.

This patch pushed to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=e5d5112e4bfc

Comment 17

[Gregory Szorc [:gps]]

(In reply to Mark Capella [:capella] from comment #16)
> This patch completes changes for the toolkits folder, and corrects an
> overlooked symbol definition (Cr / Components.results) for a couple cases
> from the previous patch that snuck past TRY.

Did the old tests fail without Cr defined? If not, there's no need to define it!

Comment 18

[Mark Capella [:capella]]

Could be my inexperience with JS ... for example I noticed in test_bug542391.js there is use of "throw Cr.NS_ERROR_NO_INTERFACE;" ...

Cr. is defined along with the other Cc, Ci. Cu. vars etc. in the httpd.js file. I had to add some of these definitions to the previous patch to make the tests work, as removing the do_load_httpd_js() function call seemed to un-define them.

Maybe Cr. is available for another reason and my re-introducing it isn't required. I thought perhaps the TRY tests passed, as the code for the "throw" wasn't executed, and therefore the absence of the Cr. definition wasn't noticed.

Comment 19

[Gregory Szorc [:gps]]

httpd.js defines Cc, Ci, Cu, etc. do_load_httpd_js() effectively #include's httpd.js. Old tests calling do_load_httpd_js() often relied on this side-effect.

Comment 20

[Jason Duell]

Might be nice to defined Cc/Ci etc in head.js instead?

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jason Duell (:jduell) from comment #20)
> Might be nice to defined Cc/Ci etc in head.js instead?

Yes. Though I'm betting that breaks a *lot* of tests, due to re-declaring consts.

Comment 22

[Jason Duell]

Indeed.  But fixing should be straightforward, perhaps even scritable.

Comment 23

[Gregory Szorc [:gps]]

(In reply to Blair McBride (:Unfocused) from comment #21)
> (In reply to Jason Duell (:jduell) from comment #20)
> > Might be nice to defined Cc/Ci etc in head.js instead?
> 
> Yes. Though I'm betting that breaks a *lot* of tests, due to re-declaring
> consts.

Yes, it definitely would.

I'm a proponent of minimizing the number of "global variables" in xpcshell test runner's head.js. If a component wants to universally defined Cc/Ci, etc, it can define it's own [head] file in its xpcshell.ini manifest and that file can define whatever it needs.

Comment 24

[Jason Duell]

Smells like lots of code duplication to me.  But not my call.

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Normally I'd agree, but I think Ci/Cc/etc is a special case. We've been entertaining the idea of globally defining those aliases for awhile, so they'd be automatically available wherever Components is available. 

But saying that, doing it per-module might be an easier middle step towards eventually doing it for all xpcshell tests.

Comment 26

[Mark Capella [:capella]]

Attachment: Patch - Misc

Ok then! This one covers Content, DOM, Image, JS, RDF, and URILoader folders, basically everything except for the final patch which will address the netwerk/test folder.

Noticing the above discussions (thx for the feedback!) I'm still inserting the Cc Ci. Cu. Cr. per-module / where required as do_load_httpd_js() references are removed. 

Push to TRY status for this patch is:
https://tbpl.mozilla.org/?tree=Try&rev=b619663c6a1a

Comment 27

[Mark Capella [:capella]]

Attachment: Patch - Netwerk

This netwerk patch finishes all of the occurrences of do_load_httpd_js calls as erquested in the bug description, except for in the case of /netwerk/test/httpserver/test/head_utils.js which controls tests in the /netwerk/test/httpserver subdirectory.

Just cutting / pasting approach doesn't work here ... and I've not looked deeper yet other than quick testing the change and seeing failures in netwerk/test/httpserver/test/test_byte_range.js, netwerk\test\httpserver\test\test_default_index_handler.js, and etc.

TRY push for this includes: https://tbpl.mozilla.org/?tree=Try&rev=64b3adacd26d

Comment 28

[(not currently active) Ted Mielczarek]

Comment on attachment 650479 [details] [diff] [review]
Patch - Toolkit

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

I'm kind of torn on the "add Cc/Ci/Cu to every test file", but relying on it to come in from importing httpd.js is kind of crazy. We should probably just change xpcshell to put those constants in scope for every file it loads.

Comment 29

[(not currently active) Ted Mielczarek]

Comment on attachment 651219 [details] [diff] [review]
Patch - Netwerk

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

My eyes are glazing over, I didn't review this whole patch, but I trust what you're doing.

Comment 30

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=43a364657554

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e3bbc5048ee
https://hg.mozilla.org/mozilla-central/rev/7ffbeea756f2
https://hg.mozilla.org/mozilla-central/rev/43a364657554

Comment 32

[Mark Capella [:capella]]

Quick status check to see where we are / and how much is left to be done for this patch.

Three references now remain regarding do_load_httpd_js() in files:

   /testing/xpcshell/example/unit/test_load_httpd_js.js
      Defines example code, another instance I could change as above.

   /testing/xpcshell/head.js
      Defines the function we're trying to remove

   /netwerk/test/httpserver/test/head_utils.js
      Helps xpcshell tests run (comment 27) for the /netwerk/test/httpserver subdirectory. 


In the last file, it looks like large portions of code would have to be duplicated down into /httpserver tests if the do_load() function was removed from the head. I'm wondering if you intended this patch to do that.

Comment 33

[Gregory Szorc [:gps]]

If all that's remaining is the bit in netwerk/, then I'd vote for:

1) Remove usage from the example file. Perhaps replace with Cu.import(...).
2) Move do_load_httpd_js()'s functionality from testing/xpcshell/head.js into netwerk/test/httpserver/head_utils.js.
3) Create a follow-up bug against netwerk for them to use Cu.import(). (It might get resolved WONTFIX).

#2 might be a little involved. If it is, I'm OK with some hackery as long as do_load_httpd_js() disappears as a global test function.

Comment 34

[(not currently active) Ted Mielczarek]

(In reply to Gregory Szorc [:gps] from comment #33)
> #2 might be a little involved. If it is, I'm OK with some hackery as long as
> do_load_httpd_js() disappears as a global test function.

It's probably less complicated if it's just netwerk, since httpd.js can just be copied into the test directory there. The only reason for the goofy hackery was so that we could find a path to load it from any test in the tree, which was tricky with packaged tests (which is where this all came to be).

Comment 35

[Mark Capella [:capella]]

Attachment: Patch - Final

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

Wait... this patch works ... there's an httpd.js already in C:\Users\Master\mozilla-central\netwerk\test\httpserver\

Comment 36

[Gregory Szorc [:gps]]

(In reply to Mark Capella [:capella] from comment #35)
> Wait... this patch works ... there's an httpd.js already in
> C:\Users\Master\mozilla-central\netwerk\test\httpserver\

That's where httpd.js lives! It just gets installed into the test directory as part of the build. See bug 755196.

Comment 37

[Mark Capella [:capella]]

Comment on attachment 652417 [details] [diff] [review]
Patch - Final

Well TRY is taking its time for the WIN builds, but that's what I develop / test on so if it fails the X tests there I'll be surprised. 

I'm adding the review? flag for now ...

Comment 38

[Mark Capella [:capella]]

Ping the reviewer ... hoping we can complete this patch...

Comment 39

[(not currently active) Ted Mielczarek]

Comment on attachment 652417 [details] [diff] [review]
Patch - Final

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

::: netwerk/test/httpserver/test/head_utils.js
@@ +7,5 @@
> +/**
> + * Loads _HTTPD_JS_PATH file, which is dynamically defined by
> + * <runxpcshelltests.py>.
> + */
> +load(_HTTPD_JS_PATH);

Can we not load this with a relative path here, since httpd.js should get installed to this directory (or somewhere near it)?

Comment 40

[Mark Capella [:capella]]

Hmmm .. on testing, it seems the _HTTPD_JS_PATH const is populated with something like:

|c:/Users/Master/mozilla-central/obj/dist/bin/components/httpd.js|

Simply loading via the below (assuming this is what you were asking), doesn't work.
load('httpd.js');

Comment 41

[(not currently active) Ted Mielczarek]

Ah, for some reason I thought it got installed into the xpcshell tests directory, but I guess not. I think back before I rewrote everything do_get_file used to take topsrcdir-relative paths, so all the tests imported it directly from network/test/httpserver/httpd.js.

Comment 42

[Mark Capella [:capella]]

Thanks ! I'll land it as-is then.

Btw- Scanning my local repo I find 9 versions of httpd.js floating around ( 3 source and 6 OJDIR / "peptest" "reftest" etc "mochitest" ... )

Comment 43

[Mark Capella [:capella]]

Final patch ... heading to inbound ...
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3110f58a46f4

Comment 44

[Gregory Szorc [:gps]]

(In reply to Mark Capella [:capella] from comment #43)
> Final patch ... heading to inbound ...
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3110f58a46f4

\o/ Thank you so much for all the work!

(In reply to Mark Capella [:capella] from comment #42)
> Thanks ! I'll land it as-is then.
> 
> Btw- Scanning my local repo I find 9 versions of httpd.js floating around (
> 3 source and 6 OJDIR / "peptest" "reftest" etc "mochitest" ... )

These were all created before we had the concept of shared testing-only JS modules. We can (and should) consolidate (some) of these. The initial work will likely involve adding support for testing-only JS modules to each test runner and will probably look a lot like bug 759664.

Comment 45

[Gregory Szorc [:gps]]

Updated https://developer.mozilla.org/en-US/docs/Httpd.js/HTTP_server_for_unit_tests and https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3110f58a46f4