make expect_lt() etc. work properly for non-numeric data

[stibu81]

This is an attempt to make expect_lt(), expect_lte(), expect_gt(), and expect_gte() work properly for non-numeric data by ensuring that the failure messages are created correctly also for those. It fixes #2268.

With these changes, the examples mentioned in #2268 lead to the following output:

expect_gt(7, 9)
## Error: Expected 7 > 9.
## Actual comparison: 7.0 <= 9.0
## Difference: -2.0 <= 0

expect_gt(as.Date("2025-01-07"), as.Date("2025-01-09"))
## Error: Expected `as.Date("2025-01-07")` > `as.Date("2025-01-09")`.
## Actual comparison: "2025-01-07" <= "2025-01-09"

expect_gt(as.POSIXct("2025-01-07 01:00:00"), as.POSIXct("2025-01-09 01:00:00"))
## Error: Expected `as.POSIXct("2025-01-07 01:00:00")` > `as.POSIXct("2025-01-09 01:00:00")`.
## Actual comparison: "2025-01-07 01:00:00" <= "2025-01-09 01:00:00"

expect_gt("7", "9")
## Error: Expected "7" > "9".
## Actual comparison: "7" <= "9"

I decided to differentiate between numeric inputs and all others. For numeric inputs, nothing changes, but for anything else, the messages are created differently. I made two decisions that others might disagree with and that I could change if requested:

• I added quotation marks around the values in the line "Actual comparison" to make sure that the output never looks like numbers.
• I don't output the line "Difference". This line could be output for dates and times, but it would need a little more preparation to make the output meaningful. I'm not sure that this is helpful, but of course I could make the change if you think it makes sense. I would format te output for dates and times similarly to as it is done in the print-method for difftime.

[hadley]

Thanks for discovering this problem and proposing a fix! I do think it would be useful to include a comparison for dates & date-times, especially since those data types often don't print small decimals. I suspect the default difftime() output should be adequate.

[hadley]

This level of tests feels a bit heavy to me, I think because you're just repeatedly testing the same bit of code:

   msg_act <- sprintf(
      "Actual comparison: \"%s\" %s \"%s\"",
      act$val,
      actual_op,
      exp$val
    )
    msg_diff <- NULL

I'd suggest testing just one of the four expectations for non-numeric inputs, something like this:

test_that("informative failure for non-numeric inputs",  {
  char1 <- "x"
  chat2 <- "y"
  expect_snapshot_failure(expect_gt(x1, x2))

   date1 <- ...
}

You might also consider refactoring the function a bit so you could just do snapshot tests of failure_compare("x", "y", ">") rather than having to do the complete test. To do that you'd need generate msg_exp in expect_compare_(), then `failure_compare() could just take the values, rather than labelled values.

[stibu81]

Thanks for your inputs. This is my second attempt: I also output the difference for dates and times now and reduced the number of tests.

In the meantime I found another problem in these expectations: the error messages fail for negative numbers:

expect_gt(-3, -2)
## Error in if (scale <= 0) { : missing value where TRUE/FALSE needed
## In addition: Warning message:
## In digits(act$val) : NaNs produced

This can be fixed easily by taking the absolute value in digits(). How do you prefer this to be handled: should I just fix it (and add some tests), fix it in a new PR or even create an issue and a PR?

[hadley]

Just fix it please 😄

[hadley]

Thanks for working on this! Much appreciated 😄