-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix error equals #2429
Fix error equals #2429
Conversation
Signed-off-by: albertteoh <alber.teoh@logz.io>
Signed-off-by: albertteoh <alber.teoh@logz.io>
49adcac
to
40a93be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Ideally, we'd start wrapping the constant parts of the messages into the variable parts, making it easy to compare with errors.Is
. That said, your PR improves the code base and prepares it for 1.15.
{"x?service=service&start=0&end=0&operation=operation&limit=200&minDuration=20", "cannot not parse minDuration: time: missing unit in duration 20", nil}, | ||
{"x?service=service&start=0&end=0&operation=operation&limit=200&minDuration=20s&maxDuration=30", "cannot not parse maxDuration: time: missing unit in duration 30", nil}, | ||
{"x?service=service&start=0&end=0&operation=operation&limit=200&minDuration=20", `cannot not parse minDuration: time: missing unit in duration "?20"?$`, nil}, | ||
{"x?service=service&start=0&end=0&operation=operation&limit=200&minDuration=20s&maxDuration=30", `cannot not parse maxDuration: time: missing unit in duration "?30"?$`, nil}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, having been burnt by the changing stdlib error messages in the past, I now try to write tests that only check for a substring or a prefix of the error which is stable and comes from our code, such as cannot not parse minDuration
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I would agree with that. Or, alternatively, wrap the error into a custom error type and assert on the type rather than error string comparisons.
Which problem is this PR solving?
Fixes a broken test introduced by an upgrade of go version to 1.15, caused by a change in the time package, specifically in format.go:1437.
Short description of the changes
strings.HasPrefix
which is slightly simpler but slightly reduces the fidelity of the test, losing a comparison on the actual value.assert.Error(t, err)
, which is the simplest yet loses the most fidelity from the original test assertion.Tested with the following to ensure they pass: