666215 - Timezone specific bugs in AVM unit tests

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Alok Manchanda]

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

Comment 1

[Alok Manchanda]

Attachment: Proposed fix

Comment 2

[Lars T Hansen]

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.

Comment 3

[Alok Manchanda]

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

[Steven Johnson]

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?

Comment 5

[Alok Manchanda]

Attachment: Fix DST issue when running in India

Comment 6

[Alok Manchanda]

Attachment: Fix for getminutes failure in odd multiple of 30min UTC offset time zone

Comment 7

[Lars T Hansen]

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.

Comment 8

[Alok Manchanda]

Thanks Lars! I've added Dan. 
Dan - if you too think the change looks OK, would you please push to tamarin for me?
-alok.

Comment 9

[Dan Schaffer]

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).

Comment 10

[Dan Schaffer]

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).

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

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

[Tamarin Bot]

changeset: 6444:8f05320b0c52
user:      Alok Manchanda <[e-mail]>
summary:   Bug 666215: Fix DST issue when running in India (r=dschaffe,r=lhansen).

http://hg.mozilla.org/tamarin-redux/rev/8f05320b0c52

Comment 13

[Tamarin Bot]

changeset: 6445:b368fd3f4322
user:      Alok Manchanda <[e-mail]>
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

[Felix S. Klock II [:pnkfelix, :fklock]]

(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.)

Comment 15

[Alok Manchanda]

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.

Comment 16

[Alok Manchanda]

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

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.