-
Notifications
You must be signed in to change notification settings - Fork 340
Description
expect_lt() and friends are implemented such that they accept any object that implements the required comparison operators (see #777 and d43ed65). And indeed this works, all of these remain silent:
expect_gt(9, 7)
expect_gt(as.Date("2025-01-09"), as.Date("2025-01-07"))
expect_gt(as.POSIXct("2025-01-09 00:00:00"), as.POSIXct("2025-01-07 00:00:00"))
expect_gt("9", "7")
However, when the tests fail, these expectations crash because during the preparation of the message the inputs are processed with functions that are not defined for some types (log10(), -):
expect_gt(9, 7)
expect_gt(as.Date("2025-01-07"), as.Date("2025-01-09"))
## Error in Math.Date(min(x)) : log10 not defined for "Date" objects
expect_gt(as.POSIXct("2025-01-07 00:00:00"), as.POSIXct("2025-01-09 00:00:00"))
## Error in Math.POSIXt(min(x)) : 'log10' not defined for "POSIXt" objects
expect_gt("7", "9")
## Error in act$val - exp$val : non-numeric argument to binary operator
This is not strictly a bug since the documentation reads "Do you expect a number bigger or smaller than this?" (emphasis mine), but at least the behaviour is inconsistent. As long as one only writes successful tests, one does not notice that the functions do not work reliably for some types.
The reason that this was not caught in the tests is that the existing tests are only for successful expectations.
Going back to strictly allowing only numeric values would likely break many existing tests, so I would to adapt the code that creates the messages to be more robust. I think I know how to do this and would create a PR if there are no objections.