Allow comparing zero for DurationWithThreshold

[ha1f]

I found DurationWithThreshold returns false if both of the values are zero.

https://github.com/gotestyourself/gotest.tools/pull/62/files#diff-d952cf5be65334da619358823c8ff4669961b0d9a30e85d542caad125beac4c6R20

I guess if both of them are zero, we should return true to compare all test cases in the consistent way.

What do you think?

[dnephin]

Thank you for raising this issue!

DurationWithThreshold returns false in this case because a 0 duration is very different from even the shortest duration of 1 nanosecond. If a duration is created using time.Since or time.Until, it will always be non-zero. If the actual value is zero then the code did not actually run, and the test should probably fail.

The way this is expected to be used is:

1. If you expect a duration of 0 then do not use DurationWithThreshold, always expect 0
2. If you expect a duration that could be very small, use DurationWithThreshold and a non-zero expected value (like time.Nanosecond) . You can be confident that there was at least a non-zero duration.

If it returned true in that case then some tests could pass when they should have failed.

Does that seems reasonable to you? Do you have a use case that doesn't work?

At the very least I think this detail should be included in the godoc for this function. So I'll leave this issue open to track adding to the godoc comment.

[ha1f]

Thank you for the response and consideration about the docs!

I'm wondering about the following [empty] test case.
I feel it weird to see - Duration: 0, + Duration: 0, violation.

package main

import (
	"fmt"
	"time"

	"github.com/google/go-cmp/cmp"
	"gotest.tools/v3/assert/opt"
)

type ObjectWithDuration struct {
	Duration time.Duration
}

func main() {
	testCases := map[string]struct {
		lhs *ObjectWithDuration
		rhs *ObjectWithDuration
	}{
		"1 nanoseconds": {
			lhs: &ObjectWithDuration{
				Duration: 1 * time.Nanosecond,
			},
			rhs: &ObjectWithDuration{
				Duration: 1 * time.Nanosecond,
			},
		},
		"empty": {
			lhs: &ObjectWithDuration{},
			rhs: &ObjectWithDuration{},
		},
	}
	opts := []cmp.Option{
		opt.DurationWithThreshold(1 * time.Millisecond),
	}
	for n, tc := range testCases {
                 // we need to skip comparison if tc.lhs == 0 && tc.rhs == 0
		if diff := cmp.Diff(tc.lhs, tc.rhs, opts...); diff != "" {
			fmt.Printf("[%v] got diff; %v\n", n, diff)
		} else {
			fmt.Printf("[%v] no diffs\n", n)
		}
	}
}

[1 nanoseconds] no diffs
[empty] got diff;   &main.ObjectWithDuration{
- 	Duration: 0,
+ 	Duration: 0,
  }

We need to check if both of the values are 0 before passing to gocmp to avoid the zero comparison, but it's a little bit weird to me.

[dnephin]

That is an interesting example. In this example you wouldn't ever need to use opt.DurationWithThreshold, because you always know exactly what the Duration value will be.

Generally opt.DurationWithThreshold is used because the time.Duration value is created by measuring some elapsed time of a running program. Since you don't know how long it will run, there needs to be some allowed variance to the comparison.

Can you help me understand the real world use case? Are you testing code that may or may not run, so the duration is either 0 or non-zero ?

One option for handling this would be to add another field to the test case struct:;

struct {
	lhs *ObjectWithDuration
	rhs *ObjectWithDuration
	cmpOpts []cmp.Option
}

Then you could use the tc.cmpOpts in all cases, and you don't need to inspect lhs or rhs.