zclconf / go-cty

A type system for dynamic values in Go applications
MIT License
338 stars 70 forks source link

TestFormatDate fails with upcoming Go 1.20 release #147

Closed ianlancetaylor closed 5 months ago

ianlancetaylor commented 1 year ago

Hi, when I run go test in the cty/function/stdlib directory using the current sources at HEAD of Go, which will be the future Go 1.20 release, I get

--- FAIL: TestFormatDate (0.00s)
    --- FAIL: TestFormatDate/2-12-02T00:00:00Z_parse_error (0.00s)
        datetime_test.go:225: wrong error
            got:  not a valid RFC3339 timestamp: cannot use "2-12-02T00:00:00Z" as year
            want: not a valid RFC3339 timestamp: cannot use "-02T00:00:00Z" as year
FAIL
FAIL    github.com/zclconf/go-cty/cty/function/stdlib   0.018s
FAIL

The test is expecting an exact match for the error string, which is generally not a good idea. The Go standard library doesn't promise that error strings will match exactly from release to release.

In this case the go-cty source code even says

            // Go parser seems to be trying to parse "2-12" as a year here,
            // producing a confusing error message.

The error message has gotten better, and now the test fails.

apparentlymart commented 1 year ago

Thanks for letting me know about this, @ianlancetaylor.

I'm general I try to avoid directly exposing Go standard library error messages in this library's interface because the errors here often end up being in the UI of calling applications where the user isn't expected to be aware that Go is involved, and those applications depend on these error messages being of suitable quality for use in that context.

Reviewing this code it looks like this is not directly returning a Go library error message but it is reinterpreting a time.ParseError into a derived message in a way that depends on how the time.Parse function classified each invalid token. Therefore although it's not depending on the error string exactly, it is depending on the data that the error string would have been built from, and so has improved in the same way that the underlying Go library error has.

(If I recall correctly, the exact string the time.ParseError type was producing was something like "cannot use 2-12-02T00:00:00Z as 2016", which is understandable only if you are familiar with the example timestamp the time package uses, and hence I wrote this function which "knows" that example timestamp and tries to translate the error into something more intelligible.)

I'll ponder how best to solve this after the US Thanksgiving holiday. I recall that there is now an RFC3339-specific parser in the time.Time UnmarshalText implementation, and it probably makes sense to use that instead here anyway since the intent is to only support RFC3339 as input, but I may need to copy it into this codebase for now to avoid imposing a Go 1.20 requirement immediately. Doing that would also have the advantage of freezing the error handling in here so that it won't be affected by future stdlib changes.

aktau commented 1 year ago

Ping. Any update?

aktau commented 1 year ago

Ping.

aktau commented 11 months ago

Go 1.20 was released (https://go.dev/doc/devel/release), and tests are now failing at HEAD.

ianlancetaylor commented 5 months ago

This was fixed by revision e24a128788f22ca66dde8273b4e0f01061a6ada0.