On master (2.1), timezone is not set correctly in automatic mode when user go into panel. Reproduce step: 1. Open Settings app, go to Date & Time panel 2. Set a time zone manually 3. enable auto timezone, the timezone is set back to current timezone 4. close app 5. open app, go to Date & Time panel Expect: Check the statusbar clock, The time does not change to manual timezone Actual: The time is set to manual timezone 6. disable auto time Expect: The time changed to manual timezone -> the time zone should be the user selected one. The other step: 1. Open Settings app, go to Date & Time panel 2. changed manual timezone to some other timezone 3. enable auto timezone, the timezone is set back to current timezone 4. reset the phone expect: the statusbar clock is set to the current(auto timezone) actual: the statusbar clock is set to the manual timezone After checking the DateTime modification history, https://github.com/mozilla-b2g/gaia/commits/master/apps/settings/js/date_time.js For 2.1 There's only Bug 1042774 modified the gaia code. revert it and that bug still occurred. set qawanted to check if it also affect other version

The bug is found during refactor date/time panel in bug 973441. Brief Phenomenon: Set auto timezone and restart the phone, the statusbar clock shows manual timezone but not auto timezone after restart.

The issue is happened by bug 1026098 in tz_select https://github.com/mozilla-b2g/gaia/commits/master/shared/js/tz_select.js (only in 2.1) After revert it to older tz_select.js, both Phenomenons are gone.

[Blocking Requested - why for this release]: This might be a dupe of bug 1049149. Good news is that this confirms the root cause of the problem we're seeing here.

Moving to Gaia since this is broken in shared code across the settings app & FTE app.

Since this is a smoketest blocker, please address this with highest priority, or backing out the offending feature commit first. Thanks.

Comment on attachment 8472137 [details] bug-1050091-fix Drive by comments: Ted, we are going to need some tests for this scenario.

Michael, aren't there already tests for this? Didn't I do this because a test was failing?

Comment on attachment 8472137 [details] bug-1050091-fix (In reply to Ted Clancy [:tedders1] from comment #9) > Michael, aren't there already tests for this? Didn't I do this because a > test was failing? Smoketests are manual tests, right? I think Michael is talking about to unit/marionette tests. On the other hand, the patch is only solving part of the problems described: (In reply to Fred Lin [:gasolin] from comment #0) > Reproduce step: > > 1. Open Settings app, go to Date & Time panel > 2. Set a time zone manually > 3. enable auto timezone, the timezone is set back to current timezone unrelated to this bug, I think that when selecting automatic timezone, if the timezone changes we should show the current timezone on the selector. Right now the time changes (correct) but we still show a different timezone as selected (even if it's disabled, it might be confusing for the user to see Europe/London selected when the time shown is America/New York, for example). Maybe this is intended, or a different bug. > 4. close app > > 5. open app, go to Date & Time panel > > Expect: > Check the statusbar clock, The time does not change to manual timezone > > Actual: > The time is set to manual timezone This is fixed by the patch, time is not set to manual anymore. > > 6. disable auto time > > Expect: > The time changed to manual timezone > > -> the time zone should be the user selected one. This is not fixed by the patch. Time is still the same as it was when automatic enabled. > > The other step: > > 1. Open Settings app, go to Date & Time panel > 2. changed manual timezone to some other timezone > 3. enable auto timezone, the timezone is set back to current timezone > 4. reset the phone > > expect: > > the statusbar clock is set to the current(auto timezone) > > actual: > > the statusbar clock is set to the manual timezone This is still happening, but only for a short time. My guess is that it waits till the SIM gets connected to the network, and we can get the current time automatically. Anyway, the first time the phone takes is wrong. So even if the code looks good for a part-fix, unless we want to divide it in different patches (no problem from me), I can't r+ this one till it solves all the problems described. Plus, if Michael was referring to automation, it's gonna need unit tests.

(In reply to Ted Clancy [:tedders1] from comment #9) > Michael, aren't there already tests for this? Didn't I do this because a > test was failing? Yes, I am referring to automated tests.

Fernando, the problems you've identified are problems that already existed before Bug 973441. They aren't particular to this bug, and they won't delay smoketests. Could you please approve this patch so smoketests can be unblocked? I will look at fixing the other problems, but those are problems in Gecko (and fairly deep down too), not Gaia. > > the time zone should be the user selected one. > > This is not fixed by the patch. Time is still the same as it was when automatic enabled. I think :gasolin means the time in the statusbar. The time shown in the Settings app doesn't change, even if I revert Bug 973441.

Michael, is there a way I can write unit tests that involve rebooting the device?

(In reply to Ted Clancy [:tedders1] from comment #13) > Michael, is there a way I can write unit tests that involve rebooting the > device? No, in general unit tests are about testing the functionality of individual functions you write (or groups of functions). To test rebooting the device with unit tests, you would need to set up the inputs of your functions such that it would mimic the environment right after the device was rebooted. In general though, you should write a test for the function you modify that fails because of the STR in comment 0, and then passes after your patch.

(In reply to Ted Clancy [:tedders1] from comment #12) > Fernando, the problems you've identified are problems that already existed > before Bug 973441. They aren't particular to this bug, and they won't delay > smoketests. Oh, didn't pay attention to that, I just followed the STR to check if all the problems described in the bug were gone. But if this bug it's focusing only in problems created by the regression, it's fine with me. > > Could you please approve this patch so smoketests can be unblocked? No problem with that, just need a clarification of the problems it's solving and the ones we should leave for other patches. I'm assuming the first 1-5 steps are the main fix (patch working for this), with the step 6 to be left for a new bug (as it was incorrect before the regression), and the same for the other 1-4 steps (statusbar is correct after connected to network, but not immediately if SIM needs PIN). Am I correct? > > I will look at fixing the other problems, but those are problems in Gecko > (and fairly deep down too), not Gaia. Thanks. Can you open the correspondent bugs please? > > > > the time zone should be the user selected one. > > > > This is not fixed by the patch. Time is still the same as it was when automatic enabled. > > I think :gasolin means the time in the statusbar. The time shown in the > Settings app doesn't change, even if I revert Bug 973441. Yeah, I was referring to the statusbar too. But as it's not part of the regression, we can carry on. Another thing is, I am peer for FTU, so I can 'officially' confirm with the review that FTU is not affected by the changes made to the shared files. I'm not sure though that my review is ok with the project WoW related to the changes affecting the Settings app, so to be on the safe side I'd ask for review to one of their peers.

Comment on attachment 8472137 [details] bug-1050091-fix and of course, I forget to mark it reviewed for FTU.

Also, it looks like there is a gaia unit test failure for timezone_test.js: https://tbpl.mozilla.org/?rev=838e2b142356c06f9734a8a2f391716e4ab657fd&tree=Gaia-Try

Comment on attachment 8472137 [details] bug-1050091-fix Step 6 is not fixed by this patch. I'm fine with unblocking the smoke test first with a followup bug filed for the issue. Thanks.

Here's a link to a green TBPL run. (There's a slight spot of orange, but it's not related to this patch; I saw the same thing on someone else's TBPL run.) https://tbpl.mozilla.org/?rev=d65582e2f5e8bb07903f1a5a7cf6657f7e8cefaa&tree=Gaia-Try

Comment on attachment 8472137 [details] bug-1050091-fix Ugh. Sorry, I just remembered that I added a line to this patch since it was last reviews. (To fix a failing unit test.) Do I need to get it reviewed again?

*since it was last reviewed

Comment on attachment 8472137 [details] bug-1050091-fix (In reply to Ted Clancy [:tedders1] from comment #20) > Comment on attachment 8472137 [details] > bug-1050091-fix > > Ugh. Sorry, I just remembered that I added a line to this patch since it was > last reviews. (To fix a failing unit test.) Do I need to get it reviewed > again? That extra return statement looks obvious to me. Since tests are also green now, let's land to unblock smoke tests.

master: https://github.com/mozilla-b2g/gaia/commit/ec5baf18d4675d01ed912d57bcef0548267b43e3 Ted please file followups for both adding the unit test, and to address Arther's concern in comment 7.

I am unable to reproduce this issue in the latest v2.1 Flame build: Enviromental Variables: ---------------------------------------- Device: Flame 2.1 Master BuildID: 20140819040202 Gaia: b33b4d9558e0b9eabbfda7be23435e2b38fd40bf Gecko: 111a1da2a95d Version: 34.0a1 (2.1 Master) Firmware: V123 User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0 Verified as fixed.