996652 - Scope the private symbols of head.js

Status: RESOLVED WONTFIX

(Testing :: XPCShell Harness, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

One of our testing mechanisms, xpcshell, relies on the JavaScript code of file head.js http://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js

Unfortunately, many symbols defined in this file are public. They should be private. The objective of this bug is to wrap all of the code of head.js in a function and only export the symbols that need to be made public.

I believe that this is a good mentored bug, for someone who already understands JavaScript scoping.

Comment 1

[Ismael Jimenez]

Hi I would like to take this bug.

Will be my first in Mozilla!!! Very excited to be part of it.

Just let me know how to start.

Comment 2

[(not currently active) Ted Mielczarek]

Hi Ismael! As a first step, you'll need to have built Firefox locally. If you haven't done so already there are instructions here:
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Once you've done that, you can ask here for further instructions, or ask on irc.mozilla.org #introduction.

Comment 3

[Ismael Jimenez]

Thanks Ted, 

I'm going to follow the built instructions, and then ask the remaining questions on the IRC.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

:Yoric, is this feature still required?  sometimes bugs get overlooked when functionality changes.

Comment 5

[(not currently active) Ted Mielczarek]

Yes, this is still something we'd like to do. It's more of a code hygiene issue than a feature.

Comment 6

[Ravi Shankar [:waffles]]

Hey Yoric, I can work on this bug. But, please help me understand a few things. What do you mean by symbols? I don't think that this is the symbol (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol) I should be looking for. In any way, I can't locate them in the `head.js` file.

And, by "wrapping all the code", did you mean that the entire code in `head.js` should be wrapped into a single function? Uh, can you get me started with this bug?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

By "symbols", I mean all the global variables, i.e. `_quit`, `_passed`, etc.

And yes, we need to wrap everything in a single function, something like:

(function(exports) {
  var _quit = false; // This variable is now hidden.
  // ... everything else in head.js

  // Now export all the public symbols
  exports.do_print = do_print;
  exports.do_timeout = do_timeout;
  // ...
})(this);

Comment 8

[Ravi Shankar [:waffles]]

Attachment: xpcshell-head.patch

Hey Yoric, the patch's `diff` looks like a mess! You should check it out... (Please tell me I'm right!) :P

Comment 9

[Ravi Shankar [:waffles]]

Oops. Yoric, I seem to have missed `add_task` and `add_test` in the patch above. But, even if we put those into the giant function (and make them public), I still get 23 failures when I run the `selftest.py`...

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Can you pastebin the logs of the test?

Comment 11

[Ravi Shankar [:waffles]]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #10)
> Can you pastebin the logs of the test?

Yoric: Here you go... http://pastebin.com/LS0r9BVS

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8577568 [details] [diff] [review]
xpcshell-head.patch

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

Can you repost the same patch, but without adding whitespace at the start of every line? This should make it easier to review, plus the whitespace changes make history of the code harder to track

Comment 13

[Ravi Shankar [:waffles]]

Attachment: xpc-selftest.patch

I understand and I'm sorry about that. I thought the content inside the function body requires indentation, but I didn't realize that it'd be harder to read. I've removed the indentations now... :)

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8577687 [details] [diff] [review]
xpc-selftest.patch

This looks good.
Not giving r+ until we have found out what causes the test failures.

The selftest error messages don't help much, so next attempt is to find other tests that fail, with clearer error messages.

I have launched all xpcshell tests on Linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a0121f9ae5

Comment 15

[David Teller [:Yoric] - still alive but not very active]

So, for instance, try
 ./mach build testing/xpcshell
 ./mach xpcshell-test testing/xpcshell/example/unit/test_check_nsIException_failing.js 
as this test fails.

Comment 16

[Ravi Shankar [:waffles]]

Yoric: The test passes! - http://pastebin.com/uDEyXy7u (But, it should fail, right?)

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Mmmh...
Yeah, it should fail.

I just realized that you should also export `_execute_test`. We'll probably want to scope this better, eventually, but let's see if this improves the situation.

Comment 18

[Ravi Shankar [:waffles]]

Running test (linux-64) for the revised patch in our try server - https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb91a201b4bf

Comment 19

[David Teller [:Yoric] - still alive but not very active]

So, if you look at the log, you may notice:

* ReferenceError: _wrap_with_quotes_if_necessary is not defined at /builds/slave/test/build/tests/xpcshell/tests/dom/encoding/test/unit/head.js:19 

* ReferenceError: runningInParent is not defined at /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js:87 

*  ReferenceError: _quit is not defined at /builds/slave/test/build/tests/xpcshell/tests/storage/test/unit/test_statement_executeAsync.js:1005

* ReferenceError: _setupDebuggerServer is not defined at /builds/slave/test/build/tests/xpcshell/tests/toolkit/devtools/server/tests/unit/test_xpcshell_debugging.js:12 

And possibly a few others.

So, let's do the following:
1. make these symbols public;
2. file a bug to add a public `do_quit` to head.js instead of `_quit` and replace `_quit` in tests that use it with `do_quit`;
3. same with `_wrap_in_quotes_if_necessary`;
4. same with `_setupDebuggerServer`;
5. test again on Try.

Comment 20

[Ravi Shankar [:waffles]]

Attachment: xpc-selftest.patch

Made the required symbols public (based on error log)

Comment 21

[Ravi Shankar [:waffles]]

The error logs have some tests which pass unexpectedly - https://treeherder.mozilla.org/#/jobs?repo=try&revision=aeee829871d4

Comment 22

[Ravi Shankar [:waffles]]

The error log shows an unexpected pass of these four tests:

1. test_check_nsIException_failing.js
2. test_fail.js
3. test_do_check_null_failing.js
4. test_do_check_matches_failing.js

Now, `test_check_nsIException_failing.js` is among them. Isn't the latest patch is supposed to make this test fail?

Comment 23

[Ravi Shankar [:waffles]]

The following bugs are filed for adding those public symbols:

1) Bug #1145404 (for `do_quit`)
2) Bug #1145419 (for `do_wrap_in_quotes_if_necessary`)
3) Bug #1145422 (for `do_setupDebuggerServer`)

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Do you get any interesting information when running these tests from the command-line?

./mach xpcshell-test

Comment 25

[Ravi Shankar [:waffles]]

Yep, out of 2184 tests, it showed the same 4 unexpected PASSes! On running the xpcshell-test specifically for each of the four tests, they showed the same errors. And, there wasn't any difference in their test log. So, I guess they're related.

For example, `test_do_check_null_failing.js` had this log (https://pastebin.mozilla.org/8826956), whereas `test_do_check_null.js` had this log (https://pastebin.mozilla.org/8826957), and they don't show any difference at all!

Even after adding a `dump('message')` at the start of the tests, they don't show the message in the log, which seems strange...

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8580331 [details] [diff] [review]
xpc-selftest.patch

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

::: testing/xpcshell/head.js
@@ +1424,5 @@
>    }
>  } catch (e) { }
> +
> +exports._execute_test = _execute_test;
> +exports._quit = _quit;

Oh, `_quit` is not a function but a variable, so this is not going to work. You should replace all instances of `_quit` with `exports._quit` – and then clean this up as part of bug 1145404.

Comment 27

[Ravi Shankar [:waffles]]

Attachment: xpc-selftest.patch

I've revised the patch and pushed to the try server. The `xpcshell-test` didn't show any errors in my local build, but let's see what the try server says...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21afe5754241

Comment 28

[Ravi Shankar [:waffles]]

Attachment: xpc-selftest.patch

Missed a symbol. It's now been corrected (made public), and the revision has been pushed to the try-server for testing...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a8196398dc4

Comment 29

[Ravi Shankar [:waffles]]

Hey Yoric, the building and testing has successfully completed. What do you think?

Comment 30

[David Teller [:Yoric] - still alive but not very active]

I'm optimistic. Can you now launch a try job also on macosx64 and win32?

Also, can you attach the latest version?

Comment 31

[Ravi Shankar [:waffles]]

Attachment: xpc-selftest.patch

Great! I've launched a try job for testing it on all platforms (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e7a7bde5ec9). By the way, here's the latest patch :)

Comment 32

[Ravi Shankar [:waffles]]

Oops, sorry. I realized that not all platforms are really necessary now. Here's the latest try-server push for macosx64 & win32 (https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ef5e8a6c7a9)

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Looks good. Let's take the opportunity for one last change.

If you look at http://hg.mozilla.org/mozilla-central/file/a10fbddc674c/testing/xpcshell/head.js#l497, you will see that liens 497-501 are used to export a few symbols without using `exports`. Can you

1/ move these lines to the end of the scoping function;
2/ replace `this` (in these lines only) with `exports`?

After that, one last test and we'll try to land your patch.

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8585032 [details] [diff] [review]
xpc-selftest.patch

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

Oh, I forgot one last thing.
I am not supposed to give r+ to code in directory testing/. So, once you are done with `Assert`, you should ask a review from ted instead.

Comment 35

[Ravi Shankar [:waffles]]

Sorry for the delay. I've updated my patch and pushed it to the try server (https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b5b40c318cf) for xpcshell-tests on linux65, macosx64, win32 platforms...

Comment 36

[Ravi Shankar [:waffles]]

Attachment: xpc-selftest.patch

Hey Ted, Can you please try to review my patch? :)

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8586296 [details] [diff] [review]
xpc-selftest.patch

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

::: testing/xpcshell/head.js
@@ +1468,5 @@
> +
> +// Tack Assert.jsm methods to the current scope.
> +exports.Assert = Assert;
> +for (let func in Assert) {
> +exports[func] = Assert[func].bind(Assert);

Nit: can you add two spaces at the start of this line?

Comment 38

[Ravi Shankar [:waffles]]

Attachment: xpc-selftest.patch

Oops... sorry, I missed that. Fixed now :)

Comment 39

[(not currently active) Ted Mielczarek]

Comment on attachment 8587162 [details] [diff] [review]
xpc-selftest.patch

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

I'm sorry for letting this sit for so long, this looks fine.

::: testing/xpcshell/head.js
@@ +1465,5 @@
> +exports.todo_check_neq = todo_check_neq;
> +exports.todo_check_null = todo_check_null;
> +exports.todo_check_true = todo_check_true;
> +
> +  // Tack Assert.jsm methods to the current scope.

The indentation here is a little odd.

Comment 40

[Ravi Shankar [:waffles]]

Phew, you're finally back. I indented that because Yoric asked me to add two spaces in those lines. Now, I'm worried whether my patch will get disturbed by the numerous changes made to `head.js` since my patch.

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Ah, right, the two spaces should be on the line that starts with `exports[...]`, not on that line.

Comment 42

[Ravi Shankar [:waffles]]

Attachment: xpcshell-test

Okay, my bad, I'm sorry about that. I've fixed that now :)

Comment 43

[Ravi Shankar [:waffles]]

Attachment: xpcshell-test

Oops, I missed a line at the last. Added now :)

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Ted, please, could you review this?

Comment 45

[David Teller [:Yoric] - still alive but not very active]

Oh, I hadnt realized that Ted had actually r+ed the previous patch. Since the only difference is in tabs, I'll proceed with the rest of the reviews.

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8606607 [details] [diff] [review]
xpcshell-test

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

Could you add ",r=ted" (without the quotes) at the end of your commit message?

::: testing/xpcshell/head.js
@@ +1542,5 @@
> +exports.todo_check_null = todo_check_null;
> +exports.todo_check_true = todo_check_true;
> +
> +// Tack Assert.jsm methods to the current scope.
> +  exports.Assert = Assert;

Just indent line 1548, not the lines around.

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Once you have done this, we will finally land the patch.

Comment 48

[Ravi Shankar [:waffles]]

Attachment: xpcshell-test

I'm terribly sorry that I forgot to remove those indents. It's fixed now, and I've also added Ted to the reviewers :)

Comment 49

[David Teller [:Yoric] - still alive but not very active]

Let's land this.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ef5e8a6c7a9

Comment 50

[Ravi Shankar [:waffles]]

Very well Yoric, thanks for mentoring me :)

Comment 51

[David Teller [:Yoric] - still alive but not very active]

My pleasure.
I'm sure we have many other bugs regarding xpcshell, if you are interested.

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d6ad2faa3a

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Backed out for checktest failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=10126935&repo=mozilla-inbound

Comment 54

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc99c65e5956

Comment 55

[David Teller [:Yoric] - still alive but not very active]

Waffles, have you seen that there is a problem with your patch?

Comment 56

[Ravi Shankar [:waffles]]

Yeah, I noticed it just now. Damn, this bug is haunting me for about 3 months! I seem to have left out something everytime (sigh). This time, the problem was due to a recent change in `head.js` and so I had to redo everything, when I forgot to add "this"...

Comment 57

[Ravi Shankar [:waffles]]

Attachment: xpcshell-test

Again, I'm sorry for any problems caused.

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8614813 [details] [diff] [review]
xpcshell-test

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

What's the difference with the previous patch, exactly?
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2694d861eb8d

Comment 59

[Ravi Shankar [:waffles]]

(comment #56) I forgot to add "this" at the end.

(It was caused by a recent pull which rejected my patch when I pushed it into the stack, and so I had rework my patch, but when I reached the end, I should've totally forgotten our scoping at the start, which explains the absence of "this" in the previous patch, silly me...)

Comment 60

[David Teller [:Yoric] - still alive but not very active]

Well, apparently, there is a problem with dom/plugins/test/unit/test_plugin_default_state_xpi.js . Waffles, can you reproduce it on your machine?

Comment 61

[Ravi Shankar [:waffles]]

I don't understand. Because, I did a rebuild after pulling the remote today. When I tested the file with ./mach xpcshell-test, I got "OK". There were no failures. And, here's the "giant" log: https://pastebin.mozilla.org/8835869.

Comment 62

[David Teller [:Yoric] - still alive but not very active]

What happens if you just run
 ./mach xpcshell-test dom/plugins/test/unit/test_plugin_default_state_xpi.js
?

Comment 63

[Ravi Shankar [:waffles]]

Um, that's what I actually did. If you checked the pastebin log (comment #61), you can notice that the test began with `./mach xpcshell-test dom/plugins/test/unit/test_plugin_default_state_xpi.js`

Comment 64

[David Teller [:Yoric] - still alive but not very active]

My bad, I misunderstood your "giant log" comment.

I relaunched the Try here just in case there was something wrong with my rebase:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0191e2488ff6

Comment 65

[Ravi Shankar [:waffles]]

The xpcshell-tests have failed. It looks like those tests are failing at the try-server, but not at my local repo (FYI, I tried testing the file again after pulling the latest changesets & updating my repo, and I still don't get an error). Meh, this is really confusing now.

Comment 66

[David Teller [:Yoric] - still alive but not very active]

Apparently, this is a shutdown-related issue. I still have no idea what causes it.
What happens if you do
 `./mach xpcshell-test dom/plugins/test/unit`
to run all the tests in that directory?

Comment 67

[Ravi Shankar [:waffles]]

I pulled everything and reran my build. When I ran the tests again, I get failures - both in the `dom/plugins/test/unit` directory and those individual files. And, this happens even before applying the patch. I also noticed that the patch doesn't change their results, because they have the same logs.

For instance, this is what I get now for `test_plugin_default_state_xpi.js` (https://pastebin.mozilla.org/8836641) and for the whole directory `dom/plugins/test/unit` (https://pastebin.mozilla.org/8836642) before & after the patch. I dunno why it said "OK" last time, but they fail now :/

Comment 68

[David Teller [:Yoric] - still alive but not very active]

Right now, I have no idea what's going on.

Comment 69

[David Teller [:Yoric] - still alive but not very active]

Any suggestion, ted?

Comment 70

[(not currently active) Ted Mielczarek]

I clearly neglected this bug and I'm sorry, but I haven't been working on these test harnesses actively in a while so I'm probably not the best person to look at this stuff anymore.

Comment 71

[BugBot [:suhaib / :marco]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jmaher, could you have a look please?
For more information, please visit auto_nag documentation.