Closed
Bug 666215
Opened 13 years ago
Closed 6 years ago
Timezone specific bugs in AVM unit tests
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P3)
Tamarin Graveyard
Virtual Machine
Tracking
(Not tracked)
RESOLVED
WONTFIX
Q1 12 - Brannan
People
(Reporter: almancha, Assigned: almancha)
Details
Attachments
(2 files, 1 obsolete file)
995 bytes,
patch
|
dschaffe
:
review+
lhansen
:
review+
|
Details | Diff | Splinter Review |
798 bytes,
patch
|
dschaffe
:
review+
lhansen
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_7) AppleWebKit/534.30 (KHTML, like Gecko) Chrome/12.0.742.100 Safari/534.30 Build Identifier: Some AVM unit tests are written in a manner that will only work in specific timezones. Specifically they will fail in: 1) Timezones whose GMT offset is an odd multiple of 1/2 hour 2) Timezones that don't use daylight savings time Reproducible: Always
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #541010 -
Flags: review?(cpeyer)
Assignee | ||
Updated•13 years ago
|
Attachment #541010 -
Flags: review?(stejohns)
Comment 2•13 years ago
|
||
Comment on attachment 541010 [details] [diff] [review] Proposed fix Two comments. Most of us are not in California (I'm in Oslo, lots of people are on the east coast) so this patch will break functionality for many of us if your comment in DaylightSavingTA is true. Tab characters in the patch - please use spaces. Getting the test cases right have been a problem, and I can live with the occasional errors around the time when we change from Daylight savings to normal time and vice versa. I think it's worth trying to fix the problem for no-DST properly, as well as for the half-hour time zone. But it does not seem that this patch fixes that.
Assignee | ||
Comment 3•13 years ago
|
||
Oops, my bad, sorry Lars! I'll fix to use spaces, no problem. The half-hour time zone issue is fixed by updating the minutes using MinFromTime, do you think that's incorrect? It should work in all geographies. You're right about the DST issue. Do the below tests currently succeed for you? ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_10_11.abc ecma3/Date/e15_9_5_14.abc ecma3/Date/e15_9_5_14.abc ecma3/Date/e15_9_5_14.abc ecma3/Date/e15_9_5_14.abc ecma3/Date/e15_9_5_14.abc ecma3/Date/e15_9_5_14.abc ecma3/Date/e15_9_5_28_1.abc ecma3/Date/e15_9_5_28_1.abc ecma3/Date/e15_9_5_28_1.abc ecma3/Date/e15_9_5_29_1.abc ecma3/Date/e15_9_5_34_1.abc ecma3/Date/e15_9_5_34_1.abc ecma3/Date/e15_9_5_34_1.abc ecma3/Date/e15_9_5_34_1.abc ecma3/Date/e15_9_5_34_1.abc ecma3/Date/e15_9_5_34_1.abc ecma3/Date/e15_9_5_34_1.abc ecma3/Date/e15_9_5_34_1.abc One way I can think of to fix this is to enumerate geographies (through GMT offset) where these tests are to be run and return the right DST value for that location in this function instead of returning 0 for anything other than Pacific Time as I currently did. Do you think that sounds acceptable?
Comment 4•13 years ago
|
||
Comment on attachment 541010 [details] [diff] [review] Proposed fix Review of attachment 541010 [details] [diff] [review]: ----------------------------------------------------------------- This fix will break things for people outside of Pacific Time that still have DST. Perhaps we could add a way to query the OS as to whether we are in a DST zone?
Attachment #541010 -
Flags: review?(stejohns) → review-
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #541010 -
Attachment is obsolete: true
Attachment #541336 -
Flags: review?(stejohns)
Attachment #541010 -
Flags: review?(cpeyer)
Assignee | ||
Updated•13 years ago
|
Attachment #541336 -
Flags: review?(lhansen)
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #541337 -
Flags: review?(stejohns)
Assignee | ||
Updated•13 years ago
|
Attachment #541337 -
Flags: review?(lhansen)
Updated•13 years ago
|
Attachment #541336 -
Flags: review?(lhansen) → review+
Updated•13 years ago
|
Attachment #541337 -
Flags: review?(lhansen) → review+
Comment 7•13 years ago
|
||
R+ based on testing in Oslo, and the code looks fine. Alok, if you think you need a second review I suggest Dan Schaffer (dschaffe) instead of Steven, since Dan is with QE and does most of the work around this part of the test suite, IIRC.
Assignee | ||
Updated•13 years ago
|
Attachment #541336 -
Flags: review?(stejohns) → review?(dschaffe)
Assignee | ||
Updated•13 years ago
|
Attachment #541337 -
Flags: review?(stejohns) → review?(dschaffe)
Assignee | ||
Comment 8•13 years ago
|
||
Thanks Lars! I've added Dan. Dan - if you too think the change looks OK, would you please push to tamarin for me? -alok.
Assignee: nobody → almancha
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug-
Priority: -- → P3
Target Milestone: --- → Q1 12 - Brannan
Comment 9•13 years ago
|
||
Comment on attachment 541336 [details] [diff] [review] Fix DST issue when running in India patch looks good. it passed all platforms in the sandbox (on EDT, US east daylight savings).
Attachment #541336 -
Flags: review?(dschaffe) → review+
Comment 10•13 years ago
|
||
Comment on attachment 541337 [details] [diff] [review] Fix for getminutes failure in odd multiple of 30min UTC offset time zone patch looks good. it passed all platforms in the sandbox (on EDT, US east daylight savings).
Attachment #541337 -
Flags: review?(dschaffe) → review+
Comment 11•13 years ago
|
||
Comment on attachment 541336 [details] [diff] [review] Fix DST issue when running in India Review of attachment 541336 [details] [diff] [review]: ----------------------------------------------------------------- ::: test/acceptance/shell.as @@ +592,5 @@ > } > > function DaylightSavingTA( t ) { > + // There is no Daylight saving time in India > + if (IST_DIFF == 0) There is a trailing space after the if condition expression above. We have whitespace checkers that flag such additions to the repository (because we are trying to normalize the code to avoid spurious whitespace deltas). Alok: If you haven't already, please install the tamarin commit hook in your repo: http://hg.mozilla.org/tamarin-redux/file/tip/utils/hooks/tamarin-commit-hook.py. That way you will catch such problems as soon as you attempt to e.g. create an MQ changeset.
Comment 12•13 years ago
|
||
changeset: 6444:8f05320b0c52 user: Alok Manchanda <almancha@adobe.com> summary: Bug 666215: Fix DST issue when running in India (r=dschaffe,r=lhansen). http://hg.mozilla.org/tamarin-redux/rev/8f05320b0c52
Comment 13•13 years ago
|
||
changeset: 6445:b368fd3f4322 user: Alok Manchanda <almancha@adobe.com> summary: Bug 666215: Fix for getminutes failure in odd multiple of 30min UTC offset time zone (r=dschaffe,r=lhansen). http://hg.mozilla.org/tamarin-redux/rev/b368fd3f4322
Comment 14•13 years ago
|
||
(In reply to comment #4) > Comment on attachment 541010 [details] [diff] [review] [review] > Proposed fix > > Review of attachment 541010 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > This fix will break things for people outside of Pacific Time that still > have DST. Perhaps we could add a way to query the OS as to whether we are in > a DST zone? Alok: I did not see a response to this comment from Steven, and I'm not clear on what the situation is from your dialogue with Lars in comment 2, comment 3, and comment 7, it sounds like things are fine in Oslo. My googling indicates that Norway does observe DST. Is the problem unresolved in full generality? If so, I recommend clarifying that point, and probably filing a follow-up bug to address that. (Incidentally, I will probably be looking into Bug 606478 soonish, and that may end up requiring some related effort.)
Assignee | ||
Comment 15•13 years ago
|
||
Oops, sorry Felix. I wasn't aware of the requirement. I don't have commit permission to tamarin - would the hook also flag issues when creating patches? (In reply to comment #11) > Comment on attachment 541336 [details] [diff] [review] [review] > Fix DST issue when running in India > > Review of attachment 541336 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > ::: test/acceptance/shell.as > @@ +592,5 @@ > > } > > > > function DaylightSavingTA( t ) { > > + // There is no Daylight saving time in India > > + if (IST_DIFF == 0) > > There is a trailing space after the if condition expression above. We have > whitespace checkers that flag such additions to the repository (because we > are trying to normalize the code to avoid spurious whitespace deltas). > > Alok: If you haven't already, please install the tamarin commit hook in your > repo: > http://hg.mozilla.org/tamarin-redux/file/tip/utils/hooks/tamarin-commit-hook. > py. That way you will catch such problems as soon as you attempt to e.g. > create an MQ changeset.
Assignee | ||
Comment 16•13 years ago
|
||
The fix added an exception for IST to the DST function in the unit tests. The existing logic was unchanged for other geographies, so the problem is not fixed in general. Note however that it's a bug in the unit test code and not in AVM itself. (In reply to comment #14) > (In reply to comment #4) > > Comment on attachment 541010 [details] [diff] [review] [review] [review] > > Proposed fix > > > > Review of attachment 541010 [details] [diff] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > This fix will break things for people outside of Pacific Time that still > > have DST. Perhaps we could add a way to query the OS as to whether we are in > > a DST zone? > > Alok: I did not see a response to this comment from Steven, and I'm not > clear on what the situation is from your dialogue with Lars in comment 2, > comment 3, and comment 7, it sounds like things are fine in Oslo. My > googling indicates that Norway does observe DST. > > Is the problem unresolved in full generality? If so, I recommend clarifying > that point, and probably filing a follow-up bug to address that. > > (Incidentally, I will probably be looking into Bug 606478 soonish, and that > may end up requiring some related effort.)
Comment 17•6 years ago
|
||
Tamarin is a dead project now. Mass WONTFIX.
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Comment 18•6 years ago
|
||
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
You need to log in
before you can comment on or make changes to this bug.
Description
•