864664 - test suite "displaying the container after a timeout" sometimes failed

Status: RESOLVED FIXED

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

Description

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

setup of test suite "displaying the container after a timeout" sometime failed on assert.isFalse(css.contains('displayed')), here is a example:

https://travis-ci.org/mozilla-b2g/gaia/builds/6555901

> 1) [system] system/Updatanager UI container visibility displaying the container after a timeout "before each" hook:
>   Error: expected true to be false
>   at chaiAssert (http://test-agent.gaiamobile.org:8080/common/test/helper.js:30)
>   at get (http://test-agent.gaiamobile.org:8080/common/vendor/chai/chai.js:397)
>   at isFalse (http://test-agent.gaiamobile.org:8080/common/vendor/chai/chai.js:1385)
>   at (anonymous) (http://system.gaiamobile.org:8080/test/unit/update_manager_test.js:626)
>   at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:62)
>   at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709)
>   at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3973)
>   at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3984)
>   at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4932)

Comment 2

[Julien Wajsberg [:julienw]]

if this still appears again, I think we'll have to find a way to make it more reliable without using timeouts.

Comment 3

[Julien Wajsberg [:julienw]]

got another error :

 1) [system] system/Updatanager UI error banner requests "before all" hook:

Error: TypeError: self.toaster is null (http://system.gaiamobile.org:8080/js/update_manager.js?time=1366808290242:402)

at onerror (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4959)


I'll check if the fix to Bug 864864 should have fixed that, and if yes, I'll propose another way of testing this. Or disable the test, which would make me sad.

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Use MutationObserver to control when all the expected DOM changes are done.
---
 apps/system/test/unit/update_manager_test.js |   73 +++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 7 deletions(-)

PR is https://github.com/mozilla-b2g/gaia/pull/9562, let's see what travis says about it.

Comment 5

[Julien Wajsberg [:julienw]]

Travis seems to be happy with this.

Comment 6

[Etienne Segonzac (:etienne)]

Comment on attachment 745640 [details] [diff] [review]
patch v1

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

This is really cool, but it took me *a while* to understand.

Here are some small changes that IMHO could help *a lot* with the comprehension:

* |nbExpectedMutations| can be misunderstood, since it's not a strict expectation in the testing sense (we can happily go over it!), maybe we can rename it to |mutationsThreshold|.
We could alternatively make it behave like an expectation but I'm not in favor of this solution since it's not part of the tests but of the setup/teardown.

* we never inspect |pastRecords|, let's just keep a |mutationsCount| and simply |mutationsCount += mutations.length| in the observer (yay, no more |.push.apply|).
We also need to put it back to 0 in the teardown instead of emptying |pastRecords|.

* which leads us the |checkAllMutations| which isn't actually checking |pastRecords|, |waitForMutationsThreshold| sounds a lot better :)
(and BTW we probably want to group it with the rest of the utils functions at the beginning of the suite)

* and finally, |onMutationWrapper| could be renamed to |onMutations|, and always do |mutationsCount += mutations.length| at first (this would be the only place where we do this) and then, if it exists, call a |checkThreshold| callback.
|waitForMutationsThreshold| would define a |checkThreshold| checking if the threshold is met and calling |done()| (if the threshold isn't already met when we call |waitForMutationsThreshold|).

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: patch v2

addressed all follow ups

the PR is still in https://github.com/mozilla-b2g/gaia/pull/9562, hopefully travis will like it :)

Thanks, the patch looks a lot cleaner now !

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 746389 [details] [diff] [review]
patch v2

\o/

Comment 9

[Julien Wajsberg [:julienw]]

master: d9041ae743d3133842ce8d163883609de90653f6

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: merge backend for send and receive VideoConduits to match AudioConduits & cleanup

Alternate implementation that mirrors the kludge used for the AudioConduit backend to share the mChannel between send and receive conduits, and transfer RTP/RTCP sends on the 'wrong' conduit to the 'right' one.  Also cleans up a bunch of stuff in AudioConduit.  This is expected to be removed when Google changes the ViE channel implementation to be unidirectional, in conjunction with BUNDLE changes in the Pipeline code

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 817699 [details] [diff] [review]
merge backend for send and receive VideoConduits to match AudioConduits & cleanup

typo bug number