code review 42440043: time: add tests for Duration.Nanoseconds, Duration.Minu...

time: add tests for Duration.Nanoseconds, Duration.Minutes, and Duration.Hours

[shawnps, 2013-12-15 04:01:39]

Hello golang-dev@googlegroups.com,

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

[rsc, 2013-12-18 18:14:18]

in the cl description s/time_test/time/
(it's the directory, not the package)

https://codereview.appspot.com/42440043/diff/30001/src/pkg/time/time_test.go
File src/pkg/time/time_test.go (right):

https://codereview.appspot.com/42440043/diff/30001/src/pkg/time/time_test.go#...
src/pkg/time/time_test.go:1482: Duration(-1):           -1.6666666666666667e-11,
is this just -1 / 60e9?  if so, write that.
same for next line

https://codereview.appspot.com/42440043/diff/30001/src/pkg/time/time_test.go#...
src/pkg/time/time_test.go:1498: Duration(-1):            
-2.777777777777778e-13,
-1/3600e9

[shawnps, 2013-12-18 22:04:43]

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

Please take another look.

[gobot, 2013-12-20 16:26:10]

Replacing golang-dev with golang-codereviews.

[shawnps, 2013-12-28 09:37:13]

https://codereview.appspot.com/42440043/diff/30001/src/pkg/time/time_test.go
File src/pkg/time/time_test.go (right):

https://codereview.appspot.com/42440043/diff/30001/src/pkg/time/time_test.go#...
src/pkg/time/time_test.go:1482: Duration(-1):           -1.6666666666666667e-11,
On 2013/12/18 18:14:18, rsc wrote:
> is this just -1 / 60e9?  if so, write that.
> same for next line

Done.

https://codereview.appspot.com/42440043/diff/30001/src/pkg/time/time_test.go#...
src/pkg/time/time_test.go:1498: Duration(-1):            
-2.777777777777778e-13,
On 2013/12/18 18:14:18, rsc wrote:
> -1/3600e9

Done.

[dave_cheney.net, 2013-12-29 11:33:14]

On 2013/12/28 09:37:13, shawnps wrote:
> https://codereview.appspot.com/42440043/diff/30001/src/pkg/time/time_test.go
> File src/pkg/time/time_test.go (right):
> 
>
https://codereview.appspot.com/42440043/diff/30001/src/pkg/time/time_test.go#...
> src/pkg/time/time_test.go:1482: Duration(-1):          
-1.6666666666666667e-11,
> On 2013/12/18 18:14:18, rsc wrote:
> > is this just -1 / 60e9?  if so, write that.
> > same for next line
> 
> Done.
> 
>
https://codereview.appspot.com/42440043/diff/30001/src/pkg/time/time_test.go#...
> src/pkg/time/time_test.go:1498: Duration(-1):            
> -2.777777777777778e-13,
> On 2013/12/18 18:14:18, rsc wrote:
> > -1/3600e9
> 
> Done.

LGTM. Thank you. I will submit this tomorrow unless there are further comments

Before:
time/time.go:           Nanoseconds             0.0%
time/time.go:           Seconds                 100.0%
time/time.go:           Minutes                 0.0%
time/time.go:           Hours                   0.0%
total:                  (statements)            89.3%

After:
time/time.go:           Nanoseconds             100.0%
time/time.go:           Seconds                 100.0%
time/time.go:           Minutes                 100.0%
time/time.go:           Hours                   100.0%
total:                  (statements)            89.9%

[dave_cheney.net, 2014-01-02 09:07:59]

Thank you. Looking at the other test's in this file, although they are a mixture
of styles, it's probably worth using the current accepted style.

https://codereview.appspot.com/42440043/diff/70001/src/pkg/time/time_test.go
File src/pkg/time/time_test.go (right):

https://codereview.appspot.com/42440043/diff/70001/src/pkg/time/time_test.go#...
src/pkg/time/time_test.go:1475: t.Errorf("d.Nanoseconds(): want: %d, have: %d",
i, ns)
could you please update this to the modern, ():= %d; want %d" form for these new
tests.

[shawnps, 2014-01-02 09:23:49]

Dave, I've changed a couple of other things, like using []struct instead of
maps, and renaming the test values to got and want.

Not sure if you meant to keep the ":" in "():", so I removed it, but let me know
if you meant to keep it there.

https://codereview.appspot.com/42440043/diff/70001/src/pkg/time/time_test.go
File src/pkg/time/time_test.go (right):

https://codereview.appspot.com/42440043/diff/70001/src/pkg/time/time_test.go#...
src/pkg/time/time_test.go:1475: t.Errorf("d.Nanoseconds(): want: %d, have: %d",
i, ns)
On 2014/01/02 09:08:00, dfc wrote:
> could you please update this to the modern, ():= %d; want %d" form for these
new
> tests.

Done.

[shawnps, 2014-01-02 09:24:36]

Hello golang-codereviews@googlegroups.com, rsc@golang.org, dave@cheney.net (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[dave_cheney.net, 2014-01-02 10:00:59]

On 2014/01/02 09:24:36, shawnps wrote:
> Hello mailto:golang-codereviews@googlegroups.com, mailto:rsc@golang.org,
mailto:dave@cheney.net (cc:
> mailto:golang-codereviews@googlegroups.com),
> 
> Please take another look.

LGTM. Submitting.

[dave_cheney.net, 2014-01-02 10:06:44]

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

time: add tests for Duration.Nanoseconds, Duration.Minutes, and Duration.Hours

R=golang-codereviews, rsc, dave
CC=golang-codereviews
https://codereview.appspot.com/42440043

Committer: Dave Cheney <dave@cheney.net>