972699 - Fix unit test zero coverage when using lazy load

Status: RESOLVED FIXED

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

Description

[Ricky Chien [:rickychien] (inactive)]

Certain modules use AMD loader (alameda.js) such as email and clock. It doesn't shows any coverage report except alameda.js.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 2

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 3

[James Burke [: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

Comment 4

[Ricky Chien [:rickychien] (inactive)]

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?

Comment 5

[Ricky Chien [:rickychien] (inactive)]

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?

Comment 7

[Ricky Chien [:rickychien] (inactive)]

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 :)

Comment 8

[James Burke [:jrburke]]

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.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia PR

Comment 10

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 11

[Kevin Grandon :kgrandon]

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.

Comment 12

[James Burke [:jrburke]]

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.

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Attachment: 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.

Comment 14

[Ricky Chien [:rickychien] (inactive)]

Attachment: with patch.png

Comment 15

[Kevin Grandon :kgrandon]

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.

Comment 16

[Ricky Chien [:rickychien] (inactive)]

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 :)

Comment 17

[Ricky Chien [:rickychien] (inactive)]

@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 :)

Comment 18

[Kevin Grandon :kgrandon]

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.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

So weird. I'm using mac too and also tested it on ubuntu.
Maybe you can rebase upstream b2g and try again.

Comment 20

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 21

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 22

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8385947 [details] [review]
Gaia PR

r=yurenju if follow up bug for using upstream version of blanket is filed.

Comment 23

[Ricky Chien [:rickychien] (inactive)]

Merged.

https://github.com/mozilla-b2g/gaia/commit/6aa6a85fd3b2b9536fd96ee8f4136603600eb54e