Skip to content

Conversation

powerman
Copy link

…because non-empty error string counts as failed test by internalCheck() even if result is true.

Maybe this should be fixed in internalCheck() instead, I'm not sure. Neither this change nor alternative change in internalCheck() breaks any existing tests, so both changes are unlikely break compatibility, but proposed change have much higher chance to not break anything and makes more sense to me (the downside is losing error message, which is probably not valuable in this case anyway). So I think some new tests should be added with this change.

…because non-empty `error` string counts as failed test by `internalCheck()` even if `result` is `true`.

Maybe this should be fixed in internalCheck() instead, I'm not sure. Neither this change nor alternative change in internalCheck() breaks any existing tests, so both changes are unlikely break compatibility, but proposed change have much higher chance to not break anything and makes more sense to me (the downside is losing error message, which is probably not valuable in this case anyway). So I think some new tests should be added with this change.
@powerman
Copy link
Author

The #78 is trying to fix same issue in internalCheck(), but as mentioned above this fix is probably more safe to keep compatibility.

@niemeyer
Copy link
Contributor

Yeah, that makes no sense. I can't remember why it'd be possibly be a good idea to test the error message. I'll merge one of these two PRs, thanks for the fix.

@aronatkins
Copy link

The error string is often used to indicate a problem with the input (incorrect type, for example). Consider the HasLen checker.

c.Check([]string{}, check.Not(check.HasLen), "balloons") // errs because balloons is not numeric
c.Check(13, check.Not(check.HasLen), 42) // errs because 13 cannot have its length computed
c.Check([]string{}, check.Not(check.HasLen), 7) // test passes because 0 != 7

There is value in having HasLen continue to cause test errors due to these bad input scenarios.

Look through the standard checkers; they all appear to use the error string return argument as an error (in fact, error may have been more appropriate). Errors are not negated; checker success/failure is negated.

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

Successfully merging this pull request may close these issues.

3 participants