Merge pull request #143 from pbchase/address_pkgdown_job_failures

Address pkgdown job failures
ctsit · Nov 21, 2023 · f90c017 · f90c017
2 parents b14bc7d + c12524f
commit f90c017
Show file tree

Hide file tree

Showing 25 changed files with 163 additions and 110 deletions.
diff --git a/R/devtools.R b/R/devtools.R
@@ -192,6 +192,9 @@ mutate_columns_to_posixct <- function(data, column_names) {
 #'
 #' dplyr::collect(dplyr::tbl(target_conn, table_name))
 #'
+#' DBI::dbDisconnect(source_conn, shutdown = TRUE)
+#' DBI::dbDisconnect(target_conn, shutdown = TRUE)
+#'
 #' \dontrun{
 #' library(tidyverse)
 #' library(lubridate)

diff --git a/R/logging.R b/R/logging.R
@@ -18,8 +18,8 @@ error_list <- dplyr::tibble(
 #' \dontrun{
 #'  build_etl_job_log_df(
 #'    job_duration,
-#'    job_summary
-#'    level,
+#'    job_summary,
+#'    level
 #'  )
 #' }
 build_etl_job_log_df <- function(job_duration, job_summary, level) {
@@ -492,9 +492,9 @@ write_success_job_log_entry <- function(con, job_duration, job_summary) {
 #'  write_error_log_entry(
 #'    conn = con,
 #'    target_db_name = rc_case,
-#'    table_written = "cases"
+#'    table_written = "cases",
 #'    df = data_written,
-#'    pk_col = "record_id",
+#'    pk_col = "record_id"
 #'  )
 #' }
 write_error_log_entry <- function(conn, target_db_name, table_written = NULL, df, pk_col) {

diff --git a/R/summary_metrics.R b/R/summary_metrics.R
@@ -15,7 +15,7 @@
 #' \dontrun{
 #'  write_summary_metrics(
 #'    reporting_period_start = ymd_hms("2022-01-01 00:00:00", tz=Sys.getenv("TIME_ZONE")),
-#'    reporting_period_end = ceiling_date(reporting_period_start, "month", change_on_boundary = T)
+#'    reporting_period_end = ceiling_date(reporting_period_start, "month", change_on_boundary = T),
 #'    metric_type = "state",
 #'    metric_dataframe = my_cool_df,
 #'    conn = my_conn

diff --git a/R/utils.R b/R/utils.R
@@ -22,8 +22,12 @@ disable_non_interactive_quit <- function() {
 #' @return the exact length of the time span between start time and end time
 #' @export
 #' @examples
-#' \dontrun{get_job_duration(get_script_run_time(),get_current_time()}
-#'
+#' \dontrun{
+#' get_job_duration(
+#'   start_time = get_script_run_time(),
+#'   end_time = get_current_time()
+#' )
+#' }
 get_job_duration <- function(start_time, end_time) {
   return(lubridate::time_length(end_time - start_time))
 }

diff --git a/R/write_data.R b/R/write_data.R
@@ -21,7 +21,7 @@
 #'   df_to_write = rule_output,
 #'   schema = Sys.getenv("ETL_DB_SCHEMA"),
 #'   overwrite = FALSE,
-#'   db_name = Sys.getenv("ETL_DB_NAME")
+#'   db_name = Sys.getenv("ETL_DB_NAME"),
 #'   append = TRUE
 #' )
 #' }

diff --git a/man/build_etl_job_log_df.Rd b/man/build_etl_job_log_df.Rd
diff --git a/man/copy_entire_table_to_db.Rd b/man/copy_entire_table_to_db.Rd
diff --git a/man/get_job_duration.Rd b/man/get_job_duration.Rd
diff --git a/man/write_error_log_entry.Rd b/man/write_error_log_entry.Rd
diff --git a/man/write_summary_metrics.Rd b/man/write_summary_metrics.Rd
diff --git a/man/write_to_sql_db.Rd b/man/write_to_sql_db.Rd
diff --git a/tests/testthat/test-dataset_diff.R b/tests/testthat/test-dataset_diff.R
@@ -3,7 +3,7 @@
 # dataset_diff result separately
 test_dataset_diff_components <- function(df, df_name) {
   testthat::test_that(paste("dataset_diff updates match for", df_name), {
-    testthat::expect_true(dplyr::all_equal(
+    testthat::expect_true(all.equal(
       dataset_diff(
         source = df$source,
         source_pk = df$source_pk,
@@ -15,7 +15,7 @@ test_dataset_diff_components <- function(df, df_name) {
   })
 
   testthat::test_that(paste("dataset_diff updates match for", df_name), {
-    testthat::expect_true(dplyr::all_equal(
+    testthat::expect_true(all.equal(
       dataset_diff(
         source = df$source,
         source_pk = df$source_pk,
@@ -27,7 +27,7 @@ test_dataset_diff_components <- function(df, df_name) {
   })
 
   testthat::test_that(paste("dataset_diff updates match for", df_name), {
-    testthat::expect_true(dplyr::all_equal(
+    testthat::expect_true(all.equal(
       dataset_diff(
         source = df$source,
         source_pk = df$source_pk,
@@ -41,3 +41,4 @@ test_dataset_diff_components <- function(df, df_name) {
 
 test_dataset_diff_components(dataset_diff_test_bar_bang, "bar_bang")
 test_dataset_diff_components(dataset_diff_test_user_data, "user_data")
+
diff --git a/tests/testthat/test-delete_project.R b/tests/testthat/test-delete_project.R
@@ -83,4 +83,4 @@ testthat::test_that("delete_project deletes, updates and returns the correct pro
 
 })
 
-DBI::dbDisconnect(conn)
+DBI::dbDisconnect(conn, shutdown = TRUE)
diff --git a/tests/testthat/test-devtools.R b/tests/testthat/test-devtools.R
@@ -22,6 +22,8 @@ testthat::test_that("create_test_table creates redcap_projects with the right di
   testthat::expect_equal(dim(tbl(conn, "redcap_projects") %>% collect()), c(20,142))
 })
 
+DBI::dbDisconnect(conn)
+
 testthat::test_that("convert_schema_to_sqlite can convert a MySQL schema to valid SQLite syntax", {
   mysql_schema <- "CREATE TABLE `redcap_entity_project_ownership` (
   `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
@@ -72,9 +74,14 @@ testthat::test_that("copy_entire_table_to_db works", {
     target_conn = target_conn
   )
 
+  result <- dplyr::collect(dplyr::tbl(target_conn, table_name))
+
+  DBI::dbDisconnect(source_conn, shutdown = TRUE)
+  DBI::dbDisconnect(target_conn, shutdown = TRUE)
+
   # verify the copy
   testthat::expect_equal(
-    dplyr::collect(dplyr::tbl(target_conn, table_name)),
+    result,
     test_data
   )
 })
diff --git a/tests/testthat/test-logging.R b/tests/testthat/test-logging.R
@@ -1,6 +1,6 @@
 test_that("log_job_debug writes a debug log entry", {
     script_name <- "test-logging-debug"
-    init_etl(script_name, log_db_drv = RSQLite::SQLite())
+    init_etl(script_name, log_db_drv = duckdb::duckdb())
     log_con <- get_package_scope_var("log_con")
     summary <- paste("Writing log for", script_name)
 
@@ -9,12 +9,13 @@ test_that("log_job_debug writes a debug log entry", {
         log_con,
         "SELECT * FROM rcc_job_log WHERE level = 'DEBUG'"
     )
+    DBI::dbDisconnect(log_con, shutdown=TRUE)
     expect_equal(result$job_summary_data, summary)
 })
 
 test_that("log_job_failure writes an error log entry", {
     script_name <- "test-logging-failure"
-    init_etl(script_name, log_db_drv = RSQLite::SQLite())
+    init_etl(script_name, log_db_drv = duckdb::duckdb())
     log_con <- get_package_scope_var("log_con")
     summary <- paste("Writing log for", script_name)
 
@@ -24,12 +25,13 @@ test_that("log_job_failure writes an error log entry", {
         "SELECT * FROM rcc_job_log WHERE level = 'ERROR'"
     )
     summary_data_json <- rjson::fromJSON(result$job_summary_data)
+    DBI::dbDisconnect(log_con, shutdown=TRUE)
     expect_equal(summary_data_json$error_message, summary)
 })
 
 test_that("log_job_success writes a success log entry", {
     script_name <- "test-logging-success"
-    init_etl(script_name, log_db_drv = RSQLite::SQLite())
+    init_etl(script_name, log_db_drv = duckdb::duckdb())
     log_con <- get_package_scope_var("log_con")
     summary <- paste("Writing log for", script_name)
 
@@ -38,5 +40,6 @@ test_that("log_job_success writes a success log entry", {
         log_con,
         "SELECT * FROM rcc_job_log WHERE level = 'SUCCESS'"
     )
+    DBI::dbDisconnect(log_con, shutdown=TRUE)
     expect_equal(result$job_summary_data, summary)
 })
diff --git a/tests/testthat/test-project_life_cycle.R b/tests/testthat/test-project_life_cycle.R
@@ -47,7 +47,7 @@ testthat::test_that("get_project_life_cycle caches and returns event_date, log_e
     read_cache = F
   )
 
-  testthat::expect_true(dplyr::all_equal(result, cached_result))
+  testthat::expect_true(all.equal(result, cached_result))
   testthat::expect_equal("Date", class(result$event_date))
   testthat::expect_equal(
     result$log_event_table %in% seq(1:9),

diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R
@@ -2,46 +2,46 @@ test_that("get_job_duration properly calculates the elapsed time from start time
     elapsed_time <- lubridate::dseconds(5)
     start_time <- lubridate::now()
     end_time <- start_time + elapsed_time
-    expect_equal(get_job_duration(start_time, end_time), lubridate::time_length(elapsed_time))
+    testthat::expect_equal(get_job_duration(start_time, end_time), lubridate::time_length(elapsed_time))
 })
 
 test_that("get_package_scope_var properly gets vars set by set_package_scope_var", {
     key <- "test-utils-key"
     value <- "hello world"
     set_package_scope_var(key, value)
     retrieved_value <- get_package_scope_var(key)
-    expect_equal(retrieved_value, value)
+    testthat::expect_equal(retrieved_value, value)
 })
 
 test_that("is_db_con returns TRUE for a DBI connection object", {
-    conn <- dbConnect(RSQLite::SQLite())
+    conn <- DBI::dbConnect(RSQLite::SQLite())
     DBI::dbGetInfo(conn)
     # TODO(mbentz-uf): Add read to check we can read/write
     DBI::dbWriteTable(conn, "mtcars", mtcars)
-    expect_true(is_db_con(conn))
+    testthat::expect_true(is_db_con(conn))
     DBI::dbDisconnect(conn)
 })
 
 test_that("is_db_con returns FALSE for a non DBI connection object", {
     conn <- "invalid connection"
-    expect_false(is_db_con(conn))
+    testthat::expect_false(is_db_con(conn))
 })
 
 test_that("init_etl properly sets script name, script run time, and initializes log connection", {
     test_script_name <- "test_script"
-    test_run_time <- lubridate::now()
+    test_run_time <- lubridate::now(tz = Sys.getenv("TIME_ZONE"))
     init_etl(script_name = test_script_name, fake_runtime = test_run_time, log_db_drv = RSQLite::SQLite())
-    expect_equal(get_script_name(), test_script_name)
-    expect_equal(get_script_run_time(), test_run_time)
-    expect_true(is_db_con(get_package_scope_var("log_con")))
+    testthat::expect_equal(get_script_name(), test_script_name)
+    testthat::expect_equal(get_script_run_time(), test_run_time)
+    testthat::expect_true(is_db_con(get_package_scope_var("log_con")))
 })
 
 test_that("is_on_ci returns TRUE when CI is set to FALSE", {
-    Sys.setenv(CI = TRUE)
-    expect_true(is_on_ci())
+  Sys.setenv(CI = TRUE)
+  testthat::expect_true(is_on_ci())
 })
 
 test_that("is_on_ci returns FALSE when CI is set to FALSE", {
-    Sys.setenv(CI = FALSE)
-    expect_false(is_on_ci())
+  Sys.setenv(CI = FALSE)
+  testthat::expect_false(is_on_ci())
 })