add helper to locate configuration files #680
Conversation
|
On my machine: library(odbc)
odbcListConfig()
#> name location
#> 1 DRIVERS /opt/homebrew/etc/odbcinst.ini
#> 2 SYSTEM DATA SOURCES /opt/homebrew/etc/odbc.ini
#> 3 USER DATA SOURCES /Users/simoncouch/.odbc.iniCreated on 2023-12-14 with reprex v2.0.2 |
tests/testthat/test-Connection.R
Outdated
|
|
||
| test_that("odbcListConfig errors informatively when unixODBC isn't available", { | ||
| skip_on_os(c("windows", "solaris")) | ||
| skip_if(!identical(unname(Sys.which("odbcinst")), ""), "odbcinst is available") |
There was a problem hiding this comment.
I don't think this test will ever be run on our CI as currently configured. Ran on a fresh RStudio Pro instance and checks were fine.
R/Connection.R
Outdated
| res <- strsplit(res, "\\:") | ||
| res <- vapply(res, `[[`, character(1), 2) |
There was a problem hiding this comment.
I think it's worth considering a quick check of the output here, maybe something like this:
if (!identical(length(res), c(2, 2, 2)) {
abort("Failed to parse output from odbcinst", internal = TRUE)
}That'll ensure the user gets a reasonable error if odbcinst gives unexpected results. (And the internal = TRUE ensures they get a prompt to file an issue).
Then you can add a snapshot test for that error by mocking system().
There was a problem hiding this comment.
My only question here is whether we should run a similar check instead on the output of system("odbcinst -j", intern = TRUE)? Based on it's source code I'm skeptical that odbcinst will ever return something that's almost parsable for us, but some issue in the communication with system() seems plausible.
There was a problem hiding this comment.
I don't think it's worth spending too much more time contemplating this as it's an unlikely failure mode; we just want to make sure we get something diagnostic in the unlikely event that the output does change.
|
BTW I'm pretty sure Also please hold of on merging this PR for a bit. |
|
Good to go with the merge. My bad for the mix-up with the merge conflict 'cause of my file reorg PR |
|
All good! Glad we made that renaming happen. |
`odbcListConfig()` now lives in `odbc-config.R` Merge branch 'main' into odbcListConfig # Conflicts: # NEWS.md # R/Connection.R
tests/testthat/test-odbc-config.R
Outdated
| }) | ||
|
|
||
| test_that("odbcListConfig errors informatively without unixODBC", { | ||
| skip_on_os(c("windows", "solaris")) |
There was a problem hiding this comment.
FWIW I'd be tempted to make a is_window() function so that you could mock it and then drop the skips.
There was a problem hiding this comment.
Sure thing. In these tests I always skip on Solaris. Should is_windows() on Solaris return true? Or figure out how to support Solaris and test on those systems as well?
There was a problem hiding this comment.
Once you've mocked the right functions, it shouldn't matter which OS it's actually running on.
And I'm reasonably certain we don't generally need to worry about solaris any more?
Closes #565.☃️