`test-riverdist.R` errors if sf adds attributes to sf objects

[mikemahoney218]

Hi there!

As currently written, test-riverdist.R is partially testing that the underlying sf object never changes. Specifically, these two tests (the last line of the code blocks) will throw errors if sf ever changes the attributes attached to sf objects:

https://github.com/mbtyers/riverdist/blob/138eb17a353e767445faf9cc58a74daf739c78d2/tests/testthat/test_riverdist.R#L141-L147

https://github.com/mbtyers/riverdist/blob/138eb17a353e767445faf9cc58a74daf739c78d2/tests/testthat/test_riverdist.R#L289-L296

The issue is that by using expect_equal(), testthat is partially testing if your objects contain exactly the same attributes. Because you're comparing against old saved sf objects, that means you're partially testing internal implementation details of the sf object that aren't guaranteed to remain the same over time. There's a bit more about how testing the attributes of objects created by other packages can be a problem on the Tidyverse blog.

At the moment, we're trying to add a new attribute to sf objects, which causes these tests to error. I'm writing to ask if you'd consider changing these tests to allow sf to improve the internal structure of sf objects.

I'll confess that I don't exactly understand what these tests are looking for. If they're trying to ensure that the new sf_current field is the same as it has been in the past, would you consider comparing the geometry of that field directly instead? For example:

expect_equal(sf::st_geometry(dissolve(Kenai2)$sf_current),sf::st_geometry(Kenai3$sf_current))

expect_equal(sf::st_geometry(Koyukuk1a$sf_current),sf::st_geometry(Koyukuk2$sf_current))

This change would mean this package is no longer blocking sf from making changes to the internal structure of sf objects, and would fix the immediate issue with the new attribute.

If you're open to it, I'd be happy to send you a PR to make these changes!

[mikemahoney218]

Another possibility, though perhaps slightly less explicit, would be to set check.attributes to FALSE:

expect_equal(dissolve(Kenai2),Kenai3, check.attributes = FALSE)

expect_equal(Koyukuk1a,Koyukuk2, check.attributes = FALSE)

[mbtyers]

Hi Mike,

Thanks for flagging this issue and for providing a solution!! I ended up using check.attributes=FALSE in both tests. Pushed to Github a moment ago, and will resubmit to CRAN soon.

[mikemahoney218]

Thank you!