test-spatial.R errors if sf adds attributes to sf objects

[mikemahoney218]

Hi there!

As currently written, test-spatial.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:

tidytransit/tests/testthat/test-spatial.R

Lines 94 to 99 in 878e64a

	duke_00 = gtfs_duke
	duke_sf = gtfs_as_sf(duke_00, crs = 3358)
	duke_df = sf_as_tbl(duke_sf)
	attributes(duke_00$shapes)$.internal.selfref <- NULL
	expect_equal(duke_df$stops[colnames(gtfs_duke$stops)], gtfs_duke$stops, tolerance = 0.0001)

tidytransit/tests/testthat/test-spatial.R

Lines 147 to 158 in 878e64a

	g_nyc = read_gtfs(system.file("extdata", "google_transit_nyc_subway.zip", package = "tidytransit"))
	test_that("stop_group_distances real feed", {
	x1 = stop_group_distances(g_nyc$stops)
	g_nyc_sf = gtfs_as_sf(g_nyc)
	x2 = stop_group_distances(g_nyc_sf$stops, "stop_name")
	expect_equal(colnames(x1), colnames(x2))
	expect_equal(x1$stop_name, x2$stop_name)
	expect_equal(x1[,c("n_stop_ids", "dist_mean", "dist_median", "dist_max")],
	x2[,c("n_stop_ids", "dist_mean", "dist_median", "dist_max")])

The issue is that by using expect_equal(), testthat is partially testing if your objects contain exactly the same attributes. 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.

Specifically, if I understand these tests correctly, I think you're testing to make sure that column values are equivalent after data transformations. Would you consider looping through to compare each of the columns, rather than comparing the data structure as a whole? The first test could be:

for (col in colnames(gtfs_duke$stops)) {
    expect_equal(duke_df$stops[[col]], gtfs_duke$stops[[col]], tolerance = 0.0001)
  }

And the second test:

for (col in c("n_stop_ids", "dist_mean", "dist_median", "dist_max")) {
    expect_equal(x1[[col]], x2[[col]])
  }

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

[mikemahoney218]

Another approach would be to set check.attributes = FALSE:

expect_equal(duke_df$stops[colnames(gtfs_duke$stops)], gtfs_duke$stops, tolerance = 0.0001, check.attributes = FALSE)

expect_equal(x1[,c("n_stop_ids", "dist_mean", "dist_median", "dist_max")],  
               x2[,c("n_stop_ids", "dist_mean", "dist_median", "dist_max")],
               check.attributes = FALSE) 

[polettif]

Thanks for the heads up and the code to fix the issue.

I used check.attributes for the first test and removed attributes(duke_00$shapes)$.internal.selfref <- NULL as well. That already pointed to something not quite clean. Comparing values for the second test looks good to me.

[mikemahoney218]

Thanks!