Unexpected message and test skipping when using `extended_types = TRUE`

[jeanetteclark]

I'm using RSQLite as the backend data store/access for a package designed to retrieve and access some fish data. When running tests, which involve creating the database using RSQLite as part of the setup, I saw some strange output ( {.fn {top_level_fun}} needs the {.pkg {.val {pkgs_not_installed}}} package{?s}. and noticed that most of my tests were being skipped. You can recreate the behavior by doing the below - my apologies for the complexity I'm not sure if I could recreate it outside of the packaging context.

I believe that the cryptic message itself is generated here, and the actual problem is that I didn't have the hms package installed, which this line checks for. Additionally, I think that dbConnect is not a package anymore?

Finally, I also question whether it is appropriate to force users to skip tests - had we not had other tests failing downstream because of the earlier skipped setup tasks, we might not have realized that all of our tests were being skipped on GHA.

In the end, I was able to resolve the issue by adding hms to Suggests. However, this behavior seems not ideal and it took me forever to figure out what the heck was happening, so I thought I'd bring it to your attention.

Reprex

# from outside a project
remove.packages("hms")
usethis::create_package("testRSQLbug")
# switch to testRSQLbug project
usethis::use_package("RSQLite")
usethis::use_package("DBI")
usethis::use_testthat()
usethis::use_test("test_connect")

test_that("test connect", {
    mydb <- DBI::dbConnect(RSQLite::SQLite(), "my-db.sqlite", extended_types = TRUE)
    expect_s4_class(mydb, "SQLiteConnection")
    DBI::dbDisconnect(mydb)
})

output from testing

==> devtools::test()

ℹ Testing testRSQbug
✔ | F W  S  OK | Context
✔ |      1   0 | test_package_loading                        

══ Results ══════════════════════════════════════════════════
── Skipped tests (1) ────────────────────────────────────────
• {.fn {top_level_fun}} needs the {.pkg {.val
  {pkgs_not_installed}}} package{?s}. (1):
  test-test_package_loading.R:4:5

[ FAIL 0 | WARN 0 | SKIP 1 | PASS 0 ]

[krlmlr]

Thanks. I'm sorry that my hack caused trouble for you. You're right, that message needs a glue() call to be formatted properly.

We can make the skip() conditional on that the package being tested is really RSQLite, and not another package. Checking the TESTTHAT environment variable is not sufficient, I had missed that use case when implementing this.

We could set a temporary flag, option, or environment variable in tests/testthat/setup.R . What do you think? Would you like to take a stab?

[jeanetteclark]

Seems easy enough - what is your preferred contribution workflow given the standalone file? Should I PR to that repo, wait for merge, and then PR to this one?

[krlmlr]

Thanks, appreciated. A single PR should work fine.

[jeanetteclark]

Sorry maybe I'm misunderstanding the idea, but I think I need to make edits to both the read only file import-standalone-check_suggested in your cynkra/dm/ repo and to make the changes to setup testthat in this repository. So you should expect to merge two small PRs in separate repos to fix this thing. Are there any other packages using that standalone file that I should be aware of? Perhaps I'm not the best person to do this since I don't totally understand why the original hack existed in the first place, or why it is is within this standalone file

[krlmlr]

Thanks, good catch. We can focus our work on this repo. I can bring the changes back to dm after that.

The hack exists so that calling check_suggested() is enough to protect tests against failing if a suggested package is missing. AFAICT it's only dm and RSQLite now, seems manageable.

[github-actions]

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.