Skip to content
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.

Sign up for GitHub

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 comparison of integers #309

Merged
merged 8 commits into from
Feb 18, 2016
Merged

Fix comparison of integers #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac3b6d8
revert to default comparison also if x is integer
Oct 11, 2015
a2830d5
NEWS
Oct 11, 2015
a9f86a8
alias compare.complex to compare.numeric
Nov 10, 2015
62563bc
better treatment of differences
Nov 10, 2015
3bec843
Merge branch 'master' into compare_integer
Nov 10, 2015
f1dadf5
update NEWS
Nov 10, 2015
823e3a4
move NEWS bullet point up
Nov 10, 2015
f2c98ad
Merge branch 'master' into compare_integer
Feb 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
S3method(as.character,expectation)
S3method(as.data.frame,testthat_results)
S3method(compare,character)
S3method(compare,complex)
S3method(compare,default)
S3method(compare,numeric)
S3method(format,expectation)
Expand Down
4 changes: 4 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# testthat 0.11.0.9000

* Comparing integers to non-numbers doesn't raise errors anymore, and falls back to
string comparison if objects have different lengths. Complex numbers are compared
using the same routine (#309, @krlmlr).

* New option `testthat.summary.max_reports` that limits the number of reports printed by the summary reporter, default: 15 (@krlmlr, #354).

* Fix failure message for `throws_error` in case where no error is raised (@nealrichardson, #342).
Expand Down
15 changes: 12 additions & 3 deletions R/compare.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,14 @@ compare.numeric <- function(x, y, ..., max_diffs = 10) {
if (isTRUE(equal)) return(comparison())

# If they're not the same type or length, fallback to default method
equal <- paste0(equal, collapse = "\n")
if (!is.integer(x) && !is.numeric(y)) return(comparison(FALSE, equal))
if (length(x) != length(y)) return(comparison(FALSE, equal))
if (length(x) != length(y)) {
return(compare(as.character(x), as.character(y)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better check would be !identical(class(x), class(y)) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently:

> testthat::expect_equal(1L, 0)
Error: 1L not equal to 0
1 - 0 == 1

I guess this case (and others) will be handled differently if comparing classes of x and y.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's probably reasonable - it doesn't really matter what the values are if the classes are different. But a bigger problem is expect_equal(1, 1L) which for convenience shouldn't fail. Maybe we should special case integer vs. numeric comparison?

This current behaviour is also bad:

expect_equal(1L, factor(1))
#  Error: 1L not equal to factor(1)
# 1 - 1 == NA In addition: Warning message:
# In Ops.factor(x, y) : ‘-’ not meaningful for factors

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this gives an uninformative message:

x <- structure(1L, class = "myclass")
expect_equal(1L, x)
#  Error: 1L not equal to x
# 1 - 1 == 0

}

if (!all(c(class(x), class(y)) %in% c("integer", "numeric", "complex"))) {
equal <- paste0(equal, collapse = "\n")
return(comparison(FALSE, equal))
}

if (length(x) == 1) {
msg <- paste0(format(x, digits = 3), " - ", format(y, digits = 3),
Expand Down Expand Up @@ -177,6 +182,10 @@ compare.numeric <- function(x, y, ..., max_diffs = 10) {
comparison(FALSE, msg)
}

#' @export
#' @rdname compare
compare.complex <- compare.numeric

print_out <- function(x, ...) {
lines <- utils::capture.output(print(x, ...))
paste0(lines, collapse = "\n")
Expand Down