Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug in validate_inputs #793

Merged
merged 15 commits into from
Jan 25, 2023
Merged

fix bug in validate_inputs #793

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c154c7c
add check on enabled status
Jan 2, 2023
3001afc
apply code review
Jan 3, 2023
69cafc1
add unit tests
Jan 3, 2023
9feaacd
more review
Jan 3, 2023
ea1400b
improve unit tests
Jan 3, 2023
7942d18
apply review
Jan 9, 2023
8b1bc91
fix unit tests
Jan 10, 2023
c8f8e29
upgrade `validate_inputs` function (#796)
chlebowa Jan 25, 2023
0e64eb5
Merge branch 'main' into fix_validate_inputs@main
chlebowa Jan 25, 2023
913ece1
apply code review
Jan 25, 2023
0247c66
amend unit tests
Jan 25, 2023
477cf58
update pkgdown
Jan 25, 2023
83fb7c3
tweak docs
Jan 25, 2023
be43336
more unit tests
Jan 25, 2023
c523feb
fix unit tests following changes in teal.slice
Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion R/validate_inputs.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,11 @@ validate_inputs <- function(..., header = "Some inputs require attention") {
lapply(vals, checkmate::assert_class, "InputValidator")
checkmate::assert_string(header, null.ok = TRUE)

if (!all(vapply(vals, validator_enabled, logical(1L)))) {
warning("Some validators are disabled and will be omitted.", call. = TRUE)
Copy link
Contributor

@nikolas-burkoff nikolas-burkoff Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use a WARN from teal.logger here? an R warning may be a bit too much - but I'll let you decide

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably should stick to just one logging channel. Either use the built-in R messages and warnings or use the logger solution.

Copy link
Contributor

@kpagacz kpagacz Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning is that logger outputs can be piped to a file without R warnings. If someone was to do it, would they care about this warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

teal.logger it is then. Any advice on how to write unit tests for it?

Copy link
Contributor

@gogonzo gogonzo Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked this issue here and situation is not that simple for me, so let me know if you have some solution or a strong opinion about how to handle this situation:

It's not straightforward how to test modules which return warnings through the logger. options(TEAL.LOG_LEVEL) + capture_output in tests doesn't seem to be optimal solution. I've checked there is no function in the logger to "convert" logger-conditions into the exceptions.

With @chlebowa we tried to find the way to temporary (for tests) turn logger entries into R errors, warnings, messages and it's also not so easy neither. Currently, it's possible to do access level and msg in layout_teal_glue_generator via register_logger which would have to be called once again especially for tests.

Alternative solution (not for today) is to use logger::log_errors, logger::log_warnings and change all logger::log_warn in teal packages to generic warning. How would it work then? - log_errors uses withGlobalCallingHandlers and intercept errors (their messages) and append them into logger. Our modules then can work normally with warning, stop and they could be appended into logger if one adds logger::log_errors on top of the app.R file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is not a trivial issue, I have modified the tests to test console output as a temporary measure.

vals <- Filter(validator_enabled, vals)
}

fail_messages <- unlist(lapply(vals, gather_messages))
failings <- add_header(fail_messages, header)

Expand All @@ -120,6 +125,11 @@ validate_inputs <- function(..., header = "Some inputs require attention") {
validate_inputs_segregated <- function(validators, ...) {
checkmate::assert_list(validators, types = "InputValidator")

if (!all(vapply(validators, validator_enabled, logical(1L)))) {
warning("Some validators are disabled and will be omitted.", call. = TRUE)
validators <- Filter(validator_enabled, validators)
}

# Since some or all names may be NULL, mapply cannot be used here, a loop is required.
fail_messages <- vector("list", length(validators))
for (v in seq_along(validators)) {
Expand All @@ -142,7 +152,6 @@ gather_messages <- function(iv) {
unique(lapply(failing_inputs, function(x) x[["message"]]))
}


#' @keywords internal
# format failing messages with optional header message
add_header <- function(messages, header) {
Expand All @@ -161,3 +170,10 @@ gather_and_add <- function(iv, header) {
failings <- add_header(fail_messages, header)
failings
}

#' @keywords internal
#' test if an InputValidator object is enabled
#' returns logical of length 1
validator_enabled <- function(x) {
x$.__enclos_env__$private$enabled
}
44 changes: 44 additions & 0 deletions tests/testthat/test-validate_inputs.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,50 @@ testthat::test_that("invalid arguments raise errors", {
})


testthat::test_that("validate_inputs: disabled validators raise warnings", {
server <- function(input, output, session) {
iv1 <- shinyvalidate::InputValidator$new()
iv1$add_rule("letter", shinyvalidate::sv_in_set(LETTERS, "choose a capital letter"))
iv1$enable()
iv2 <- shinyvalidate::InputValidator$new()
iv2$add_rule("number", ~ if (as.integer(.) %% 2L == 1L) "choose an even number")
values <- shiny::reactive({
validate_inputs(iv1, iv2)
list(
"letter" = input[["letter"]],
"number" = input[["number"]]
)
})
}

shiny::testServer(server, {
testthat::expect_warning(values())
})
})


testthat::test_that("validate_inputs_segregated: disabled validators raise warnings", {
server <- function(input, output, session) {
iv1 <- shinyvalidate::InputValidator$new()
iv1$add_rule("letter", shinyvalidate::sv_in_set(LETTERS, "choose a capital letter"))
iv1$enable()
iv2 <- shinyvalidate::InputValidator$new()
iv2$add_rule("number", ~ if (as.integer(.) %% 2L == 1L) "choose an even number")
values <- shiny::reactive({
validate_inputs_segregated(list(iv1, iv2))
list(
"letter" = input[["letter"]],
"number" = input[["number"]]
)
})
}

shiny::testServer(server, {
testthat::expect_warning(values())
})
})


testthat::test_that("validate_inputs: valid inputs produce desired output", {
server <- function(input, output, session) {
iv <- shinyvalidate::InputValidator$new()
Expand Down