Skip to content

Commit

Permalink
Update exec_cfg_check to use config source path relative to the hub…
Browse files Browse the repository at this point in the history
… root (#142)

* add tests for running custom checks outside of hub root

* add proposed fix for running checks outside of hub root

* rewrite helper to allow for multiple checks

* refactor custom check tests

* remove superseded validations test files

* Revert "remove superseded validations test files"

This reverts commit 15a0939.

* update path in test yamls

Using the dynamic test path meant that we could not test these files as
they existed in a hub. With this pattern, we can copy the test files
over instead of having to do the cumbersome programmatic generation.

Less code == better code

* dang fingies missspelling fings

* simplify helpers

* simplify tests

* SNAP!

* update NEWS
  • Loading branch information
zkamvar authored Oct 31, 2024
1 parent a75e337 commit bf7e6e8
Show file tree
Hide file tree
Showing 9 changed files with 68 additions and 38 deletions.
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# hubValidations (development version)

* Custom checks no longer fail if validation is run outside of the root of the hub (#141)
* Downgrade result of missing model metadata file check from `check_error` to `check_failure` and suppress early return in case of check failure in `validate_model_file()` (#138).
* Add `check_file_n()` function to validate that the number of files submitted per round does not exceed the allowed number of submissions per team (#139).
* Ignore `NA`s in relevant `tbl` columns in `opt_check_tbl_col_timediff()` and `opt_check_tbl_horizon_timediff()` checks to ensure rows that may not be targeting relevant to modeling task do not cause false check failure. (#140).
Expand Down
4 changes: 3 additions & 1 deletion R/exec_cfg_check.R
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ exec_cfg_check <- function(check_name, validations_cfg, caller_env, caller_call)
)
} else if (!is.null(fn_cfg[["source"]])) {
# TODO Validate source script.
source(fn_cfg[["source"]], local = TRUE)
hub_path <- rlang::env_get(env = caller_env, nm = "hub_path")
src <- fs::path(hub_path, fn_cfg[["source"]])
source(src, local = TRUE)
fn <- get(fn_cfg[["fn"]])
}

Expand Down
3 changes: 1 addition & 2 deletions tests/testthat/_snaps/execute_custom_checks.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@
# execute_custom_checks sourcing functions from scripts works

Code
test_custom_checks_caller(validations_cfg_path = testthat::test_path("testdata",
"config", "validations-src.yml"))
validations_src_external
Message
-- 2023-05-08-hub-ensemble.parquet ----
Expand Down
31 changes: 31 additions & 0 deletions tests/testthat/helper-custom-fns.R
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,34 @@ test_custom_checks_caller <- function(
# nolint end
execute_custom_checks(validations_cfg_path = validations_cfg_path)
}

stand_up_custom_check_hub <- function(
# nolint start
hub_path = system.file("testhubs/flusight", package = "hubValidations"),
new_path = NULL,
check_dir = testthat::test_path("testdata/src/R/"),
yaml_path = testthat::test_path("testdata/config/validations-src.yml")
) {
if (is.null(new_path)) {
return()
}
# make a copy of the hub
fs::dir_copy(hub_path, new_path)

# create the script path
new_path <- fs::path(new_path, fs::path_file(hub_path))
script_path <- fs::path(new_path, "src/validations/R")
# NOTE: this creates the `src/validatons` directory because if the `R/` dir
# exists, `dir_copy()` will place the copied directory _inside_ it, which is
# not what we want.
fs::dir_create(fs::path_dir(script_path))

# Copy the directory of the existing files to the new script path
fs::dir_copy(check_dir, script_path)

# Copy the provided yaml file to the config folder
new_yaml <- fs::path(new_path, "hub-config", "validations.yml")
fs::file_copy(yaml_path, new_yaml, overwrite = TRUE)
# nolint end
return(new_path)
}
53 changes: 25 additions & 28 deletions tests/testthat/test-execute_custom_checks.R
Original file line number Diff line number Diff line change
Expand Up @@ -25,54 +25,51 @@ test_that("execute_custom_checks works", {
})

test_that("execute_custom_checks sourcing functions from scripts works", {
tmp <- withr::local_tempdir()
the_config <- testthat::test_path("testdata/config/validations-src.yml")

expect_snapshot(
test_custom_checks_caller(
validations_cfg_path = testthat::test_path(
"testdata",
"config",
"validations-src.yml"
)
)
)
hub <- stand_up_custom_check_hub(new_path = tmp, yaml_path = the_config)

expect_no_error({
withr::with_tempdir({
validations_src_external <- test_custom_checks_caller(hub_path = hub)
})
})

expect_snapshot(validations_src_external)
})


test_that("execute_custom_checks return early when appropriate", {
# When the first custom check returns an check_error class object, custom check
# execution should return early
early_ret_custom <- test_custom_checks_caller(
validations_cfg_path = testthat::test_path(
"testdata",
"config",
"validations-early-ret.yml"
)
tmp1 <- withr::local_tempdir()
hub1 <- stand_up_custom_check_hub(new_path = tmp1,
yaml_path = testthat::test_path("testdata/config/validations-early-ret.yml")
)
early_ret_custom <- test_custom_checks_caller(hub_path = hub1)
expect_snapshot(early_ret_custom)
expect_length(early_ret_custom, 1L)
expect_false("check_2" %in% names(early_ret_custom))

# Same when the first custom check returns an exec_error class object
early_ret_exec_error <- test_custom_checks_caller(
validations_cfg_path = testthat::test_path(
"testdata",
"config",
"validations-exec-error.yml"
)
tmp2 <- withr::local_tempdir()
hub2 <- stand_up_custom_check_hub(new_path = tmp2,
yaml_path = testthat::test_path("testdata/config/validations-exec-error.yml")
)
early_ret_exec_error <- test_custom_checks_caller(hub2)
expect_snapshot(early_ret_exec_error)
expect_length(early_ret_exec_error, 1L)
expect_false("check_2" %in% names(early_ret_exec_error))
expect_false("check_2" %in% names(early_ret_custom))


# When the first custom check returns an check_failure class object, custom check
# execution should proceed
no_early_ret_custom <- test_custom_checks_caller(
validations_cfg_path = testthat::test_path(
"testdata",
"config",
"validations-no-early-ret.yml"
)
tmp3 <- withr::local_tempdir()
hub3 <- stand_up_custom_check_hub(new_path = tmp3,
yaml_path = testthat::test_path("testdata/config/validations-no-early-ret.yml")
)
no_early_ret_custom <- test_custom_checks_caller(hub3)
expect_snapshot(no_early_ret_custom)
expect_length(no_early_ret_custom, 2L)
expect_true("check_2" %in% names(no_early_ret_custom))
Expand Down
4 changes: 2 additions & 2 deletions tests/testthat/testdata/config/validations-early-ret.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ default:
test_custom_checks_caller:
check_1:
fn: "simple_example_check"
source: !expr testthat::test_path("testdata/src/R/src_simple_example_check.R")
source: "src/validations/R/src_simple_example_check.R"
args:
check: FALSE
error: TRUE
check_2:
fn: "simple_example_check"
source: !expr testthat::test_path("testdata/src/R/src_simple_example_check.R")
source: "src/validations/R/src_simple_example_check.R"
args:
check: TRUE
error: FALSE
4 changes: 2 additions & 2 deletions tests/testthat/testdata/config/validations-exec-error.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ default:
test_custom_checks_caller:
check_1:
fn: "simple_example_check"
source: !expr testthat::test_path("testdata/src/R/src_simple_example_check.R")
source: "src/validations/R/src_simple_example_check.R"
args:
check: FALSE
error: TRUE
exec_error: TRUE
check_2:
fn: "simple_example_check"
source: !expr testthat::test_path("testdata/src/R/src_simple_example_check.R")
source: "src/validations/R/src_simple_example_check.R"
args:
check: TRUE
error: FALSE
4 changes: 2 additions & 2 deletions tests/testthat/testdata/config/validations-no-early-ret.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ default:
test_custom_checks_caller:
check_1:
fn: "simple_example_check"
source: !expr testthat::test_path("testdata/src/R/src_simple_example_check.R")
source: "src/validations/R/src_simple_example_check.R"
args:
check: FALSE
error: FALSE
check_2:
fn: "simple_example_check"
source: !expr testthat::test_path("testdata/src/R/src_simple_example_check.R")
source: "src/validations/R/src_simple_example_check.R"
args:
check: TRUE
error: FALSE
2 changes: 1 addition & 1 deletion tests/testthat/testdata/config/validations-src.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ default:
test_custom_checks_caller:
src_check_works:
fn: "src_check_works"
source: !expr testthat::test_path("testdata/src/R/src_check_works.R")
source: "src/validations/R/src_check_works.R"
args:
extra_msg: "Extra arguments passed"

0 comments on commit bf7e6e8

Please sign in to comment.