Intermittent ftu/test/unit/timezone_test.js | timezones > > user chose previously > previous selection prevails - Error: expected 'Madrid' to equal 'London'
RESOLVED
FIXED
in 2.1 S8 (7Nov)
Status
People
(Reporter: RyanVM, Assigned: fcampo)
Tracking
({intermittent-failure})
Firefox Tracking Flags
(b2g-v2.1 fixed, b2g-v2.2 fixed)
Details
Attachments
(1 attachment)
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 (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
|
||
Comment 18•4 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 (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
|
(Assignee) | |
Comment 22•4 years ago
|
||
Created attachment 8510355 [details] [review] 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 hidden (Treeherder Robot) |
|
||
Comment 24•4 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•4 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•4 years ago
|
||
Comment on attachment 8510355 [details] [review] Pull Request oops sorry for removing Samuel's r+
Attachment #8510355 -
Flags: review+
|
|
(Assignee) | |
Comment 27•4 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•4 years ago
|
||
Thanks a lot, I appreciate it :)
|
|
||
Comment 29•4 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•4 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•4 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 (Treeherder Robot) |
|
|
||
Comment 33•4 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•4 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•4 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•4 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•4 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 (Treeherder Robot) |
|
|
(Assignee) | |
Comment 39•4 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•4 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•4 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•4 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 (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
|
||
Comment 46•4 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 (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
|
|
||
Comment 49•4 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 (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
|
|
||
Updated•4 years ago
|
||
Keywords: checkin-needed
| Comment hidden (Treeherder Robot) |
|
(Reporter) | |
Comment 55•4 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/2a90a30b628370bbdc54442556c2c5c02fd0d19f
Assignee: nobody → fernando.campo
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.2: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
| Comment hidden (Treeherder Robot) |
| Comment hidden (Treeherder Robot) |
|
|
(Reporter) | |
Comment 58•4 years ago
|
||
v2.1: https://github.com/mozilla-b2g/gaia/commit/be8a015cda805c9238e23196f6e0f6fe68eeb6f3
status-b2g-v2.1: --- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•