decimal: impement Sum and Prod functions #54
Conversation
| func(t *testing.T, dneg bool, dscale int, dcoef uint64, eneg bool, escale int, ecoef uint64) { | ||
| d, err := newSafe(dneg, fint(dcoef), dscale) | ||
| if err != nil { | ||
| t.Skip() |
There was a problem hiding this comment.
Why skipping?
Add a comment if it's normal, otherwise please fix it
It it's something you consider as a prerequisites condition before testing something else, it should fail
| } | ||
| e, err := newSafe(eneg, fint(ecoef), escale) | ||
| if err != nil { | ||
| t.Skip() |
|
|
||
| got, err := d.Mul(e) | ||
| if err != nil { | ||
| t.Skip() |
|
|
||
| got, err := d.Add(e) | ||
| if err != nil { | ||
| t.Skip() |
| } | ||
| e, err := newSafe(eneg, fint(ecoef), escale) | ||
| if err != nil { | ||
| t.Skip() |
| func(t *testing.T, dneg bool, dscale int, dcoef uint64, eneg bool, escale int, ecoef uint64) { | ||
| d, err := newSafe(dneg, fint(dcoef), dscale) | ||
| if err != nil { | ||
| t.Skip() |
|
@ccoVeille, there could be two situations:
The case of false positive is relatively safe. There is logging and monitoring in place to detect such cases. Eventually, the function will be fixed and unit tests will be added to prevent this from happening in the future. The case of false negative is much more dangerous. It can lead to data corruption, financial loss, etc. And this is what tested by fuzz tests. If function returns an error we don't care about it, but if it returns some result then this result MUST satisfy some property: be equal to the result of the reference implementation, be in some range, etc. |
|
Thanks for your reply. Your explanation is good,it makes sense Maybe you could add a method to call skip, then document the reason you do that something like // fuzzSkipError blah blah
func(t *testing.T) fuzzSkipError(err error) {
t.Helper()
t.Skipf("skipping error in fuzzy mode: %s", err)
} |
| @@ -3697,7 +3866,7 @@ func TestDecimal_Min(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| //nolint:predeclared | |||
| //nolint:revive | |||
There was a problem hiding this comment.
You are disabling a whole linter here. You should add a comment.
You should consider adding nolintlint linter and add require-specific and require-explanation
https://golangci-lint.run/usage/linters/#nolintlint
Also, does this directive have to be applied to the whole function? Can't it be move somewhat only where it's needed?
No description provided.