code review 7323063: exp/cookiejar: replace fake creation time by order parameter

exp/cookiejar: make cookie sorting deterministic

Sorting of cookies with same path length and same creation
time is done by an additional seqNum field.
This makes the order in which cookies are returned in Cookies
deterministic, even if the system clock is manipulated or on
systems with a low-resolution clock.

The tests now use a synthetic time: This makes cookie testing
reliable in case of bogus system clocks and speeds up the
expiration tests.

[volker.dobler, 2013-02-14 09:51:32]

Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[volker.dobler, 2013-02-14 10:13:46]

Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2013-02-14 23:41:44]

Still fails on windows:

=== RUN TestCanonicalHost
--- PASS: TestCanonicalHost (0.00 seconds)
=== RUN TestHasPort
--- PASS: TestHasPort (0.00 seconds)
=== RUN TestJarKey
--- PASS: TestJarKey (0.00 seconds)
=== RUN TestIsIP
--- PASS: TestIsIP (0.00 seconds)
=== RUN TestDefaultPath
--- PASS: TestDefaultPath (0.00 seconds)
=== RUN TestDomainAndType
--- PASS: TestDomainAndType (0.00 seconds)
=== RUN TestBasics
--- PASS: TestBasics (0.00 seconds)
=== RUN TestUpdateAndDelete
--- FAIL: TestUpdateAndDelete (0.00 seconds)
jar_test.go:266: 	Test "Delete all." Content
		got  "b=2"
		want ""
jar_test.go:276: 	Test "Delete all." #0
		got  "b=2"
		want ""
jar_test.go:266: 	Test "Refill #1." Content
		got  "A=1 A=2 A=3 A=4 b=2"
		want "A=1 A=2 A=3 A=4"
jar_test.go:276: 	Test "Refill #1." #0
		got  "A=2 A=4 b=2 A=1 A=3"
		want "A=2 A=4 A=1 A=3"
jar_test.go:266: 	Test "Refill #2." Content
		got  "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9"
jar_test.go:276: 	Test "Refill #2." #0
		got  "A=2 A=4 b=2 A=1 A=3"
		want "A=2 A=4 A=1 A=3"
jar_test.go:266: 	Test "Delete A7." Content
		got  "A=1 A=2 A=3 A=4 A=6 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=4 A=6 A=8 A=9"
jar_test.go:276: 	Test "Delete A7." #0
		got  "A=2 A=4 b=2 A=1 A=3"
		want "A=2 A=4 A=1 A=3"
jar_test.go:266: 	Test "Delete A4." Content
		got  "A=1 A=2 A=3 A=6 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=6 A=8 A=9"
jar_test.go:276: 	Test "Delete A4." #0
		got  "A=2 b=2 A=1 A=3"
		want "A=2 A=1 A=3"
jar_test.go:266: 	Test "Delete A6." Content
		got  "A=1 A=2 A=3 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=8 A=9"
jar_test.go:276: 	Test "Delete A6." #0
		got  "A=2 b=2 A=1 A=3"
		want "A=2 A=1 A=3"
jar_test.go:266: 	Test "Delete A3." Content
		got  "A=1 A=2 A=8 A=9 b=2"
		want "A=1 A=2 A=8 A=9"
jar_test.go:276: 	Test "Delete A3." #0
		got  "A=2 b=2 A=1"
		want "A=2 A=1"
jar_test.go:266: 	Test "No cross-domain delete." Content
		got  "A=1 A=2 A=8 A=9 b=2"
		want "A=1 A=2 A=8 A=9"
jar_test.go:276: 	Test "No cross-domain delete." #0
		got  "A=2 b=2 A=1"
		want "A=2 A=1"
jar_test.go:266: 	Test "Delete A8 and A9." Content
		got  "A=1 A=2 b=2"
		want "A=1 A=2"
jar_test.go:276: 	Test "Delete A8 and A9." #0
		got  "A=2 b=2 A=1"
		want "A=2 A=1"
=== RUN TestExpiration
--- FAIL: TestExpiration (1.50 seconds)
jar_test.go:266: 	Test "Check jar." Content
		got  "a=1 c=3 d=4"
		want "a=1 d=4"
jar_test.go:276: 	Test "Check jar." #0
		got  "a=1 c=3 d=4"
		want "a=1 d=4"
=== RUN TestChromiumBasics
--- PASS: TestChromiumBasics (0.00 seconds)
=== RUN TestChromiumDomain
--- PASS: TestChromiumDomain (0.00 seconds)
=== RUN TestChromiumDeletion
--- FAIL: TestChromiumDeletion (0.00 seconds)
jar_test.go:266: 	Test "Delete sc b2 via Expires." Content
		got  "b=2"
		want ""
jar_test.go:276: 	Test "Delete sc b2 via Expires." #0
		got  "b=2"
		want ""
jar_test.go:266: 	Test "Create persistent cookie c3." Content
		got  "b=2 c=3"
		want "c=3"
jar_test.go:276: 	Test "Create persistent cookie c3." #0
		got  "b=2 c=3"
		want "c=3"
jar_test.go:266: 	Test "Delete pc c3 via MaxAge." Content
		got  "b=2"
		want ""
jar_test.go:276: 	Test "Delete pc c3 via MaxAge." #0
		got  "b=2"
		want ""
jar_test.go:266: 	Test "Create persistent cookie d4." Content
		got  "b=2 d=4"
		want "d=4"
jar_test.go:276: 	Test "Create persistent cookie d4." #0
		got  "b=2 d=4"
		want "d=4"
jar_test.go:266: 	Test "Delete pc d4 via Expires." Content
		got  "b=2 d=4"
		want ""
jar_test.go:276: 	Test "Delete pc d4 via Expires." #0
		got  "b=2 d=4"
		want ""
FAIL

Alex

[nigeltao, 2013-02-15 04:43:00]

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:68: cnt uint64
I'd call the field nextSeqNum.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:84: // The upper case fields are those of RFC 6265.
// entry is the internal representation of a cookie.
//
// This struct type is not used outside of this package per se, but the exported
// fields are those of RFC 6265.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:97: order      uint64 // sequence number of cookies
set in one Set-Cookie header
I'd write
LastAccess time.Time

// seqNum is a sequence number so that Cookies returns cookies in a 
// deterministic order, even for cookies that have equal Path length and
// equal Creation time. This simplifies testing.
seqNum uint64

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:142: in, jn := len(s[i].Path), len(s[j].Path)
How about

if len(s[i].Path) != len(s[j].Path) {
  return len(s[i].Path) > len(s[j].Path)
}
if !s[i].Creation.Equal(s[j].Creation) {
  return s[i].Creation.Before(s[j].Creation)
}
return s[i].seqNum < s[j].seqNum

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:270: e.order = j.cnt
e.seqNum = j.nextSeqNum
j.nextSeqNum++

[volker.dobler, 2013-02-15 08:15:01]

PTAL

Incorporated all the feedback and tried to make the
expiration code more stable by:
* replacing "expires < now" by "!(expires > now)"
* increasing some deltas
* waiting 1.6 seconds for expiration as rounding may
  create Expires 1.5 seconds in the future.

I did some tests with a complete fake clock which reports
the same time instance and the deletion tests pass.
So this should fix the Windows build.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:68: cnt uint64
On 2013/02/15 04:43:01, nigeltao wrote:
> I'd call the field nextSeqNum.

Done.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:84: // The upper case fields are those of RFC 6265.
On 2013/02/15 04:43:01, nigeltao wrote:
> // entry is the internal representation of a cookie.
> //
> // This struct type is not used outside of this package per se, but the
exported
> // fields are those of RFC 6265.

Done.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:97: order      uint64 // sequence number of cookies
set in one Set-Cookie header
On 2013/02/15 04:43:01, nigeltao wrote:
> I'd write
> LastAccess time.Time
> 
> // seqNum is a sequence number so that Cookies returns cookies in a 
> // deterministic order, even for cookies that have equal Path length and
> // equal Creation time. This simplifies testing.
> seqNum uint64

Done.

I kept the "simplifies testing" even if I think that the
deterministic order is required by RFC 6265.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:142: in, jn := len(s[i].Path), len(s[j].Path)
On 2013/02/15 04:43:01, nigeltao wrote:
> How about
> 
> if len(s[i].Path) != len(s[j].Path) {
>   return len(s[i].Path) > len(s[j].Path)
> }
> if !s[i].Creation.Equal(s[j].Creation) {
>   return s[i].Creation.Before(s[j].Creation)
> }
> return s[i].seqNum < s[j].seqNum

Done.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:270: e.order = j.cnt
On 2013/02/15 04:43:01, nigeltao wrote:
> e.seqNum = j.nextSeqNum
> j.nextSeqNum++

Done.

[brainman, 2013-02-15 10:06:19]

Still fails on windows:

--- FAIL: TestUpdateAndDelete (0.00 seconds)
jar_test.go:266: 	Test "Delete all." Content
		got  "b=2"
		want ""
jar_test.go:276: 	Test "Delete all." #0
		got  "b=2"
		want ""
jar_test.go:266: 	Test "Refill #1." Content
		got  "A=1 A=2 A=3 A=4 b=2"
		want "A=1 A=2 A=3 A=4"
jar_test.go:276: 	Test "Refill #1." #0
		got  "A=2 A=4 b=2 A=1 A=3"
		want "A=2 A=4 A=1 A=3"
jar_test.go:266: 	Test "Refill #2." Content
		got  "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9"
jar_test.go:276: 	Test "Refill #2." #0
		got  "A=2 A=4 b=2 A=1 A=3"
		want "A=2 A=4 A=1 A=3"
jar_test.go:266: 	Test "Delete A7." Content
		got  "A=1 A=2 A=3 A=4 A=6 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=4 A=6 A=8 A=9"
jar_test.go:276: 	Test "Delete A7." #0
		got  "A=2 A=4 b=2 A=1 A=3"
		want "A=2 A=4 A=1 A=3"
jar_test.go:266: 	Test "Delete A4." Content
		got  "A=1 A=2 A=3 A=6 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=6 A=8 A=9"
jar_test.go:276: 	Test "Delete A4." #0
		got  "A=2 b=2 A=1 A=3"
		want "A=2 A=1 A=3"
jar_test.go:266: 	Test "Delete A6." Content
		got  "A=1 A=2 A=3 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=8 A=9"
jar_test.go:276: 	Test "Delete A6." #0
		got  "A=2 b=2 A=1 A=3"
		want "A=2 A=1 A=3"
jar_test.go:266: 	Test "Delete A3." Content
		got  "A=1 A=2 A=8 A=9 b=2"
		want "A=1 A=2 A=8 A=9"
jar_test.go:276: 	Test "Delete A3." #0
		got  "A=2 b=2 A=1"
		want "A=2 A=1"
jar_test.go:266: 	Test "No cross-domain delete." Content
		got  "A=1 A=2 A=8 A=9 b=2"
		want "A=1 A=2 A=8 A=9"
jar_test.go:276: 	Test "No cross-domain delete." #0
		got  "A=2 b=2 A=1"
		want "A=2 A=1"
jar_test.go:266: 	Test "Delete A8 and A9." Content
		got  "A=1 A=2 b=2"
		want "A=1 A=2"
jar_test.go:276: 	Test "Delete A8 and A9." #0
		got  "A=2 b=2 A=1"
		want "A=2 A=1"
--- FAIL: TestExpiration (1.60 seconds)
jar_test.go:266: 	Test "Check jar." Content
		got  "a=1 c=3 d=4"
		want "a=1 d=4"
jar_test.go:276: 	Test "Check jar." #0
		got  "a=1 c=3 d=4"
		want "a=1 d=4"
--- FAIL: TestChromiumDeletion (0.00 seconds)
jar_test.go:266: 	Test "Delete sc b2 via Expires." Content
		got  "b=2"
		want ""
jar_test.go:276: 	Test "Delete sc b2 via Expires." #0
		got  "b=2"
		want ""
jar_test.go:266: 	Test "Create persistent cookie c3." Content
		got  "b=2 c=3"
		want "c=3"
jar_test.go:276: 	Test "Create persistent cookie c3." #0
		got  "b=2 c=3"
		want "c=3"
jar_test.go:266: 	Test "Delete pc c3 via MaxAge." Content
		got  "b=2"
		want ""
jar_test.go:276: 	Test "Delete pc c3 via MaxAge." #0
		got  "b=2"
		want ""
jar_test.go:266: 	Test "Create persistent cookie d4." Content
		got  "b=2 d=4"
		want "d=4"
jar_test.go:276: 	Test "Create persistent cookie d4." #0
		got  "b=2 d=4"
		want "d=4"
jar_test.go:266: 	Test "Delete pc d4 via Expires." Content
		got  "b=2 d=4"
		want ""
jar_test.go:276: 	Test "Delete pc d4 via Expires." #0
		got  "b=2 d=4"
		want ""
FAIL
FAIL	exp/cookiejar	2.240s

[volker.dobler, 2013-02-15 11:33:05]

That is strange. I cannot reproduce the failure on
Windows 64.

Could you try it once more with 1600 replaced by 10000
in jar_test.go:612 just to see if it is a timing issue or
something fundamental.

Thanks in advance.




On Fri, Feb 15, 2013 at 11:06 AM, <alex.brainman@gmail.com> wrote:

> Still fails on windows:
>
>
> --- FAIL: TestUpdateAndDelete (0.00 seconds)
> jar_test.go:266:        Test "Delete all." Content
>                 got  "b=2"
>                 want ""
> jar_test.go:276:        Test "Delete all." #0
>                 got  "b=2"
>                 want ""
> jar_test.go:266:        Test "Refill #1." Content
>                 got  "A=1 A=2 A=3 A=4 b=2"
>                 want "A=1 A=2 A=3 A=4"
> jar_test.go:276:        Test "Refill #1." #0
>                 got  "A=2 A=4 b=2 A=1 A=3"
>                 want "A=2 A=4 A=1 A=3"
> jar_test.go:266:        Test "Refill #2." Content
>                 got  "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9 b=2"
>                 want "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9"
> jar_test.go:276:        Test "Refill #2." #0
>                 got  "A=2 A=4 b=2 A=1 A=3"
>                 want "A=2 A=4 A=1 A=3"
> jar_test.go:266:        Test "Delete A7." Content
>                 got  "A=1 A=2 A=3 A=4 A=6 A=8 A=9 b=2"
>                 want "A=1 A=2 A=3 A=4 A=6 A=8 A=9"
> jar_test.go:276:        Test "Delete A7." #0
>                 got  "A=2 A=4 b=2 A=1 A=3"
>                 want "A=2 A=4 A=1 A=3"
> jar_test.go:266:        Test "Delete A4." Content
>                 got  "A=1 A=2 A=3 A=6 A=8 A=9 b=2"
>                 want "A=1 A=2 A=3 A=6 A=8 A=9"
> jar_test.go:276:        Test "Delete A4." #0
>                 got  "A=2 b=2 A=1 A=3"
>                 want "A=2 A=1 A=3"
> jar_test.go:266:        Test "Delete A6." Content
>                 got  "A=1 A=2 A=3 A=8 A=9 b=2"
>                 want "A=1 A=2 A=3 A=8 A=9"
> jar_test.go:276:        Test "Delete A6." #0
>                 got  "A=2 b=2 A=1 A=3"
>                 want "A=2 A=1 A=3"
> jar_test.go:266:        Test "Delete A3." Content
>                 got  "A=1 A=2 A=8 A=9 b=2"
>                 want "A=1 A=2 A=8 A=9"
> jar_test.go:276:        Test "Delete A3." #0
>                 got  "A=2 b=2 A=1"
>                 want "A=2 A=1"
> jar_test.go:266:        Test "No cross-domain delete." Content
>                 got  "A=1 A=2 A=8 A=9 b=2"
>                 want "A=1 A=2 A=8 A=9"
> jar_test.go:276:        Test "No cross-domain delete." #0
>                 got  "A=2 b=2 A=1"
>                 want "A=2 A=1"
> jar_test.go:266:        Test "Delete A8 and A9." Content
>                 got  "A=1 A=2 b=2"
>                 want "A=1 A=2"
> jar_test.go:276:        Test "Delete A8 and A9." #0
>                 got  "A=2 b=2 A=1"
>                 want "A=2 A=1"
> --- FAIL: TestExpiration (1.60 seconds)
>
> jar_test.go:266:        Test "Check jar." Content
>                 got  "a=1 c=3 d=4"
>                 want "a=1 d=4"
> jar_test.go:276:        Test "Check jar." #0
>                 got  "a=1 c=3 d=4"
>                 want "a=1 d=4"
> --- FAIL: TestChromiumDeletion (0.00 seconds)
> jar_test.go:266:        Test "Delete sc b2 via Expires." Content
>                 got  "b=2"
>                 want ""
> jar_test.go:276:        Test "Delete sc b2 via Expires." #0
>                 got  "b=2"
>                 want ""
> jar_test.go:266:        Test "Create persistent cookie c3." Content
>                 got  "b=2 c=3"
>                 want "c=3"
> jar_test.go:276:        Test "Create persistent cookie c3." #0
>                 got  "b=2 c=3"
>                 want "c=3"
> jar_test.go:266:        Test "Delete pc c3 via MaxAge." Content
>                 got  "b=2"
>                 want ""
> jar_test.go:276:        Test "Delete pc c3 via MaxAge." #0
>                 got  "b=2"
>                 want ""
> jar_test.go:266:        Test "Create persistent cookie d4." Content
>                 got  "b=2 d=4"
>                 want "d=4"
> jar_test.go:276:        Test "Create persistent cookie d4." #0
>                 got  "b=2 d=4"
>                 want "d=4"
> jar_test.go:266:        Test "Delete pc d4 via Expires." Content
>                 got  "b=2 d=4"
>                 want ""
> jar_test.go:276:        Test "Delete pc d4 via Expires." #0
>                 got  "b=2 d=4"
>                 want ""
> FAIL
> FAIL    exp/cookiejar   2.240s
>
>
>
https://codereview.appspot.**com/7323063/<https://codereview.appspot.com/7323...
>



-- 
Dr. Volker Dobler

[dave_cheney.net, 2013-02-15 11:33:49]

Time inside vm's is strange.

On Fri, Feb 15, 2013 at 10:33 PM, Volker Dobler
<dr.volker.dobler@gmail.com> wrote:
> That is strange. I cannot reproduce the failure on
> Windows 64.
>
> Could you try it once more with 1600 replaced by 10000
> in jar_test.go:612 just to see if it is a timing issue or
> something fundamental.
>
> Thanks in advance.
>
>
>
>
> On Fri, Feb 15, 2013 at 11:06 AM, <alex.brainman@gmail.com> wrote:
>>
>> Still fails on windows:
>>
>>
>> --- FAIL: TestUpdateAndDelete (0.00 seconds)
>> jar_test.go:266:        Test "Delete all." Content
>>                 got  "b=2"
>>                 want ""
>> jar_test.go:276:        Test "Delete all." #0
>>                 got  "b=2"
>>                 want ""
>> jar_test.go:266:        Test "Refill #1." Content
>>                 got  "A=1 A=2 A=3 A=4 b=2"
>>                 want "A=1 A=2 A=3 A=4"
>> jar_test.go:276:        Test "Refill #1." #0
>>                 got  "A=2 A=4 b=2 A=1 A=3"
>>                 want "A=2 A=4 A=1 A=3"
>> jar_test.go:266:        Test "Refill #2." Content
>>                 got  "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9 b=2"
>>                 want "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9"
>> jar_test.go:276:        Test "Refill #2." #0
>>                 got  "A=2 A=4 b=2 A=1 A=3"
>>                 want "A=2 A=4 A=1 A=3"
>> jar_test.go:266:        Test "Delete A7." Content
>>                 got  "A=1 A=2 A=3 A=4 A=6 A=8 A=9 b=2"
>>                 want "A=1 A=2 A=3 A=4 A=6 A=8 A=9"
>> jar_test.go:276:        Test "Delete A7." #0
>>                 got  "A=2 A=4 b=2 A=1 A=3"
>>                 want "A=2 A=4 A=1 A=3"
>> jar_test.go:266:        Test "Delete A4." Content
>>                 got  "A=1 A=2 A=3 A=6 A=8 A=9 b=2"
>>                 want "A=1 A=2 A=3 A=6 A=8 A=9"
>> jar_test.go:276:        Test "Delete A4." #0
>>                 got  "A=2 b=2 A=1 A=3"
>>                 want "A=2 A=1 A=3"
>> jar_test.go:266:        Test "Delete A6." Content
>>                 got  "A=1 A=2 A=3 A=8 A=9 b=2"
>>                 want "A=1 A=2 A=3 A=8 A=9"
>> jar_test.go:276:        Test "Delete A6." #0
>>                 got  "A=2 b=2 A=1 A=3"
>>                 want "A=2 A=1 A=3"
>> jar_test.go:266:        Test "Delete A3." Content
>>                 got  "A=1 A=2 A=8 A=9 b=2"
>>                 want "A=1 A=2 A=8 A=9"
>> jar_test.go:276:        Test "Delete A3." #0
>>                 got  "A=2 b=2 A=1"
>>                 want "A=2 A=1"
>> jar_test.go:266:        Test "No cross-domain delete." Content
>>                 got  "A=1 A=2 A=8 A=9 b=2"
>>                 want "A=1 A=2 A=8 A=9"
>> jar_test.go:276:        Test "No cross-domain delete." #0
>>                 got  "A=2 b=2 A=1"
>>                 want "A=2 A=1"
>> jar_test.go:266:        Test "Delete A8 and A9." Content
>>                 got  "A=1 A=2 b=2"
>>                 want "A=1 A=2"
>> jar_test.go:276:        Test "Delete A8 and A9." #0
>>                 got  "A=2 b=2 A=1"
>>                 want "A=2 A=1"
>> --- FAIL: TestExpiration (1.60 seconds)
>>
>> jar_test.go:266:        Test "Check jar." Content
>>                 got  "a=1 c=3 d=4"
>>                 want "a=1 d=4"
>> jar_test.go:276:        Test "Check jar." #0
>>                 got  "a=1 c=3 d=4"
>>                 want "a=1 d=4"
>> --- FAIL: TestChromiumDeletion (0.00 seconds)
>> jar_test.go:266:        Test "Delete sc b2 via Expires." Content
>>                 got  "b=2"
>>                 want ""
>> jar_test.go:276:        Test "Delete sc b2 via Expires." #0
>>                 got  "b=2"
>>                 want ""
>> jar_test.go:266:        Test "Create persistent cookie c3." Content
>>                 got  "b=2 c=3"
>>                 want "c=3"
>> jar_test.go:276:        Test "Create persistent cookie c3." #0
>>                 got  "b=2 c=3"
>>                 want "c=3"
>> jar_test.go:266:        Test "Delete pc c3 via MaxAge." Content
>>                 got  "b=2"
>>                 want ""
>> jar_test.go:276:        Test "Delete pc c3 via MaxAge." #0
>>                 got  "b=2"
>>                 want ""
>> jar_test.go:266:        Test "Create persistent cookie d4." Content
>>                 got  "b=2 d=4"
>>                 want "d=4"
>> jar_test.go:276:        Test "Create persistent cookie d4." #0
>>                 got  "b=2 d=4"
>>                 want "d=4"
>> jar_test.go:266:        Test "Delete pc d4 via Expires." Content
>>                 got  "b=2 d=4"
>>                 want ""
>> jar_test.go:276:        Test "Delete pc d4 via Expires." #0
>>                 got  "b=2 d=4"
>>                 want ""
>> FAIL
>> FAIL    exp/cookiejar   2.240s
>>
>>
>> https://codereview.appspot.com/7323063/
>
>
>
>
> --
> Dr. Volker Dobler
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[volker.dobler, 2013-02-15 18:09:59]

PTAL

Started to remove expiration tests based on passing time
by artificially creating expired cookies and checking
internal state.

[nigeltao, 2013-02-16 01:05:15]

Looks good in general, but let's see what brainman says.

https://codereview.appspot.com/7323063/diff/23001/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7323063/diff/23001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:67: // nextSeqNum is the next sequence number
assigned to a new cookie
s/cookie/entry/

https://codereview.appspot.com/7323063/diff/23001/src/pkg/exp/cookiejar/jar.g...
src/pkg/exp/cookiejar/jar.go:68: // created SetCookies.
created by SetCookies.

https://codereview.appspot.com/7323063/diff/23001/src/pkg/exp/cookiejar/jar_t...
File src/pkg/exp/cookiejar/jar_test.go (right):

https://codereview.appspot.com/7323063/diff/23001/src/pkg/exp/cookiejar/jar_t...
src/pkg/exp/cookiejar/jar_test.go:598: t.Skip("test is broken on windows/386")
// issue 4823
This one stays??

https://codereview.appspot.com/7323063/diff/23001/src/pkg/exp/cookiejar/jar_t...
src/pkg/exp/cookiejar/jar_test.go:632: "b=2; max-age=1000",       // should
expire in 1000 second
s/second/seconds/ and similarly on the next line.

[brainman, 2013-02-16 03:11:54]

On 2013/02/16 01:05:15, nigeltao wrote:
> Looks good in general, but let's see what brainman says.
> 

This still fails on my windows 7 amd64 notebook: 

--- FAIL: TestUpdateAndDelete (0.00 seconds)
jar_test.go:266: 	Test "Delete all." Content
		got  "b=2"
		want ""
jar_test.go:276: 	Test "Delete all." #0
		got  "b=2"
		want ""
jar_test.go:266: 	Test "Refill #1." Content
		got  "A=1 A=2 A=3 A=4 b=2"
		want "A=1 A=2 A=3 A=4"
jar_test.go:276: 	Test "Refill #1." #0
		got  "A=2 A=4 b=2 A=1 A=3"
		want "A=2 A=4 A=1 A=3"
jar_test.go:266: 	Test "Refill #2." Content
		got  "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9"
jar_test.go:276: 	Test "Refill #2." #0
		got  "A=2 A=4 b=2 A=1 A=3"
		want "A=2 A=4 A=1 A=3"
jar_test.go:266: 	Test "Delete A7." Content
		got  "A=1 A=2 A=3 A=4 A=6 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=4 A=6 A=8 A=9"
jar_test.go:276: 	Test "Delete A7." #0
		got  "A=2 A=4 b=2 A=1 A=3"
		want "A=2 A=4 A=1 A=3"
jar_test.go:266: 	Test "Delete A4." Content
		got  "A=1 A=2 A=3 A=6 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=6 A=8 A=9"
jar_test.go:276: 	Test "Delete A4." #0
		got  "A=2 b=2 A=1 A=3"
		want "A=2 A=1 A=3"
jar_test.go:266: 	Test "Delete A6." Content
		got  "A=1 A=2 A=3 A=8 A=9 b=2"
		want "A=1 A=2 A=3 A=8 A=9"
jar_test.go:276: 	Test "Delete A6." #0
		got  "A=2 b=2 A=1 A=3"
		want "A=2 A=1 A=3"
jar_test.go:266: 	Test "Delete A3." Content
		got  "A=1 A=2 A=8 A=9 b=2"
		want "A=1 A=2 A=8 A=9"
jar_test.go:276: 	Test "Delete A3." #0
		got  "A=2 b=2 A=1"
		want "A=2 A=1"
jar_test.go:266: 	Test "No cross-domain delete." Content
		got  "A=1 A=2 A=8 A=9 b=2"
		want "A=1 A=2 A=8 A=9"
jar_test.go:276: 	Test "No cross-domain delete." #0
		got  "A=2 b=2 A=1"
		want "A=2 A=1"
jar_test.go:266: 	Test "Delete A8 and A9." Content
		got  "A=1 A=2 b=2"
		want "A=1 A=2"
jar_test.go:276: 	Test "Delete A8 and A9." #0
		got  "A=2 b=2 A=1"
		want "A=2 A=1"
--- FAIL: TestPersistentCookies (0.00 seconds)
jar_test.go:656: 	Cookie "www.host.test;/;c": got session, want persistent
jar_test.go:660: 	Cookie "www.host.test;/;c": bad ttl -6213099928106725324 ns
--- FAIL: TestChromiumDeletion (0.00 seconds)
jar_test.go:266: 	Test "Delete sc b2 via Expires." Content
		got  "b=2"
		want ""
jar_test.go:276: 	Test "Delete sc b2 via Expires." #0
		got  "b=2"
		want ""
jar_test.go:266: 	Test "Create persistent cookie c3." Content
		got  "b=2 c=3"
		want "c=3"
jar_test.go:276: 	Test "Create persistent cookie c3." #0
		got  "b=2 c=3"
		want "c=3"
jar_test.go:266: 	Test "Delete pc c3 via MaxAge." Content
		got  "b=2"
		want ""
jar_test.go:276: 	Test "Delete pc c3 via MaxAge." #0
		got  "b=2"
		want ""
jar_test.go:266: 	Test "Create persistent cookie d4." Content
		got  "b=2 d=4"
		want "d=4"
jar_test.go:276: 	Test "Create persistent cookie d4." #0
		got  "b=2 d=4"
		want "d=4"
jar_test.go:266: 	Test "Delete pc d4 via Expires." Content
		got  "b=2 d=4"
		want ""
jar_test.go:276: 	Test "Delete pc d4 via Expires." #0
		got  "b=2 d=4"
		want ""
FAIL
FAIL	exp/cookiejar	0.800s

This code is unfamiliar, and I don't have time to investigate it closer now. I
will debug it properly when I have time. If you have a windows computer, you
could try it yourself. This
http://code.google.com/p/go-wiki/wiki/WindowsCrossCompiling is what I do. Just
build you test executable "GOOS=windows GOARCH=... go test -c", copy it to
windows, and run it with appropriate -test.... flags there.

Alex

PS: You still have TestExpiration disabled. I hope it is intentional.

[volker.dobler, 2013-02-16 08:46:56]

Thanks for running the test. I cross-compiled the test and on my
Windows 7 Enterprise, 64-bit, Service Pack 1 laptop I get:

D:\tmp>cookiejar.test.exe -test.v
=== RUN TestCanonicalHost
--- PASS: TestCanonicalHost (0.00 seconds)
=== RUN TestHasPort
--- PASS: TestHasPort (0.00 seconds)
=== RUN TestJarKey
--- PASS: TestJarKey (0.00 seconds)
=== RUN TestIsIP
--- PASS: TestIsIP (0.00 seconds)
=== RUN TestDefaultPath
--- PASS: TestDefaultPath (0.00 seconds)
=== RUN TestDomainAndType
--- PASS: TestDomainAndType (0.00 seconds)
=== RUN TestBasics
--- PASS: TestBasics (0.00 seconds)
=== RUN TestUpdateAndDelete
--- PASS: TestUpdateAndDelete (0.00 seconds)
=== RUN TestExpiration
--- SKIP: TestExpiration (0.00 seconds)
jar_test.go:598:        test is broken on windows/386
=== RUN TestPersistentCookies
--- PASS: TestPersistentCookies (0.00 seconds)
=== RUN TestExpired
--- PASS: TestExpired (0.00 seconds)
=== RUN TestChromiumBasics
--- PASS: TestChromiumBasics (0.00 seconds)
=== RUN TestChromiumDomain
--- PASS: TestChromiumDomain (0.00 seconds)
=== RUN TestChromiumDeletion
--- PASS: TestChromiumDeletion (0.00 seconds)
PASS

Maybe the tests should not use time.Now() but fixed
timestamps. I'll update the CL.

The skipped test is intentional. I think getting the
sequence Number into tip should be prio 1, followed
by getting the tests properly.

V.



On Sat, Feb 16, 2013 at 4:11 AM, <alex.brainman@gmail.com> wrote:

> On 2013/02/16 01:05:15, nigeltao wrote:
>
>> Looks good in general, but let's see what brainman says.
>>
>
>
> This still fails on my windows 7 amd64 notebook:
>
>
> --- FAIL: TestUpdateAndDelete (0.00 seconds)
> jar_test.go:266:        Test "Delete all." Content
>                 got  "b=2"
>                 want ""
> jar_test.go:276:        Test "Delete all." #0
>                 got  "b=2"
>                 want ""
> jar_test.go:266:        Test "Refill #1." Content
>                 got  "A=1 A=2 A=3 A=4 b=2"
>                 want "A=1 A=2 A=3 A=4"
> jar_test.go:276:        Test "Refill #1." #0
>                 got  "A=2 A=4 b=2 A=1 A=3"
>                 want "A=2 A=4 A=1 A=3"
> jar_test.go:266:        Test "Refill #2." Content
>                 got  "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9 b=2"
>                 want "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9"
> jar_test.go:276:        Test "Refill #2." #0
>                 got  "A=2 A=4 b=2 A=1 A=3"
>                 want "A=2 A=4 A=1 A=3"
> jar_test.go:266:        Test "Delete A7." Content
>                 got  "A=1 A=2 A=3 A=4 A=6 A=8 A=9 b=2"
>                 want "A=1 A=2 A=3 A=4 A=6 A=8 A=9"
> jar_test.go:276:        Test "Delete A7." #0
>                 got  "A=2 A=4 b=2 A=1 A=3"
>                 want "A=2 A=4 A=1 A=3"
> jar_test.go:266:        Test "Delete A4." Content
>                 got  "A=1 A=2 A=3 A=6 A=8 A=9 b=2"
>                 want "A=1 A=2 A=3 A=6 A=8 A=9"
> jar_test.go:276:        Test "Delete A4." #0
>                 got  "A=2 b=2 A=1 A=3"
>                 want "A=2 A=1 A=3"
> jar_test.go:266:        Test "Delete A6." Content
>                 got  "A=1 A=2 A=3 A=8 A=9 b=2"
>                 want "A=1 A=2 A=3 A=8 A=9"
> jar_test.go:276:        Test "Delete A6." #0
>                 got  "A=2 b=2 A=1 A=3"
>                 want "A=2 A=1 A=3"
> jar_test.go:266:        Test "Delete A3." Content
>                 got  "A=1 A=2 A=8 A=9 b=2"
>                 want "A=1 A=2 A=8 A=9"
> jar_test.go:276:        Test "Delete A3." #0
>                 got  "A=2 b=2 A=1"
>                 want "A=2 A=1"
> jar_test.go:266:        Test "No cross-domain delete." Content
>                 got  "A=1 A=2 A=8 A=9 b=2"
>                 want "A=1 A=2 A=8 A=9"
> jar_test.go:276:        Test "No cross-domain delete." #0
>                 got  "A=2 b=2 A=1"
>                 want "A=2 A=1"
> jar_test.go:266:        Test "Delete A8 and A9." Content
>                 got  "A=1 A=2 b=2"
>                 want "A=1 A=2"
> jar_test.go:276:        Test "Delete A8 and A9." #0
>                 got  "A=2 b=2 A=1"
>                 want "A=2 A=1"
> --- FAIL: TestPersistentCookies (0.00 seconds)
> jar_test.go:656:        Cookie "www.host.test;/;c": got session, want
> persistent
> jar_test.go:660:        Cookie "www.host.test;/;c": bad ttl
> -6213099928106725324 ns
>
> --- FAIL: TestChromiumDeletion (0.00 seconds)
> jar_test.go:266:        Test "Delete sc b2 via Expires." Content
>                 got  "b=2"
>                 want ""
> jar_test.go:276:        Test "Delete sc b2 via Expires." #0
>                 got  "b=2"
>                 want ""
> jar_test.go:266:        Test "Create persistent cookie c3." Content
>                 got  "b=2 c=3"
>                 want "c=3"
> jar_test.go:276:        Test "Create persistent cookie c3." #0
>                 got  "b=2 c=3"
>                 want "c=3"
> jar_test.go:266:        Test "Delete pc c3 via MaxAge." Content
>                 got  "b=2"
>                 want ""
> jar_test.go:276:        Test "Delete pc c3 via MaxAge." #0
>                 got  "b=2"
>                 want ""
> jar_test.go:266:        Test "Create persistent cookie d4." Content
>                 got  "b=2 d=4"
>                 want "d=4"
> jar_test.go:276:        Test "Create persistent cookie d4." #0
>                 got  "b=2 d=4"
>                 want "d=4"
> jar_test.go:266:        Test "Delete pc d4 via Expires." Content
>                 got  "b=2 d=4"
>                 want ""
> jar_test.go:276:        Test "Delete pc d4 via Expires." #0
>                 got  "b=2 d=4"
>                 want ""
> FAIL
> FAIL    exp/cookiejar   0.800s
>
> This code is unfamiliar, and I don't have time to investigate it closer
> now. I will debug it properly when I have time. If you have a windows
> computer, you could try it yourself. This
>
http://code.google.com/p/go-**wiki/wiki/**WindowsCrossCompiling<http://code.g...
what I
> do. Just build you test executable "GOOS=windows GOARCH=... go test -c",
> copy it to windows, and run it with appropriate -test.... flags there.
>
> Alex
>
> PS: You still have TestExpiration disabled. I hope it is intentional.
>
>
https://codereview.appspot.**com/7323063/<https://codereview.appspot.com/7323...
>

[brainman, 2013-02-17 06:59:14]

The problem seems to be my timezone. expiresIn in jar_test.go creates "expires=
... AUSEDT". http.readSetCookies fails to convert that string to time.Time, so
it sets Expires to time.Time{}. So all expires are effectively ignored.

I think, we need to fix time.Parse, so it accepts timezones like AUSEDT.

Alex

[volker.dobler, 2013-02-17 12:47:49]

Thanks for debugging this.
I changed the tests to a) use UTC as timezone and
b) to use a completely synthetic time. The tests thus
do no longer depend on the system time or the timezone.
This should make them reliable (and faster).

It seems worth fixing if time.Now returns AUSEDT but
time.Parse cannot handle it.  Did you file an issue?

V.


On Sun, Feb 17, 2013 at 7:59 AM, <alex.brainman@gmail.com> wrote:

> The problem seems to be my timezone. expiresIn in jar_test.go creates
> "expires= ... AUSEDT". http.readSetCookies fails to convert that string
> to time.Time, so it sets Expires to time.Time{}. So all expires are
> effectively ignored.
>
> I think, we need to fix time.Parse, so it accepts timezones like AUSEDT.
>
> Alex
>
>
https://codereview.appspot.**com/7323063/<https://codereview.appspot.com/7323...
>

[volker.dobler, 2013-02-17 12:48:50]

PTAL

Should work reliable on any OS and under any strange
system clock.

[brainman, 2013-02-17 23:35:51]

Tests PASS on my windows computer now. Thank you.

Alex

PS: I have created http://code.google.com/p/go/issues/detail?id=4838 for the bug
we discovered.

https://codereview.appspot.com/7323063/diff/32001/src/pkg/exp/cookiejar/jar_t...
File src/pkg/exp/cookiejar/jar_test.go (right):

https://codereview.appspot.com/7323063/diff/32001/src/pkg/exp/cookiejar/jar_t...
src/pkg/exp/cookiejar/jar_test.go:207: 
I do not think you need tNow. Just assume your location is UTC for the purpose
of this test:

now := time.Now().In(time.UTC)                  
t := now.Add(time.Duration(delta) * time.Second)

[nigeltao, 2013-02-18 00:27:03]

LGTM, submitting...

A couple of minor points. First, the CL description should mention re-enabling
those tests on Windows.

Second, I would make the now argument the last one, not the first one, for these
two methods:
func (j *Jar) cookies(now time.Time, u *url.URL) (cookies []*http.Cookie)
func (j *Jar) setCookies(now time.Time, u *url.URL, cookies []*http.Cookie)

Third, some of the s/10/1000/ changes can now be reverted.

As for tNow, I would prefer using a fixed tNow instead of calling time.Now; the
former is more predictable.

[nigeltao, 2013-02-18 00:27:58]

*** Submitted as https://code.google.com/p/go/source/detail?r=10534bf6553f ***

exp/cookiejar: make cookie sorting deterministic.

Re-enable TestUpdateAndDelete, TestExpiration, TestChromiumDomain and
TestChromiumDeletion on Windows.

Sorting of cookies with same path length and same creation
time is done by an additional seqNum field.
This makes the order in which cookies are returned in Cookies
deterministic, even if the system clock is manipulated or on
systems with a low-resolution clock.

The tests now use a synthetic time: This makes cookie testing
reliable in case of bogus system clocks and speeds up the
expiration tests.

R=nigeltao, alex.brainman, dave
CC=golang-dev
https://codereview.appspot.com/7323063

Committer: Nigel Tao <nigeltao@golang.org>