Intermittent ftu/test/unit/timezone_test.js | timezones > > user chose previously > previous selection prevails - Error: expected 'Madrid' to equal 'London'

RESOLVED FIXED in Firefox OS v2.1

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: RyanVM, Assigned: fcampo)

Tracking

({intermittent-failure})

unspecified
2.1 S8 (7Nov)
x86
Linux
intermittent-failure

Firefox Tracking Flags

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

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
kgrandon
: review+
alive
: review+
julienw
: feedback+
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
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)
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 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+
(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+
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+
Comment hidden (Treeherder Robot)
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.
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 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-
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 hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
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)
(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)
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)
You need to log in before you can comment on or make changes to this bug.