Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

require.Equal: Comparing time.Time after marshalling and unmarshalling is not correct #502

Closed
clebs opened this issue Sep 13, 2017 · 12 comments

Comments

@clebs
Copy link

clebs commented Sep 13, 2017

When testing sending data that contains a time.Time via JSON the sent data and received data are not equal according to require.Equal.

Sample unit test:

import (
	"encoding/json"
	"log"
	"testing"
	"time"

	"github.com/stretchr/testify/require"
)

type event struct {
	What string
	When time.Time
}

func TestIt(t *testing.T) {

	send := event{
		What: "My birthday",
		When: time.Now(),
	}

	//test marshaling and sending data
	data, err := json.Marshal(send)
	if err != nil {
		log.Fatal("Could not marshal event.")
	}

	//sending ommitted

	// unmarshal obtained data
	receive := event{}
	err = json.Unmarshal(data, &receive)

	if err != nil {
		log.Fatal("Could not unmarshal event.")
	}

	require.Equal(t, send, receive, "Comparing marshal and unmarshal of time.Time.")

}

Equal diff output:

- When: (time.Time) 2017-09-13 14:54:52.119930466 +0200 CEST m=+0.093164966
+ When: (time.Time) 2017-09-13 14:54:52.119930466 +0200 CEST

A possible solution would be having a special comparison for time.Time fields using for example Unix timestamps.

@zbindenren
Copy link

Same issue here.

@szabado
Copy link

szabado commented Nov 15, 2017

See golang/go#19502.

To summarize the link: this is how the time package works. Marshalling the time drops the monotonic clock reading, as that only has meaning within the current process. Round trip marshalling and unmarshalling of times will produce different structs.

@ernesto-jimenez
Copy link
Member

We should use go-cmp instead of reflect.DeepEqual, which would solve this and some other issues.

There's an issue to track that here: #535

Contributions more than welcome :)

@posener
Copy link
Contributor

posener commented Mar 2, 2018

Since #535 is kind of stuck, maybe we should add a special case to time.Time?

@ernesto-jimenez
Copy link
Member

@posener unfortunately, that would require implementing our own version of go-cmp, since we would have to look for it recursively.

@gomezjdaniel
Copy link

Any progress on this?

@Linesmerrill
Copy link

We just used go-cmp to do a basic comparison check. Would be nice to have this baked into the assertion library though. 😉 https://godoc.org/github.com/google/go-cmp/cmp

tetsuok added a commit to tetsuok/sourcachefs that referenced this issue Jan 2, 2020
Comparing Time values with require.Equal fails because
one of the arguments has a monotonic clock reading and
because require.Equal doesn't use time.Equal for
comparison [1].

[1] stretchr/testify#502
jmmv pushed a commit to jmmv/sourcachefs that referenced this issue Jan 5, 2020
Comparing Time values with require.Equal fails because
one of the arguments has a monotonic clock reading and
because require.Equal doesn't use time.Equal for
comparison [1].

[1] stretchr/testify#502
@leoleoasd
Copy link

Any progress on this?

@boyan-soubachov
Copy link
Collaborator

go-cmp is on the list of possible features for testify v2 (as it would be a breaking change). If there's some way to cleanly fix this problem without breaking backwards compatibility for now, I'd be happy to review and get it merged for v1.7.0. in the mean-time

@leoleoasd
Copy link

@boyan-soubachov Maybe we can use the solution of #951 on the require package as well??

@boyan-soubachov
Copy link
Collaborator

@boyan-soubachov Maybe we can use the solution of #951 on the require package as well??

The code for require is auto-generated from the assert code. Once that's merged, the fix will also be available in the require package :)

@seantcanavan
Copy link

a workaround for me was assert.Equal(t, time1.UnixMillis(), time2.UnixMillis()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants