Opened 7 years ago

Closed 2 years ago

#5290 closed task (wontfix)

Collect+write test pages for JavaScript hooks

Reported by: mikeperry Owned by: boklm
Priority: High Milestone:
Component: Applications/Quality Assurance and Testing Version:
Severity: Normal Keywords:
Cc: g.koppen@…, saint Actual Points:
Parent ID: #5292 Points:
Reviewer: Sponsor:

Description

Looks like Firefox changed some aspects of its JS scope resolution: https://bugzilla.mozilla.org/show_bug.cgi?id=632064

We should review that. Along the way, I should also collect the set of circumvention tests from Greg Fleischer as well as gather my own tests, and try to distill them down into a couple simple web pages.

I should also verify WebWorker scope and other newer JS tech still gets the hooks properly applied to it.

Child Tickets

Attachments (3)

hookingTests.zip (4.9 KB) - added by gk 6 years ago.
hookingTests2.zip (11.2 KB) - added by gk 6 years ago.
windowScreenTests.zip (12.1 KB) - added by gk 6 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 7 years ago by mikeperry

Parent ID: #5292

comment:2 Changed 7 years ago by gk

Cc: g.koppen@… added

comment:3 Changed 6 years ago by mikeperry

data: urls can also execute script. We'll definitely want to write tests for those, along with javascript: urls, too, of course.

comment:4 Changed 6 years ago by mikeperry

WebSockets now DNS leak. #5741. When I first reviewed the WebSocket code, it was ok. They've since added "Binary WebSockets", and I guess that code changed. We might want a test case for them now, too. The code will probably change again.

comment:5 Changed 6 years ago by mikeperry

Georg Koppen found a race condition in our JS hook code. We may just toss it all away in favor of patches, instead. See #5856 and #5857.

comment:6 Changed 6 years ago by mikeperry

Component: TorBrowserButtonQuality Assurance and Testing
Owner: changed from mikeperry to cypherpunks

comment:7 Changed 6 years ago by gk

Changing the owner to cypherpunks seems wrong to me. The one who fixes #5857 has to test that (i.e. write tests) anyway. Thus, there is no need for random people to contribute tests unless the cypherpunks own #5857 as well (which is not the case yet). A second concern to me is that it is error-prone if the developer does not write the tests himself but rather let third party folk do this. The dev is (hopefully) knowing the code better and thus better equipped to write useful/good tests.

comment:8 Changed 6 years ago by mikeperry

Yes, gk, from a best practice point of view, you are absolutely right. Dogma dictates that engineers should at least write their own unit tests. However, I am just barely skating above parity with opened bugs at this point. Any help I can get wrt people testing stuff for me is beyond useful.

Additionally, there is a natural mental barrier that you experience when you are forced to test your own code, rooted in incentive. If you find bugs in your own code, this only creates more work for you. No dopamine bonus there, only pain is your reward. A different individual, however, is more motivated to create *excellent* tests, to prove that they are useful by finding bugs (because of course, they are).

Further, I think I might go with a more direct solution for JS issues anyways (#5856). So... Meh?

comment:9 Changed 6 years ago by gk

Okay, here we go. I created some unit tests using Mozmill (https://developer.mozilla.org/en/Mozmill/) which I am going to attach in a second (yes, the current hooks are indeed not working properly if local files are loaded...). I'd further propose to use Mozmill for other Gecko based products as well (you know, new browsers and such kind of stuff ;) ). We could use it even for Torbirdy testing (which is not possible with Mochitest) and it allows us to test everything reachable by a XPCOM component. Thus, if there is ever need to write C++ unit tests that is kept to a minimum, I assume.
The .zip file I'll attach contains a runTests shell script where you have to enter your Torbrowser path manually in order to get the tests running (you could also test just the Torbutton xpi which just needs different parameters) (not very convenient yet).
I plan to convert Gregory Fleischer's and my own tests to Mozmill tests step by step as well depending on available spare time.

Changed 6 years ago by gk

Attachment: hookingTests.zip added

comment:10 Changed 6 years ago by gk

Oh, I forgot to add: Due to the fancy modal dialog shown on startup (really, a modal dialog on startup? Oh my god!) the tests are not running automatically yet: You have to close that dialog once on startup manually in order to run the test "suite".

comment:11 Changed 6 years ago by mikeperry

Keywords: MikePerry201208 added

gk: this is awesome. I am wondering two things: First, what made you pick mozmill over mochitest or xpcshell? Or for that matter, anything else out of https://developer.mozilla.org/en/Mozilla_automated_testing?

Second, what do you propose we should do with this zip? Does Firefox have Makefile rules for mozmill we should use? I know it has xpcshell and Mochitest.. If Makefiles are a poor plan, should we create a new testing repo for stuff like this?

comment:12 in reply to:  11 ; Changed 6 years ago by gk

Replying to mikeperry:

gk: this is awesome. I am wondering two things: First, what made you pick mozmill over mochitest or xpcshell? Or for that matter, anything else out of https://developer.mozilla.org/en/Mozilla_automated_testing?

Well, the overall goal I had in mind was to have just one testing framework for everything reachable by JavaScript. That includes add-on testing (Torbutton AND Torbirdy) and Torbrowser testing. Add-on testing (functionality and UI) is not possible with xpcshell and Mochitest is supposed to be a browser-based tool which therefore does not seem fit to the Torbirdy requirement (you'll find the browser-based argument and other interesting points about mochitest vs. mozmill here: https://groups.google.com/group/mozmill-dev/browse_thread/thread/c7170970800f6e79/9a4c8721310412ee?hide_quotes=no). Looking at the other available tools and the above mentioned requirements made me believe as well we should give Mozmill a try. Sure it is no catch-all, we may need compiled-code tests but having just to types of tests to maintain and develop is much, much easier than half a dozen types. That said, I am open for (even) better solutions, though.

Second, what do you propose we should do with this zip? Does Firefox have Makefile rules for mozmill we should use?

It does not have those yet. The current tests are in a spearate repo, http://hg.mozilla.org/qa/mozmill-tests but that is mainly due to not using Mozmill as a unit/whitebox testing tool (which it could be, though) as far as I understand it. Therefore, Mozmill itself is not included in the main tree (mozilla-central) but must be installed separately to run the tests.

If Makefiles are a poor plan, should we create a new testing repo for stuff like this?

I don't think Makefiles are a poor plan. It all depends on how the tests shall be used or better how the testing infrastructure should look like in the end, a thing that should probably discussed somewhere else with a wider audience than this bug as it is off topic and more important than the particular tests I attached. That said, if we want to start with some sort of blackbox/greybox tests like the ones I inlcuded, an own repo and some tweaks to Torbutton (the modal dialog on start-up) should be enough to get the tests integrated smoothly into the Torbutton/Torbrowser development workflow, I guess.

comment:13 in reply to:  12 Changed 6 years ago by gk

Replying to gk:

Replying to mikeperry:

gk: this is awesome. I am wondering two things: First, what made you pick mozmill over mochitest or xpcshell? Or for that matter, anything else out of https://developer.mozilla.org/en/Mozilla_automated_testing?

Well, the overall goal I had in mind was to have just one testing framework for everything reachable by JavaScript.

That needs discussion as well, of course, but as I said, not here.

comment:14 Changed 6 years ago by mikeperry

Keywords: MikePerry201210 added; MikePerry201208 removed

I think we'll want to revisit this sometime around when we start working on #5856.

comment:15 Changed 6 years ago by gk

Okay, I added a new zip file containing more tests, a fixed old test and a first README which explains some things but is far from being complete yet.

Changed 6 years ago by gk

Attachment: hookingTests2.zip added

Changed 6 years ago by gk

Attachment: windowScreenTests.zip added

comment:17 Changed 6 years ago by mikeperry

Keywords: MikePerry201210 removed

The question remains as to how to actually use these in production. It probably requires a dedicated person to create some kind of automation for them. Hopefully we'll figure out what to do on that front soon.

comment:18 in reply to:  17 Changed 6 years ago by gk

Replying to mikeperry:

The question remains as to how to actually use these in production. It probably requires a dedicated person to create some kind of automation for them.

Well, I don't think there is much effort needed here. The only things that are required are IMO installing Mozmill on the build machines (which is a one-time investment) and then a new target in the Makefile that builds the browser which runs Mozmill with the browser executable, the profile directory and the test directory as arguments. But on the other hand I don't know how your build infrastructure is working. So, maybe there is really more to it I am currently not aware of. And, yes, integrating these tests into Mozilla's existing testing frameworks or adding Mozmill to the ones already delivered with Firefox could indeed be time-consuming (if that road is preferred).

comment:19 Changed 3 years ago by saint

Cc: saint added
Severity: Normal

comment:20 Changed 2 years ago by bugzilla

Milestone: TorBrowserBundle 2.3.x-stable
Owner: changed from cypherpunks to boklm
Status: newassigned

comment:21 Changed 2 years ago by gk

Resolution: wontfix
Status: assignedclosed

This is an old and cluttered bug, we don't have JS hooks anymore and we don't use Mozmill anymore. Time for closing this I guess.

Note: See TracTickets for help on using tickets.