Closed Bug 1044984 Opened 10 years ago Closed 9 years ago

Intermittent worker_test.js | Latin en_us worker Worker should throw if language doesnt exist | timeout of 10000ms exceeded

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Keyboard
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: cbook, Assigned: timdream)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(4 files)

Patch
46 bytes, text/x-github-pull-request
rudyl
: review+
Details | Review
Re-enable the test
52 bytes, patch
rudyl
: review+
Details | Diff | Splinter Review
[Gaia PR] disable worker_test.js in tbpl
46 bytes, text/x-github-pull-request
Details | Review
[gaia] timdream:keyboard-worker-test > mozilla-b2g:master
46 bytes, text/x-github-pull-request
julienw
: review+
Details | Review 
b2g_ubuntu64_vm b2g-inbound opt test gaia-unit on 2014-07-27 10:34:03 PDT for push 8b48f1036652

slave: tst-linux64-spot-563

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



gaia-unit-tests TEST-UNEXPECTED-FAIL | keyboard/test/unit/worker_test.js | Latin en_us worker Worker should throw if language doesnt exist | timeout of 10000ms exceeded
All of the failures from today were caused by bug 1038988, which has been reverted.
Hi Jan,

My patch in bug 1038988 turns this intermittent failure into a permanent failure. The weird thing is, that patch is a css-only change, and this test is for workers. Any ideas why this test would be effected by such a change? Also, would you mind if I disabled this test while we investigate, since bug 1038988 is a 2.0 CAF blocker and won't affect keyboard workers.

Note: this test runs fine for me locally with my patch both in b2g-desktop and firefox nightly.
Flags: needinfo?(janjongboom)
Assignee: nobody → janjongboom
Flags: needinfo?(janjongboom)
Attached file Patch — 
Ran tests twice, no failures anymore.

Did some things:

1. The test to see if the file exist is wrong, as we get a byte array back nowadays. It's just not valid dict format so it'll fail later. Status codes are now implemented correctly so rely on that.
2. Wait until dict is loaded before moving from suiteSetup, removing timing issue
3. Make the XHR call async in the worker.
Attachment #8465373 - Flags: review?(rlu)
In the meantime, I'm disabling this test because bug 1038988 is a CAF blocker that blocks a CAF blocker, and I want to land today to make sure it doesn't break anything else by deadline date of Aug 1 (tomorrow). You'll have to update your PR to remove the early return. Sorry about that guys.

master: https://github.com/mozilla-b2g/gaia/commit/031b8cc197c92219285bebee068b90ce85d38cde
v2.0: https://github.com/mozilla-b2g/gaia/commit/1d1a47a1135d8ce44db9b88db4d2ea6f454cf0a8
Comment on attachment 8465373 [details] [review]
Patch

Jan, thanks for the patch.

However, since we've also change the xhr from sync to async, I could see a regression that the suggestion won't be updated when you switch to a new language that is just loaded.
 STR
 ===
 1. Kill the keyboard and make sure all dict have been cleaned up.
 2. type some word to show the suggestion bar.
 3. Press [En] to switch to another language => the suggestion won't change.

So, I'll r- this first.
Attachment #8465373 - Flags: review?(rlu) → review-
Comment on attachment 8465373 [details] [review]
Patch

Oh, wow, didn't see that. Relying on sync XHR to keep code working looks like a bug to me though, but hey.

I made the code sync again, seems to work on device. Let's see what Try thinks. https://tbpl.mozilla.org/?rev=9d4212952c91b256aa34f6def111847191b0452b&tree=Gaia-Try
Attachment #8465373 - Flags: review- → review?(rlu)
(In reply to Michael Henretty [:mhenretty] from comment #20)
> In the meantime, I'm disabling this test because bug 1038988 is a CAF
> blocker that blocks a CAF blocker, and I want to land today to make sure it
> doesn't break anything else by deadline date of Aug 1 (tomorrow). You'll
> have to update your PR to remove the early return. Sorry about that guys.

Sure, but you owe me a beer when I'm in SF next month.
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #24)
> (In reply to Michael Henretty [:mhenretty] from comment #20)
> > In the meantime, I'm disabling this test because bug 1038988 is a CAF
> > blocker that blocks a CAF blocker, and I want to land today to make sure it
> > doesn't break anything else by deadline date of Aug 1 (tomorrow). You'll
> > have to update your PR to remove the early return. Sorry about that guys.
> 
> Sure, but you owe me a beer when I'm in SF next month.

Deal
Comment on attachment 8465373 [details] [review]
Patch

r=me.
Thanks.
Attachment #8465373 - Flags: review?(rlu) → review+
https://github.com/mozilla-b2g/gaia/commit/e103e787a28156088fc3e0d334b34607f8262674
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
We need to remove the return statement at the top as well...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8467739 [details] [diff] [review]
Re-enable the test

Oh right.
Thanks.
Attachment #8467739 - Flags: review?(rlu) → review+
Landed https://github.com/mozilla-b2g/gaia/commit/d2124ca2a6eff0d73954fb1f59cbf74275f28cb9
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Jan, I think this test is failing often enough that we should disable it until we re-investigate what is going on. What do you think?
Flags: needinfo?(janjongboom)
Julien, do we have a manifest for disabling unit tests in tbpl?
Flags: needinfo?(felash)
I think James helped me find the tbpl manifest. Submitted this PR to test it.
Yup, that did it. I disabled the test for now.

master: https://github.com/mozilla-b2g/gaia/commit/31cd87328f5fc76c60097d25354fe84bd6456542
Flags: needinfo?(felash)
Does it fail on Travis too?
Flags: needinfo?(janjongboom)
Keywords: leave-open
Kevin, this test should be disabled according to comment 61. How comes it is run by TC?
Flags: needinfo?(kgrandon)
Moreoever the error is not the same as this bug's summary, no timeout is involved here.
After some quick investigation it appears that this test *was* disabled, then the file was moved in bug 972210 without updating the disabled test location. After that it was then removed from the disabled.json file here, probably because the location was wrong and it wasn't failing too much: https://github.com/mozilla-b2g/gaia/commit/4218ac24394a498e3a132a5ba18fcc38bff63545

In any case, the intermittent rate doesn't seem terrible (once per day so far). I don't mind if we disable it again or wait to monitor the failing rate over the next few days as we're still rolling out taskcluster updates.
Flags: needinfo?(kgrandon)
latest is really bug 1165469 -- but the others are not.
Failure appears to have morphed, but still pretty regular.
Flags: needinfo?(kgrandon)
This has an assignee already, so asking Tim or Jan to look into it (otherwise we could consider disabling the test).
Flags: needinfo?(timdream)
Flags: needinfo?(kgrandon)
Flags: needinfo?(janjongboom)
I tried to analyze the code of this test, and I don't understand several things.

1. Why is it so complicated to get messages from the worker?
=> https://github.com/mozilla-b2g/gaia/blob/a47e0c58dd1340a17be7cc96ccd90fcad15a922b/apps/keyboard/test/unit/imes/latin/worker_test.js#L454-L469

2. The test says "should throw if language doesn't exist" but the truth is that it really throws because we don't pass enough arguments and "buffer" is undefined here: 
=> https://github.com/mozilla-b2g/gaia/blob/a47e0c58dd1340a17be7cc96ccd90fcad15a922b/apps/keyboard/js/imes/latin/predictions.js#L109


Hey Tim, Jan, do you know why 1 has been done this way, and don't you think we should just remove the test? Actually I don't see any code in worker.js that checks whether the language is known. Maybe it's been done somewhere else ?
We should disable that again until there is someone available to work on the dictionary again. I will make sure that happens before we do feature work on it.
Flags: needinfo?(timdream)
Keeping this on my backlog too ... look like the tests was not updated when I remove XHR from worker.js, according to comment 207.
Flags: needinfo?(timdream)
Comment on attachment 8637111 [details] [review]
[gaia] timdream:keyboard-worker-test > mozilla-b2g:master

Julien, assuming you have read the file already and since there is no other keyboard developer left, would you do the honor and review this?

I realized we reuse the worker instance in each of the test, probably an attempt to speed things up, so I changed it -- should get rid of the instability. Also for the test you talked about, I reworded it so we know it throw because the dictionary is not set.
Flags: needinfo?(timdream)
Attachment #8637111 - Flags: review?(felash)
Assignee: janjongboom → timdream
Status: REOPENED → ASSIGNED
Comment on attachment 8637111 [details] [review]
[gaia] timdream:keyboard-worker-test > mozilla-b2g:master

r=me with nits

Most nits are optional (we can do them later) but I'd like to insist on the comment to remove the listener in onWorkerMessage: https://github.com/mozilla-b2g/gaia/pull/31070/files#r35429136

I retriggered the test several times in treeherder and it didn't fail once. So I think it's safe to assume the intermittency is fixed, and that's what is important here. All the other nits can be done later :)
Attachment #8637111 - Flags: review?(felash) → review+
Flags: needinfo?(janjongboom)
I added a simple |worker.onmessage = null| for addressing your review comment. I agree with you switching to Promise is better for async tests, but not today.

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

Attachment

General

Creator:
Created:
Updated:
Size: