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.

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

testthat::expect_equal() returns spurious pass result when comparing Message objects #53

Closed
jarodmeng opened this issue Nov 26, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@jarodmeng
Copy link
Contributor

commented Nov 26, 2018

testthat::expect_equal passes even when two Message objects have totally different content (see reprex below).

expect_equal() calls the generic all.equal() to compare two objects. Instead of dispatching on the S4 method defined in RProtoBuf, all.equal() (when called from testthat) actually dispatch to the default all.equal.default() which in turn uses attr.all.equal() to only compare the attributes of the two objects.

This is likely because RProtoBuf doesn't define an S3method for all.equal as the manual suggests. A simple addition to the NAMESPACE file should fix the issue. Happy to send in a PR on this if necessary.

library(RProtoBuf)
library(testthat)

# Create two obviously different messages from the same descriptor
p <- new(tutorial.Person, name = "John", id = 1)
p2 <- new(tutorial.Person, name = "Doe", id = 2)

# Directly calling all.equal() returns the correct result
all.equal(p, p2)
#> [1] FALSE

# Calling all.equal.default() returns the wrong result
all.equal.default(p, p2)
#> [1] TRUE

# testthat::expect_equal() is calling the wrong method and returns a spurious
# pass
expect_equal(p, p2)

Created on 2018-11-26 by the reprex package (v0.2.1)

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 26, 2018

Interesting. I will note that our use of RUnit has no such issue.

To be precise, you are suggesting a one-liner addition to NAMESPACE defining an S3method as we currently do for a few others? If you could fork, add this and demonstrate that this (likely error in testthat ?) then goes away then we can add it.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 26, 2018

Odd. RUnit also gets the wrong result:

R> library(RUnit)
R> checkEquals(p, p2)
[1] TRUE
R>

but that uses a direct comparison. If we are explicit this works

R> checkTrue( FALSE == all.equal(p, p2) )
[1] TRUE
R> 
@jarodmeng

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

I've created #54 to fix this.

Here's the result from the same reprex after the fix.

library(RProtoBuf)
library(testthat)

# Create two obviously different messages from the same descriptor
p <- new(tutorial.Person, name = "John", id = 1)
p2 <- new(tutorial.Person, name = "Doe", id = 2)

# Directly calling all.equal() returns the correct result
all.equal(p, p2)
#> [1] FALSE

# Calling all.equal.default() returns the wrong result
all.equal.default(p, p2)
#> [1] TRUE

# testthat::expect_equal() now correctly fails the test
expect_equal(p, p2)
#> Error: `p` not equal to `p2`.
#> FALSE

Created on 2018-11-26 by the reprex package (v0.2.1)

@jarodmeng

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

Added an RUnit test in the reprex.

library(RProtoBuf)
library(testthat)
library(RUnit)

# Create two obviously different messages from the same descriptor
p <- new(tutorial.Person, name = "John", id = 1)
p2 <- new(tutorial.Person, name = "Doe", id = 2)

# Directly calling all.equal() returns the correct result
all.equal(p, p2)
#> [1] FALSE

# Calling all.equal.default() returns the wrong result
all.equal.default(p, p2)
#> [1] TRUE

# testthat::expect_equal() now correctly fails the test
expect_equal(p, p2)
#> Error: `p` not equal to `p2`.
#> FALSE

# RUnit::checkEquals now returns the correct result too
checkEquals(p, p2)
#> Error in checkEquals(p, p2): FALSE

Created on 2018-11-26 by the reprex package (v0.2.1)

@jarodmeng

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

I think the root cause of this lies in RProtoBuf rather than testthat.

testthat::expect_equal() calls testthat::compare() which in turn calls the all.equal() generic. RProtoBuf needs to make the S3 dispatch available for all.equal() generic to see. Currently since all.equal() can't see the method, it dispatches to all.equal.default() which only compares S4 objects' attributes rather than their contents.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 26, 2018

Very helpful follow-ups, thanks. In all those of using RUnit I did not have to follow that chain. But what you reason there seems legit -- a generic is missing, .default gets called, madness ensues. And it is in fact symmetric between both test runners which is a good reason for a change.

Thanks for all that, will follow-up.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 27, 2018

Added ChangeLog and NEWS in 6298c7c -- thanks again for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.