|
|
Descriptiontesting: fix timing format inconsistency
Fixes issue 8175.
Patch Set 1 #Patch Set 2 : diff -r 6146799f32ed https://code.google.com/p/go #Patch Set 3 : diff -r 6146799f32ed https://code.google.com/p/go #
Total comments: 2
Patch Set 4 : diff -r 1282e1d59a81 https://code.google.com/p/go #Patch Set 5 : diff -r 951cc5f6d52f https://code.google.com/p/go #Patch Set 6 : diff -r 951cc5f6d52f https://code.google.com/p/go #
Total comments: 1
Patch Set 7 : diff -r 528a025b902b https://code.google.com/p/go #
Total comments: 2
MessagesTotal messages: 18
Hello golang-codereviews@googlegroups.com (cc: r@golang.org), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/103320043/diff/40001/src/pkg/testing/example.go File src/pkg/testing/example.go (right): https://codereview.appspot.com/103320043/diff/40001/src/pkg/testing/example.g... src/pkg/testing/example.go:74: d := fmt.Sprintf("%.2f seconds", time.Now().Sub(start).Seconds()) I'd prefer you did this the other way around, so the output is more concise rather than more verbose.
Sign in to reply to this message.
https://codereview.appspot.com/103320043/diff/40001/src/pkg/testing/example.go File src/pkg/testing/example.go (right): https://codereview.appspot.com/103320043/diff/40001/src/pkg/testing/example.g... src/pkg/testing/example.go:74: d := fmt.Sprintf("%.2f seconds", time.Now().Sub(start).Seconds()) In that case I will change the format for tests and leave this one as is. Also, “func (d Duration) String() string” needs to be changed to address the “us”/microseconds formatting; I will submit that one in a different CL. On 2014/06/11 15:16:06, r wrote: > I'd prefer you did this the other way around, so the output is more concise > rather than more verbose.
Sign in to reply to this message.
I already have a CL outstanding for the formatting of microseconds, although it needs some discussion. https://codereview.appspot.com/105030046 On Wed, Jun 11, 2014 at 9:14 AM, <r@varp.se> wrote: > > https://codereview.appspot.com/103320043/diff/40001/src/pkg/testing/example.go > File src/pkg/testing/example.go (right): > > https://codereview.appspot.com/103320043/diff/40001/src/pkg/testing/example.g... > src/pkg/testing/example.go:74: d := fmt.Sprintf("%.2f seconds", > time.Now().Sub(start).Seconds()) > In that case I will change the format for tests and leave this one as > is. > > Also, "func (d Duration) String() string" needs to be changed to address > the "us"/microseconds formatting; I will submit that one in a different > CL. > > > On 2014/06/11 15:16:06, r wrote: >> >> I'd prefer you did this the other way around, so the output is more > > concise >> >> rather than more verbose. > > > https://codereview.appspot.com/103320043/
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, r@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Do you think that we should format to some precision, or does this look reasonable? === RUN TestEmpty --- PASS: TestEmpty (826ns) === RUN TestNanosecond --- PASS: TestNanosecond (13.796us) === RUN TestMicrosecond --- PASS: TestMicrosecond (1.792377ms) === RUN TestMillisecond --- PASS: TestMillisecond (2.271155ms) === RUN TestSecond --- PASS: TestSecond (1.002801201s) === RUN: ExampleEmpty --- PASS: ExampleEmpty (978ns) === RUN: ExampleNanosecond --- PASS: ExampleNanosecond (1.932562ms) === RUN: ExampleMicroSecond --- PASS: ExampleMicroSecond (2.343049ms) === RUN: ExampleMillisecond --- PASS: ExampleMillisecond (1.826363ms) === RUN: ExampleSecond --- PASS: ExampleSecond (1.002185473s)
Sign in to reply to this message.
I think the old, two-decimal-point format for the tests was correct, and the right fix is to apply the identical logic (preferably with some code reuse) for the examples. The user doesn't or at least shouldn't care about the timing of these things in more detail than that provides: Benchmarks are there for when it actually matters.
Sign in to reply to this message.
On 2014/06/12 15:37:56, r wrote: > I think the old, two-decimal-point format for the tests was correct, and the > right fix is to apply the identical logic (preferably with some code reuse) for > the examples. The user doesn't or at least shouldn't care about the timing of > these things in more detail than that provides: Benchmarks are there for when it > actually matters. These are exactly my thoughts. Output of Patch Set 5 looks like this: === RUN TestEmpty --- PASS: TestEmpty (0.00s) === RUN TestNanosecond --- PASS: TestNanosecond (0.00s) === RUN TestMicrosecond --- PASS: TestMicrosecond (0.00s) === RUN TestMillisecond --- PASS: TestMillisecond (0.00s) === RUN TestSecond --- PASS: TestSecond (1.00s) === RUN: ExampleEmpty --- PASS: ExampleEmpty (0.00s) === RUN: ExampleNanosecond --- PASS: ExampleNanosecond (0.00s) === RUN: ExampleMicroSecond --- PASS: ExampleMicroSecond (0.00s) === RUN: ExampleMillisecond --- PASS: ExampleMillisecond (0.00s) === RUN: ExampleSecond --- PASS: ExampleSecond (1.00s) PASS ok github.com/untilted/i8175 2.020s I am a bit hesitant on using the format above. My reasoning is that since on the first sight it looks very similar to the representation produced by time.Duration (except for the decimal places), the user could be expecting it to behave the same way as that one (scale based on the figure), and in fact it does not. If we use "seconds" instead of "s" we end up with the following report: === RUN TestEmpty --- PASS: TestEmpty (0.00 seconds) === RUN TestNanosecond --- PASS: TestNanosecond (0.00 seconds) === RUN TestMicrosecond --- PASS: TestMicrosecond (0.00 seconds) === RUN TestMillisecond --- PASS: TestMillisecond (0.00 seconds) === RUN TestSecond --- PASS: TestSecond (1.00 seconds) === RUN: ExampleEmpty --- PASS: ExampleEmpty (0.00 seconds) === RUN: ExampleNanosecond --- PASS: ExampleNanosecond (0.00 seconds) === RUN: ExampleMicroSecond --- PASS: ExampleMicroSecond (0.00 seconds) === RUN: ExampleMillisecond --- PASS: ExampleMillisecond (0.00 seconds) === RUN: ExampleSecond --- PASS: ExampleSecond (1.00 seconds) PASS ok github.com/untilted/i8175 2.020s
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, r@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/103320043/diff/150004/src/pkg/testing/testing.go File src/pkg/testing/testing.go (right): https://codereview.appspot.com/103320043/diff/150004/src/pkg/testing/testing.... src/pkg/testing/testing.go:463: fmt.Printf(format, "PASS", t.name, t.duration, t.output) if you just had a function that returned a time.Duration formatted consistently, you could avoid the special type and confusing duplication of functionality. simple is good.
Sign in to reply to this message.
R=close (assigned by r@golang.org)
Sign in to reply to this message.
R=r not sure why this was closed
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, r@golang.org, gobot@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/103320043/diff/210001/src/pkg/testing/testing.go File src/pkg/testing/testing.go (right): https://codereview.appspot.com/103320043/diff/210001/src/pkg/testing/testing.... src/pkg/testing/testing.go:228: return fmt.Sprintf("%.2fs", d.Seconds()) Please note that "%.2fs" is not identical the legacy format. In order for it to be identical it should be "%.2f seconds". Do let me know if the change breaks any contracts and must to be kept identical with legacy. I chose to do it this way for conciseness.
Sign in to reply to this message.
Patch Set 7 Output: === RUN TestEmpty --- PASS: TestEmpty (0.00s) === RUN TestNanosecond --- PASS: TestNanosecond (0.00s) === RUN TestMicrosecond --- PASS: TestMicrosecond (0.00s) === RUN TestMillisecond --- PASS: TestMillisecond (0.00s) === RUN TestSecond --- PASS: TestSecond (1.00s) === RUN Test87Seconds --- PASS: Test87Seconds (87.00s) === RUN: ExampleEmpty --- PASS: ExampleEmpty (0.00s) === RUN: ExampleNanosecond --- PASS: ExampleNanosecond (0.00s) === RUN: ExampleMicroSecond --- PASS: ExampleMicroSecond (0.00s) === RUN: ExampleMillisecond --- PASS: ExampleMillisecond (0.00s) === RUN: ExampleSecond --- PASS: ExampleSecond (1.00s) PASS ok github.com/untilted/i8175 89.016s
Sign in to reply to this message.
LGTM https://codereview.appspot.com/103320043/diff/210001/src/pkg/testing/testing.go File src/pkg/testing/testing.go (right): https://codereview.appspot.com/103320043/diff/210001/src/pkg/testing/testing.... src/pkg/testing/testing.go:228: return fmt.Sprintf("%.2fs", d.Seconds()) On 2014/06/17 07:41:31, rd wrote: > Please note that "%.2fs" is not identical the legacy format. In order for it to > be identical it should be "%.2f seconds". Do let me know if the change breaks > any contracts and must to be kept identical with legacy. I chose to do it this > way for conciseness. This is what I was asking for. Thanks.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=f3a39473db21 *** testing: fix timing format inconsistency Fixes issue 8175. LGTM=r R=golang-codereviews, r, gobot CC=golang-codereviews https://codereview.appspot.com/103320043 Committer: Rob Pike <r@golang.org>
Sign in to reply to this message.
This CL appears to have broken the nacl-amd64p32 builder. See http://build.golang.org/log/ab67ec29637297bfc0ca67e02baf5500c249f3bc
Sign in to reply to this message.
|