chris-prener / qualmap Goto Github PK

Tasks:

1. Set-up CI build on GitHub actions
2. Address build errors
3. Add pkgdown GitHub action for site build
4. README.md and repo housekeeping (remove old CI files, update badges)
5. Ensure code coverage is working correctly

Hi there!

As currently written, test_4_qm_create.R and test_5_qm_combine.R are partially testing that the underlying sf object never changes. Specifically, these tests will throw errors if sf ever changes the attributes attached to sf objects:

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 think these tests are trying to make sure that the resulting data frame is stable, and the columns are identical. Would it be possible to pass the check.attributes = FALSE argument, so that they won't error if sf adds attributes?

resultV1 <- qm_create(ref = test_sf, key = "TRACTCE", value = test_cluster, rid = 1, cid = 1, category = "positive")
resultV2 <- qm_create(ref = test_sf, key = TRACTCE, value = test_cluster, rid = 1, cid = 1, category = "positive")

test_that("returns TRUE - test result 1 matches test_tbl2", {
  expect_equal(resultV1, test_tbl2, check.attributes = FALSE)
})

test_that("returns TRUE - test result 2 matches test_tbl2", {
  expect_equal(resultV2, test_tbl2, check.attributes = FALSE)
})

resultV3 <- qm_create(ref = test_sf, key = "TRACTCE", value = test_cluster, rid = 1, cid = 1, category = "positive", NAME)

test_that("returns TRUE - test result 3 matches test_tbl3", {
  expect_equal(resultV3, test_tbl3, check.attributes = FALSE)
})

clustersV1 <- qm_combine(cluster1_obj, cluster2_obj, cluster3_obj)

test_that("returns TRUE - test result 1 matches test_tbl2", {
  expect_equal(clustersV1, test_obj2, check.attributes = FALSE)
})

clustersV2 <- qm_combine(cluster4b_obj, cluster5_obj, cluster6_obj)

test_that("returns TRUE - test result 2 matches test_tbl3", {
  expect_equal(clustersV2, test_obj3, check.attributes = FALSE)
})

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!

Next release should drop support for this version