Closed
Bug 1077258
Opened 11 years ago
Closed 11 years ago
Intermittent ftu/test/unit/timezone_test.js | timezones > > user chose previously > previous selection prevails - Error: expected 'Madrid' to equal 'London'
Categories
(Firefox OS Graveyard :: Gaia::First Time Experience, defect)
Tracking
(b2g-v2.1 fixed, b2g-v2.2 fixed)
RESOLVED
FIXED
2.1 S8 (7Nov)
People
(Reporter: RyanVM, Assigned: fcampo)
Details
(Keywords: intermittent-failure)
Attachments
(1 file)
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=290087&repo=mozilla-aurora
builder b2g_ubuntu64_vm mozilla-aurora opt test gaia-unit
buildid 20141002173321
builduid 594d988886554c64a5d0d950c9765136
results failure (2)
revision 4ddcd02bb5c3
slave tst-linux64-spot-175
starttime Thu Oct 02 2014 22:24:26 GMT-0400 (Eastern Standard Time)
19:46:40 INFO - TEST-START | ftu/test/unit/timezone_test.js
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > observing changes on time.timezone
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > no previous user interaction, use DEFAULT timezone
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > very first run, no user interaction > no SIM card we use DEFAULT value
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > very first run, no user interaction > SIM ready, connection avaiable get timezone from the connection
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > very first run, no user interaction > SIM ready, connection avaiable if unknown MCC/MNC, use default
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > very first run, no user interaction > SIM ready, no connection get timezone from SIM
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > very first run, no user interaction > SIM ready, no connection if unknown MCC/MNC, use default
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > very first run, no user interaction > protected SIM card > default is used while waiting for access
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > very first run, no user interaction > protected SIM card > reload when granted access to SIM info
19:46:41 INFO - TEST-PASS | ftu/test/unit/timezone_test.js | timezones > > user chose previously > previous selection prevails
19:46:41 INFO - JavaScript error: app://ftu.gaiamobile.org/common/test/helper.js?time=1412304400379, line 31: expected 'Madrid' to equal 'London'
19:46:41 INFO - TEST-UNEXPECTED-FAIL | ftu/test/unit/timezone_test.js | timezones > > user chose previously > previous selection prevails - Error: expected 'Madrid' to equal 'London' (app://ftu.gaiamobile.org/common/test/helper.js?time=1412304400379:31)
19:46:41 INFO - Error: Error: expected 'Madrid' to equal 'London' (app://ftu.gaiamobile.org/common/test/helper.js?time=1412304400379:31)
19:46:41 INFO - at onerror (app://ftu.gaiamobile.org/common/vendor/mocha/mocha.js:4959:10)
19:46:41 INFO - TEST-OK | ftu/test/unit/timezone_test.js | took 850ms
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 18•11 years ago
|
||
I think the issue is actually in the previous test "reload when granted access to SIM info". Likely we enter the tzLoaded callback again after "done" has been called, after the following test started.
Fernando, maybe you can have a look?
Flags: needinfo?(fernando.campo)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 22•11 years ago
|
||
Made some changes on setup/teardown, seems to work on local but waiting to TBPL results
Flags: needinfo?(fernando.campo)
Attachment #8510355 -
Flags: review?(sfoster)
Attachment #8510355 -
Flags: feedback?(felash)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 24•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
thanks for looking at this. That's a nice wall of green Gu after this patch. I agree Julien's comments about the setup vs. suiteSetup. I was going to put in a pull request with that change in the shared mock and see how it played out. But I don't think that should block landing this, we can follow up as necessary.
Attachment #8510355 -
Flags: review?(sfoster) → review+
Comment 25•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
This seems to fix it, but I think it's better to change the MockIccHelper directly, because this can fix other tests as well, and it's cleaner.
Once you change the mock, please ask review from kgrandon because it's in /shared.
Thanks a lot Fernando!
Attachment #8510355 -
Flags: review+
Attachment #8510355 -
Flags: feedback?(felash)
Attachment #8510355 -
Flags: feedback-
Comment 26•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
oops sorry for removing Samuel's r+
Attachment #8510355 -
Flags: review+
Assignee | ||
Comment 27•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
(In reply to Julien Wajsberg [:julienw] from comment #25)
> Comment on attachment 8510355 [details] [review]
> Pull Request
>
> This seems to fix it, but I think it's better to change the MockIccHelper
> directly, because this can fix other tests as well, and it's cleaner.
>
> Once you change the mock, please ask review from kgrandon because it's in
> /shared.
>
> Thanks a lot Fernando!
I tried to change the Mock and had a few other tests failing (/ftu/navigation.js). It took me a while to figure it out, but it seems to be working now. Updated PR, so asking for review again (sorry for the noise)
Attachment #8510355 -
Flags: review?(sfoster)
Attachment #8510355 -
Flags: review?(kgrandon)
Attachment #8510355 -
Flags: review+
Comment 28•11 years ago
|
||
Thanks a lot, I appreciate it :)
Comment 29•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
I like it if tree-herder does. Lets re-trigger Gu a bunch of times again to be sure.
Attachment #8510355 -
Flags: review?(sfoster) → review+
Comment 30•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
Unfortunately I see a lot of test failures, and I think they may be related to this fix, so I'm unable to leave an R+ here. Please take a look, and flag me again if needed.
Attachment #8510355 -
Flags: review?(kgrandon) → review-
Comment 31•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
yeah, what kgrandon said. Those Gu failures do look like the result of this shared mock change. So we can either 1) dig into and fix those, or 2) go back to the slightly-unintuitive-but-not-failing patch we had earlier. For my part I'm fine with either if we end up with one less intermittent - and this one has been a noisy one.
Attachment #8510355 -
Flags: review+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 33•11 years ago
|
||
1 less intermittent is always good, but someone will have to do this work at one point though.
It's all about priority :) If you have time to do this now, I'd prefer to do it now. Otherwise let's move forward with the first patch.
Assignee | ||
Comment 34•11 years ago
|
||
I've been trying to fix the /system/ test failures during the weekend, but it was harder than expected. Problem is that "for asyncStorage scoping reasons, we can't simply use 'attachTestHelpers' anywhere in the internet sharing tests." (comment on the suite), so I'm finding a lot of trouble when fixing them (also, not familiar at all with system tests). As it is not a priority on my queue, I'm not giving it a lot of time either, so up to you guys if you want to land the smaller (and working) patch with a followup, or wait for this bigger (and supposedly more correct) second patch.
Comment 35•11 years ago
|
||
I'll have a look starting with your bigger patch, but if it's too mich work we can stick with the first.
Comment 36•11 years ago
|
||
I did a quick and dirty fix in https://github.com/mozilla-b2g/gaia/pull/25539
Let's see what try is saying and if it's green we can ask a review from a system peer.
Comment 37•11 years ago
|
||
Hey Fernando, looks like my fix is working, can I let you pick it into your patch and ask a review from a System peer? Looks like :alive did most reviews on this file so he should be a good reviewer.
Flags: needinfo?(fernando.campo)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 39•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
PR updated with Julien's commit (thanks!). I was going crazy logging states before and after setups.
Although, we ended up changing a call to mock.setup inside of a suiteSetup for a call to mock.setup AND mock.suiteSetup inside of another suiteSetup. I feel dirty.
My opinion is that internet_sharing is quite tricky because of asyncStorage and wrong mocks, and probably needs a re-do with proper setups, but I say this from total ignorance about system tests, so you can kick me hard if I deserve it.
Flags: needinfo?(fernando.campo)
Attachment #8510355 -
Flags: review?(kgrandon)
Attachment #8510355 -
Flags: review?(alive)
Attachment #8510355 -
Flags: review-
Attachment #8510355 -
Flags: feedback?(felash)
Attachment #8510355 -
Flags: feedback-
Comment 40•11 years ago
|
||
The main issue is that internet_sharing suite shares states between tests, and that's why it's happening.
Each test run should be independant. If we want to share states, then it should just be in one test, no in several tests. That's the main issue in this test suite IMO.
Comment 41•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
I'm fine trying this and seeing if it fixes the intermittent test, but maybe the whole suite needs a refactoring as Julien suggested. For some reason it seems gaia-try didn't pick this up, can you try rebasing against master and force pushing to re-trigger gaia-try? Thanks!
Attachment #8510355 -
Flags: review?(kgrandon) → review+
Comment 42•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
feedback+ given you can make a try run with at least 50 Gu runs and they're all green (or not the same intermittent :p)
Attachment #8510355 -
Flags: feedback?(felash) → feedback+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 46•11 years ago
|
||
Comment on attachment 8510355 [details] [review]
Pull Request
So .attachTestHelpers() does not work for internet_sharing_test? Hmmm...not sure if we could/should track what's the root cause in follow up.
Attachment #8510355 -
Flags: review?(alive) → review+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 49•11 years ago
|
||
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #46)
> Comment on attachment 8510355 [details] [review]
> Pull Request
>
> So .attachTestHelpers() does not work for internet_sharing_test? Hmmm...not
> sure if we could/should track what's the root cause in follow up.
The issue is that attachTestHelpers is just doing "setup(() => mocksHelper.setup())", which calls all mocks' mSetup functions on setup. This is what breaks here: the tests in internet_sharing_test rely on keeping a state between tests.
The fix is what I said in comment 40. I agree we should track this, I filed bug 1090786 :)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•11 years ago
|
Keywords: checkin-needed
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Comment 55•11 years ago
|
||
Assignee: nobody → fernando.campo
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g-v2.2:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Comment 58•11 years ago
|
||
status-b2g-v2.1:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•