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)

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

Tracking

(b2g-v2.1 fixed, b2g-v2.2 fixed)

Status:
RESOLVED FIXED
Milestone:
2.1 S8 (7Nov)
Tracking Flags:
Tracking Status
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: RyanVM, Assigned: fcampo)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Pull Request
46 bytes, text/x-github-pull-request
kgrandon
: review+
alive
: review+
julienw
: feedback+
Details | Review
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
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)
Attached file Pull Request —
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 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 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 on attachment 8510355 [details] [review] Pull Request oops sorry for removing Samuel's r+
Attachment #8510355 - Flags: review+
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+
Thanks a lot, I appreciate it :)
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 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 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+
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.
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.
I'll have a look starting with your bigger patch, but if it's too mich work we can stick with the first.
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.
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 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-
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 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 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 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+
(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 :)
Master: https://github.com/mozilla-b2g/gaia/commit/2a90a30b628370bbdc54442556c2c5c02fd0d19f
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)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: