Closed Bug 908288 Opened 8 years ago Closed 8 years ago

gaia-unit-tests - "WindowManager is null" intermittent failures when running the whole system app unit test suite

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: etienne, Assigned: etienne)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #907669 +++

This is currently one of the bugs blocking Gaia unit tests from being unhidden on TBPL.

https://tbpl.mozilla.org/php/getParsedLog.php?id=26813346&tree=B2g-Inbound

b2g_ubuntu64_vm b2g-inbound opt test gaia-unit on 2013-08-21 04:21:07 PDT for push 37a1fbdaa344
slave: tst-linux64-ec2-380

04:25:50     INFO -  gaia-unit-tests TEST-START | toggleScreen()
04:25:50     INFO -  gaia-unit-tests TEST-UNEXPECTED-FAIL | notifications system/ScreenManager toggleScreen() if screenEnabled is true | expected false to be true
04:25:50     INFO -  gaia-unit-tests INFO       | stack trace:
04:25:50     INFO -      Error: expected false to be true
04:25:50     INFO -          at chaiAssert (http://test-agent.gaiamobile.org:8080/common/test/helper.js:30)
04:25:50     INFO -          at get (http://test-agent.gaiamobile.org:8080/common/vendor/chai/chai.js:374)
04:25:50     INFO -          at isTrue (http://test-agent.gaiamobile.org:8080/common/vendor/chai/chai.js:1367)
04:25:50     INFO -          at (anonymous) (http://system.gaiamobile.org:8080/test/unit/screen_manager_test.js:712)
04:25:50     INFO -          at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:62)
04:25:50     INFO -          at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709)
04:25:50     INFO -          at runTest (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4081)
04:25:50     INFO -          at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4127)
04:25:50     INFO -          at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4007)
04:25:50     INFO -          at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4016)
04:25:50     INFO -          at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3964)
04:25:50     INFO -          at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3979)
04:25:50     INFO -          at done (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3700)
04:25:50     INFO -          at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3712)
04:25:50     INFO -          at (anonymous) (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:46)
04:25:50     INFO -          at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:73)
04:25:50     INFO -          at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709)
04:25:50     INFO -          at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3973)
04:25:50     INFO -          at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3984)
04:25:50     INFO -          at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4932)
04:25:50     INFO -  gaia-unit-tests TEST-PASS  | notifications system/ScreenManager toggleScreen() if screenEnabled is false
04:25:50     INFO -  gaia-unit-tests TEST-END   | toggleScreen()
Summary: gaia-unit-tests TEST-UNEXPECTED-FAIL | notifications system/ScreenManager toggleScreen() if screenEnabled is true | expected false to be true → gaia-unit-tests - "WindowManager is null" intermittent failures when running the whole system app unit test suite
The investigations led to the ScreenManager which has a bunch of setTimeouts causing rangom later tests to fail.
Attached patch Patch proposal (obsolete) — Splinter Review
Finally got it to work.

What this patch does is:
* switching to `this.sinon` instead of `sinon` to avoid missing `.restore()` related errors
* loading the `screen_manager.js` file *only* when we're sure that the fake timers are in place to avoid setimeouts running in the wild
Assignee: nobody → etienne
Attachment #794137 - Flags: review?(felash)
Comment on attachment 794137 [details] [diff] [review]
Patch proposal

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

I made some generic comments about stubbing internal methods. Maybe the comments are not correct in the context of this test as I don't know the tested file, so you can disregard those comments if you think they're irrelevant. Anyway they're not blockers because they could need to refactor the whole unit test file and I understand this is not the scope of this bug.

other comments about the proper use of sinon are important to have a readable test though :)

thanks !

::: apps/system/test/unit/mock_navigator_moz_power.js
@@ +6,5 @@
> +  screenBrightness: 0,
> +  screenEnabled: false,
> +
> +  mTeardown: function teardown() {
> +    this.addWakeLockListener = function() {};

you don't need to do this if you use sinon properly

::: apps/system/test/unit/screen_manager_test.js
@@ +39,5 @@
> +  setup(function(done) {
> +    switchProperty(navigator, 'mozPower', MockMozPower, reals, true);
> +    switchProperty(window, 'WindowManager', MockWindowManager, reals);
> +    switchProperty(window, 'SettingsListener', MockSettingsListener, reals);
> +    fakeTimers = this.sinon.useFakeTimers();

nit: you don't need a local variable.

Just use "this.sinon.clock" instead where you need it, I find this more readable.

@@ +49,5 @@
> +  teardown(function() {
> +    MockMozPower.mTeardown();
> +    MockWindowManager.mTeardown();
> +    MockSettingsListener.mTeardown();
> +    fakeTimers.restore();

you don't need this restore

@@ +65,4 @@
>            .returns(document.createElement('div'));
>  
> +      MockMozPower.addWakeLockListener = this.sinon.stub().callsArgWith(0,
> +                                                 'screen', 'locked-foreground');

just use

  this.sinon.stub(MockMozPower, "addWakeLockListener").yields('screen', 'locked-foreground');

note that I usually greatly prefer to use a spy and use its yield() function to call the argument whenever I want, so that it's asynchronous-like but still synchronous since I control when the callback is called. That way it's also closer to what happens for real.

Moreover, I see that you redefine this stub with different values... therefore I think it would _really_ be both more readable and more maintenable to use the spy/yield solution. Just put the spy once in a top-level setup, and use yield in the tests.

@@ +71,1 @@
>        stubLockscreen.locked = true;

maybe just use the existing mock for LockScreen and augment it instead ?

@@ +77,1 @@
>        switchProperty(navigator, 'mozTelephony', stubTelephony, reals);

I get it that you don't want to use real mocks for this ?

especially we have already one in the system unit tests...

@@ +96,1 @@
>            .callsArgWith(0, 'screen', 'locked-background');

you could directly use the spy's "yield" method after calling "init" here.

@@ +107,1 @@
>            .callsArgWith(0, 'cpu', 'another-state');

same here

@@ +112,1 @@
>            .callsArgWith(0, 'cpu', 'locked-background');

same here... and probably you wouldn't even need to call init again, just yield the new value... it's more like the real stuff.

@@ +149,5 @@
>      suite('Testing devicelight event', function() {
>        var stubAutoAdjust;
>  
>        setup(function() {
> +        stubAutoAdjust = this.sinon.stub(ScreenManager, 'autoAdjustBrightness');

I'm never very comfortable with stubbing and even spying internal methods of the tested class. Could we test their behavior towards the external world instead ?

This could be done in a follow-up bug though.

@@ +183,5 @@
>        });
>      });
>  
>      test('Testing sleep event', function() {
> +      var stubTurnOff = this.sinon.stub(ScreenManager, 'turnScreenOff');

same comment

@@ +190,4 @@
>      });
>  
>      test('Testing wake event', function() {
> +      var stubTurnOn = this.sinon.stub(ScreenManager, 'turnScreenOn');

same comment

@@ +207,4 @@
>  
>          switchProperty(window, 'Bluetooth', stubBluetooth, reals);
>          switchProperty(window, 'StatusBar', stubStatusBar, reals);
>          switchProperty(navigator, 'mozTelephony', stubTelephony, reals);

could you use the existing mocks insstead ? and maybe create a simple one for Bluetooth ?

@@ +217,5 @@
>        });
>  
>        test('if Bluetooth SCO connected', function() {
>          stubBluetooth.Profiles = {};
> +        stubBluetooth.isProfileConnected = this.sinon.stub().returns(true);

better use

  this.sinon.stub(stubBluetooth, 'isProfileConnected').returns(true)

you can define the stub in the setup, and just do the "returns(true)" in the test, so that you can do the same in the following test.

@@ +260,4 @@
>          ScreenManager._cpuWakeLock = stubCpuWakeLock;
>  
>          switchProperty(navigator, 'mozTelephony', stubTelephony, reals);
>          switchProperty(window, 'AttentionScreen', stubAttentionScreen, reals);

can't you use the existing mock for mozTelephony and create a minimalist one for AttentionScreen ?

and then use "this.sinon.spy(object, 'method')" if you need to spy the calls

@@ +261,5 @@
>  
>          switchProperty(navigator, 'mozTelephony', stubTelephony, reals);
>          switchProperty(window, 'AttentionScreen', stubAttentionScreen, reals);
>          switchProperty(window, 'removeEventListener',
>              stubRemoveListener, reals);

you can just use :

  this.sinon.stub(window, 'removeEventListener');

@@ +294,1 @@
>          stubTelephony.calls = [{'addEventListener': stubAddListener}];

I'd find this very nice to have a "real" MockCall instead of this.

@@ +317,1 @@
>          switchProperty(window, 'addEventListener', stubAddListener, reals);

same remark as before: you can directly stub the window property using sinon.

@@ +363,4 @@
>        stubScreen = {'classList': {'add': stubScnClassListAdd}};
> +      stubFireEvent = this.sinon.stub(ScreenManager, 'fireScreenChangeEvent');
> +      stubUnlock = this.sinon.stub();
> +      stubSetBrightness = this.sinon.stub(ScreenManager, 'setScreenBrightness');

same comment about stubbing internal methods

@@ +431,1 @@
>        switchProperty(window, 'addEventListener', stubAddListener, reals);

just stub directly the window's property instead

@@ +489,5 @@
>  
>      setup(function() {
> +      stubClearTimeout = this.sinon.stub();
> +      stubTransBrightness = this.sinon.stub(ScreenManager,
> +                                            'transitionBrightness');

same comment about stubbing internal methods. Is this really necessary ? (especially that because you're using the fake timers, it should not have any side effects then)

@@ +494,1 @@
>        switchProperty(window, 'clearTimeout', stubClearTimeout, reals);

stub the window's property directly

@@ +547,1 @@
>        switchProperty(window, 'removeEventListener', stubRemoveListener, reals);

directly stub window's properties

@@ +551,2 @@
>        restoreProperty(window, 'addEventListener', reals);
>        restoreProperty(window, 'removeEventListener', reals);

and so you can remove this

@@ +591,5 @@
>      });
>  
>      teardown(function() {
>        restoreProperty(window, 'clearIdleTimeout', reals);
>        restoreProperty(window, 'setIdleTimeout', reals);

same comments

@@ +614,5 @@
>  
>      ScreenManager.fireScreenChangeEvent();
>      assert.isTrue(stubDispatchEvent.called);
>  
>      restoreProperty(window, 'dispatchEvent', reals);

same comment
Attachment #794137 - Flags: review?(felash)
Attached patch Patch v2 (obsolete) — Splinter Review
Addressed the non-invasive part of the comments.

As discussed, opening a follow-up bug about cleaning the screen_manager_test.js
Attachment #794137 - Attachment is obsolete: true
Attachment #794698 - Flags: review?(felash)
(In reply to Etienne Segonzac (:etienne) from comment #4)
> As discussed, opening a follow-up bug about cleaning the
> screen_manager_test.js

bug 908682
Learned something from this improving test work ;)
C.C. more people working in system app's test.
Comment on attachment 794698 [details] [diff] [review]
Patch v2

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

this is really in a good shape, but I don't understand why you didn't stub directly window's method ? Was this a problem ?

::: apps/system/test/unit/screen_manager_test.js
@@ +63,1 @@
>            .returns(document.createElement('div'));

nit: can we do better with the indentation here ? ;) 2 or 4 is fine for me, but please align both on the same column.

Or probably more readable:

  var stubById = this.sinon.stub(document, 'getElementById');
  stubById.withArgs('screen').returns(document.createElement('div'));

@@ +263,5 @@
>  
>          switchProperty(navigator, 'mozTelephony', stubTelephony, reals);
>          switchProperty(window, 'AttentionScreen', stubAttentionScreen, reals);
>          switchProperty(window, 'removeEventListener',
>              stubRemoveListener, reals);

Isn't it possible to simply use the following ?

  stubRemoveListener = this.sinon.stub(window, 'removeEventListener');

and then you don't need to restore this later either.

@@ +319,1 @@
>          switchProperty(window, 'addEventListener', stubAddListener, reals);

same comment here: why don't you stub window's method directly ?

@@ +369,2 @@
>  
>        switchProperty(window, 'removeEventListener', stubRemoveListener, reals);

same comment here

@@ +433,1 @@
>        switchProperty(window, 'addEventListener', stubAddListener, reals);

same comment here: why don't you stub window's method directly ?

@@ +441,1 @@
>        restoreProperty(navigator, 'mozTelephony', reals);

you already do this in the top level teardown

@@ +496,1 @@
>        switchProperty(window, 'clearTimeout', stubClearTimeout, reals);

same comment here: why don't you directly stub the window property ?

@@ +549,1 @@
>        switchProperty(window, 'removeEventListener', stubRemoveListener, reals);

same here

@@ +592,1 @@
>        switchProperty(window, 'setIdleTimeout', stubSetIdleTimeout, reals);

same here
(btw: these functions are really in window ? are they standard ?)

@@ +615,1 @@
>      switchProperty(window, 'dispatchEvent', stubDispatchEvent, reals);

same here
Attachment #794698 - Flags: review?(felash)
Blocks: 908682
Attached patch Patch v3Splinter Review
Crossing fingers ;)
Attachment #794698 - Attachment is obsolete: true
Attachment #797163 - Flags: review?(felash)
Comment on attachment 797163 [details] [diff] [review]
Patch v3

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

r=me

Travis fails in another app => unrelated.

Let's land this, thanks for this work !
Attachment #797163 - Flags: review?(felash) → review+
https://github.com/mozilla-b2g/gaia/commit/4c7deaf16f859783b494fcc4f119474c2c018953
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.