Closed Bug 972699 Opened 6 years ago Closed 6 years ago

Fix unit test zero coverage when using lazy load

Categories

(Firefox OS Graveyard :: Gaia::TestAgent, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rickychien, Assigned: rickychien)

Details

Attachments

(3 files)

Certain modules use AMD loader (alameda.js) such as email and clock. It doesn't shows any coverage report except alameda.js.
This bug needs to send a patch to blanket.js for supporting lazy loading coverage.
And then update the blanket on Gaia.

Maybe this new change to blanket will not be merged immediately or need to take a time to experiment.
Hi James Burke,

I encountered some problem in this bug with using alameda.js.

I want to support lazy loading coverage detect on blanket.js and try to overwrite native function such as appendChild and XMLHttpRequest. Because I think overwrite these native calls can support AMD loader as well.
The important part of my change is that I rewrote appendChild and imitate original call to send load event by itself.

Until now, my effort work fine in most of cases (even pass unit tests in Gaia which include SMS, Setting, Search...). But some alameda based moudles are failed except Email. (and I didn't know why)

Took a lot of time to trace this problem and I doubt it maybe arise from load event triggering.
https://github.com/RickyChien/gaia/blob/bug-972699/test_apps/test-agent/common/vendor/blanket/blanket.js#L7312-L7323

I'm not familiar with how to manage these events order to make my code work correctly. So need your help:)
That will be great if you can tell me some details about how alameda.js manage the callback event.
Or maybe you can tell me this overwrite approach is impossible.
Flags: needinfo?(jrburke)
If you need to get notification of new script tags added to the page, I wonder if you use a mutation observer[1] to watch to changes in the HEAD element, and get notifications of the scripts that way?

[1] https://developer.mozilla.org/en/docs/Web/API/MutationObserver
Flags: needinfo?(jrburke)
Thanks your reply. :)

Unfortunately, the result was same but I have new findings in these test fails.
As I said before, this patch can work on email but not on other alameda used modules such as camera and clock. Fails only appeared when trying to load mock scripts through their customized require call.

(clock: https://github.com/RickyChien/gaia/blob/bug-972699/apps/clock/test/unit/test_require.js#L37-L91)
(email: https://github.com/RickyChien/gaia/blob/bug-972699/apps/email/test/config.js#L36-L104)

It's somewhat weird. Mocks can be loaded correctly on email but fail on clock. So I think problem maybe is the differences between their customized require call.

Do you know what is the differences between them?
Flags: needinfo?(jrburke)
Mechanics of js coverage is detecting files which need to cover by blanket. Then it will generates instrumentation version source and append to DOM. (Force to overwrite original context)

After overwritting, all of these pre-defined globals become instrumented that blanket can count coverage rate.

Maybe this problem is in require context right?
Duplicate of this bug: 978622
Let me describe more details about this problem.

I found a weird situation in https://github.com/RickyChien/gaia/blob/master/apps/clock/test/unit/alarm_test.js#L11-L24

If clock app use the mocks option that will cause test fail with "mockMozAlarms.MockMozAlarms is not a constructor" in alarm_test of clock, but once in a while success. Because the mockMozAlarms object initialized incorrectly, sometimes it's content become previous object (activeAlarm). When tracing in debug mode, the result appears correctly. So I guess the reason relate to delay time (Console debugger will slow down entire execution).

I have tested my patch in requirejs and alameda. It works fine with my simple use cases. Still couldn't figure out why it cause failure in clock and camera. Tracing into alameda is so hard for me. Need your suggestion for help if you can :)
I wonder if it is the version of alameda.js: I believe email is using a different version than clock for example. If you copy the alameda.js from email and put that one in the clock app, does that work? I would not expect it to make a difference, but I am just thinking about what is different.

Looking at the test setup files, nothing stands out as an issue, both seem to be doing fairly standard loader setup.

Also feel free to point me to how I can run what you are testing and any patch I should try, if the alameda version does not make a difference.
Flags: needinfo?(jrburke)
Attached file Gaia PR
Thanks. Unfortunately, copying new version alameda doesn't work.

Usage this patch is very easy.

1. Apply the patch https://github.com/mozilla-b2g/gaia/pull/16891.patch

2. Launch firefox-nightly by paht-to-nightly -profile path-to-gaia/profile-debug http://test-agent.gaiamobile.org:8080

3. You will see the test-agent web UI is launched.

4. Select a enableCoverage checkbox on top panel.

5. Click #clock (Or you can also choose another app) to select entire tests for app.

6. Press execute button to run tests.
Flags: needinfo?(jrburke)
Hey Ricky - just curious, should that patch also work for a deferred requireApp() call? I applied your patch, but I am still missing coverage for files in the search and other apps that are not using alameda.
The requireApp style of usage might also explain why email works vs clock or camera:

In email, every test has this sort of preamble:

requireApp('email/js/alameda.js');
requireApp('email/test/config.js');

Where in clock, I do not see that preamble, instead it relies on a setup.js that calls test_require.js, and test_require.js internally does:

requireApp('clock/js/alameda.js', function() {
  //sets up testRequire here
});

So I expect those different setup approaches cause the problem. However, I do not know the unit test internals or blanket to know how that messes up the blanket wiring.
Flags: needinfo?(jrburke)
Attached image without patch.png
@Kevin Grandon

I found new search app forgot to restore fakeXHR in marketplace_test so that it causes my patch fail.
I fixed it and take a screenshot between my path and without path.
Attached image with patch.png
Cool, thanks for the info! I made the fix locally as well, but I was still only seeing three files loaded =/  Maybe I'm doing something wrong though.
Yeah, I apply my patch again, it works for me.

git apply step:

1. git checkout -b tmp master

2. curl https://github.com/mozilla-b2g/gaia/pull/16891.patch | git am

3. remember to restore fakeXHR in search app

4. enable coverage and run tests :)
@James Burke, 

Try to modify clock app to make it same as email with:

requireApp('clock/js/alameda.js');
requireApp('clock/test/unit/test_require.js');

It doesn't work again.

The test-agent will preload setup.js before any tests running. Both require or requireApp calls in Gaia are sync should block of loading any tests. So I think this workaround is irrelevant to our problem.
But I appreciate your help :)
Still no luck for me here, but I'll wait till after this lands and we can investigate further. Is it possible that there could be anything OS specific? I'm using a mac currently.
So weird. I'm using mac too and also tested it on ubuntu.
Maybe you can rebase upstream b2g and try again.
Flags: needinfo?(jrburke)
Flags: needinfo?(jrburke)
I resolved this issue by using Object URL to load script: 

https://github.com/RickyChien/blanket/blob/support-lazyloading/src/blanketRequire.js#L472-L473

Through this workaround we can still load script by src and trigger load / error event properly without MutationObserver's help.

It's very miraculous!!

Still got a little problems with new workaround but I feel it's better than before.
Finally, I got back to early workaround which using data uri scheme as well as fixing encoding issue.
This scheme can pass almost all unit tests in gaia except a assert error in communication/dialer/test/unit/lazy_l10n_test.js.
The reason caused by the content of script.src has been replaced by data uri, so occur a fail in test. But I think it is irrelevant to the correctness of coverage report.
Attachment #8385947 - Flags: review?(yurenju.mozilla)
Comment on attachment 8385947 [details] [review]
Gaia PR

r=yurenju if follow up bug for using upstream version of blanket is filed.
Attachment #8385947 - Flags: review?(yurenju.mozilla) → review+
Merged.

https://github.com/mozilla-b2g/gaia/commit/6aa6a85fd3b2b9536fd96ee8f4136603600eb54e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.