936369 - [system] Intermittent Travis Unit Test Failure: KeyboardManager Transitions Call showKeyboard against visible keyboard: Error: timeout of 2000ms exceeded

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

See also bug 935904 and bug 936011.

[system] KeyboardManager Transitions Call showKeyboard against visible keyboard:

Error: timeout of 2000ms exceeded

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

Comment 1

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

ping Jan and Gary.
Could one of you help look into this?

Thanks.

Comment 2

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Attachment: pull request: https://github.com/mozilla-b2g/gaia/pull/14436

Comment 3

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Comment on attachment 8343515 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/14436

got wrong way to resolve this issue, remove review flasg first.

Comment 4

[Julien Wajsberg [:julienw]]

I've seen this intermittent again today and I really want to get rid of it.

There are lots of tests where we wait for a specific transition or things like this. I really think this is useless:

* the transition CSS is injected in the test, therefore, in the test, you're really testing your test data instead of real code
* this makes the test extremely fragile

No, what you need to test is what your JS code is doing: that it correctly changes a class, things like that.

As a rule, you should avoid as much as possible to rely on asynchronous things to happen, as this will break from time to time on Travis and TBPL.

Gary, I can try to patch the unit tests if you're ok with these rules?

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

Well, in this case it tests the underlying showing/hiding logic, by piggy-backing on the fact that a transition is made if the keyboard gets shown. Which is important because the underlying code also relies on the animation finishing succesfully to trigger future events.

Comment 8

[Julien Wajsberg [:julienw]]

If you want to test whether the underlying code works, you can send a "transitionend" in the test. There is no need to use a real transition.

Again, waiting for a transition event from a CSS you inject is just testing nothing at all.

Comment 9

[Julien Wajsberg [:julienw]]

I'm doing it right now, you'll take it or not ;)

Comment 10

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

It's fine, 
Sorry, I am busy on fugu issue.

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: Github pull request

Rewritten most of the unit test in a reliable, synchronous way. It's way faster now, and it won't show any intermittent failure.

Testing the CSS needs integration tests.

Waiting a Travis run to request review.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8348711 [details] [review]
Github pull request

Travis has a green test-agent job!

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

Ok, that's a big patch. :p

Comment 14

[Julien Wajsberg [:julienw]]

Rebased on latest master, resolved conflicts, and added comments.

CAN I HAZ REVIEW PLIZ????

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8348711 [details] [review]
Github pull request

I find it hard to be very sure that we still test exactly the same, but it's tests and they're green. Thanks.

Comment 16

[Julien Wajsberg [:julienw]]

(In reply to Jan Jongboom [:janjongboom] from comment #15)
> Comment on attachment 8348711 [details] [review]
> Github pull request
> 
> I find it hard to be very sure that we still test exactly the same, but it's
> tests and they're green. Thanks.

To reassure you, I rewrote each test one at a time. I only merged 2 of them when it made sense (like doing the same action and then testing something different).

Comment 17

[Julien Wajsberg [:julienw]]

master: ecf64f1c4bbbe48b0570fabee708d514872cb50b

Comment 18

[Julien Wajsberg [:julienw]]

a=tests
v1.3: 73942ce4885fc8274654f832466002476a8aaf41

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Thanks Julien!

Green travis ftw