Open Bug 996652 Opened 5 years ago Updated 2 years ago

Scope the private symbols of head.js

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: Yoric, Assigned: waffles, Mentored)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [lang=js][good second bug])

Attachments

(1 file, 11 obsolete files)

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.
Blocks: 1003168
Whiteboard: [mentor=Yoric][lang=js] → [mentor=Yoric][lang=js][good second bug]
Mentor: dteller
Whiteboard: [mentor=Yoric][lang=js][good second bug] → [lang=js][good second bug]
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.
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.
Thanks Ted, 

I'm going to follow the built instructions, and then ask the remaining questions on the IRC.
:Yoric, is this feature still required?  sometimes bugs get overlooked when functionality changes.
Flags: needinfo?(dteller)
Yes, this is still something we'd like to do. It's more of a code hygiene issue than a feature.
Flags: needinfo?(dteller)
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?
Flags: needinfo?(dteller)
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);
Flags: needinfo?(dteller)
Attached patch xpcshell-head.patch (obsolete) — Splinter Review
Hey Yoric, the patch's `diff` looks like a mess! You should check it out... (Please tell me I'm right!) :P
Attachment #8577568 - Flags: review?(dteller)
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`...
Can you pastebin the logs of the test?
Assignee: nobody → wafflespeanut
(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 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
Attachment #8577568 - Flags: review?(dteller)
Attached patch xpc-selftest.patch (obsolete) — Splinter Review
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... :)
Attachment #8577568 - Attachment is obsolete: true
Attachment #8577687 - Flags: review?(dteller)
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
Attachment #8577687 - Flags: review?(dteller) → feedback+
So, for instance, try
 ./mach build testing/xpcshell
 ./mach xpcshell-test testing/xpcshell/example/unit/test_check_nsIException_failing.js 
as this test fails.
Yoric: The test passes! - http://pastebin.com/uDEyXy7u (But, it should fail, right?)
Flags: needinfo?(dteller)
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.
Flags: needinfo?(dteller) → needinfo?(wafflespeanut)
Running test (linux-64) for the revised patch in our try server - https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb91a201b4bf
Flags: needinfo?(wafflespeanut)
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.
Attached patch xpc-selftest.patch (obsolete) — Splinter Review
Made the required symbols public (based on error log)
Attachment #8577687 - Attachment is obsolete: true
Attachment #8580331 - Flags: review?(dteller)
Attachment #8580331 - Flags: review?(dteller)
The error logs have some tests which pass unexpectedly - https://treeherder.mozilla.org/#/jobs?repo=try&revision=aeee829871d4
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?
Flags: needinfo?(dteller)
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`)
Do you get any interesting information when running these tests from the command-line?

./mach xpcshell-test
Flags: needinfo?(dteller)
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 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.
Attached patch xpc-selftest.patch (obsolete) — Splinter Review
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
Attachment #8580331 - Attachment is obsolete: true
Attached patch xpc-selftest.patch (obsolete) — Splinter Review
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
Attachment #8584654 - Attachment is obsolete: true
Hey Yoric, the building and testing has successfully completed. What do you think?
Flags: needinfo?(dteller)
I'm optimistic. Can you now launch a try job also on macosx64 and win32?

Also, can you attach the latest version?
Flags: needinfo?(dteller)
Attached patch xpc-selftest.patch (obsolete) — Splinter Review
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 :)
Attachment #8584699 - Attachment is obsolete: true
Attachment #8585032 - Flags: review?(dteller)
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)
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 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.
Attachment #8585032 - Flags: review?(dteller) → feedback+
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...
Flags: needinfo?(dteller)
Attached patch xpc-selftest.patch (obsolete) — Splinter Review
Hey Ted, Can you please try to review my patch? :)
Attachment #8585032 - Attachment is obsolete: true
Attachment #8586296 - Flags: review?(ted)
Attachment #8586296 - Flags: feedback?(dteller)
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?
Attachment #8586296 - Flags: feedback?(dteller) → feedback+
Flags: needinfo?(dteller)
Attached patch xpc-selftest.patch (obsolete) — Splinter Review
Oops... sorry, I missed that. Fixed now :)
Attachment #8586296 - Attachment is obsolete: true
Attachment #8586296 - Flags: review?(ted)
Attachment #8587162 - Flags: review?(dteller)
Attachment #8587162 - Flags: review?(dteller) → review?(ted)
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.
Attachment #8587162 - Flags: review?(ted) → review+
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.
Flags: needinfo?(ted)
Ah, right, the two spaces should be on the line that starts with `exports[...]`, not on that line.
Attached patch xpcshell-test (obsolete) — Splinter Review
Okay, my bad, I'm sorry about that. I've fixed that now :)
Attachment #8587162 - Attachment is obsolete: true
Attachment #8606605 - Flags: review?(ted)
Attached patch xpcshell-test (obsolete) — Splinter Review
Oops, I missed a line at the last. Added now :)
Attachment #8606605 - Attachment is obsolete: true
Attachment #8606605 - Flags: review?(ted)
Attachment #8606607 - Flags: review?(ted)
Ted, please, could you review this?
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.
Flags: needinfo?(ted)
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.
Attachment #8606607 - Flags: review?(ted) → review+
Once you have done this, we will finally land the patch.
Flags: needinfo?(wafflespeanut)
Attached patch xpcshell-test (obsolete) — Splinter Review
I'm terribly sorry that I forgot to remove those indents. It's fixed now, and I've also added Ted to the reviewers :)
Attachment #8606607 - Attachment is obsolete: true
Flags: needinfo?(wafflespeanut)
Attachment #8609555 - Flags: review?(dteller)
Attachment #8609555 - Flags: review?(dteller) → review+
Very well Yoric, thanks for mentoring me :)
Flags: needinfo?(dteller)
My pleasure.
I'm sure we have many other bugs regarding xpcshell, if you are interested.
Flags: needinfo?(dteller)
Waffles, have you seen that there is a problem with your patch?
Flags: needinfo?(wafflespeanut)
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"...
Flags: needinfo?(wafflespeanut) → needinfo?(dteller)
Attached patch xpcshell-testSplinter Review
Again, I'm sorry for any problems caused.
Attachment #8609555 - Attachment is obsolete: true
Attachment #8614813 - Flags: review?
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
Attachment #8614813 - Flags: review? → review+
(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...)
Flags: needinfo?(dteller)
Flags: needinfo?(dteller)
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?
Flags: needinfo?(dteller) → needinfo?(wafflespeanut)
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.
Flags: needinfo?(wafflespeanut) → needinfo?(dteller)
What happens if you just run
 ./mach xpcshell-test dom/plugins/test/unit/test_plugin_default_state_xpi.js
?
Flags: needinfo?(dteller)
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`
Flags: needinfo?(dteller)
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
Flags: needinfo?(dteller)
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.
Flags: needinfo?(dteller)
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?
Flags: needinfo?(dteller) → needinfo?(wafflespeanut)
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 :/
Flags: needinfo?(wafflespeanut) → needinfo?(dteller)
Right now, I have no idea what's going on.
Flags: needinfo?(dteller)
Any suggestion, ted?
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
You need to log in before you can comment on or make changes to this bug.