TealAppDriver skips tests when shinytest2 or rvest are not installed #1432
Conversation
|
Code Coverage SummaryDiff against mainResults for commit: 72cc3d8 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 27 suites 10m 26s ⏱️ Results for commit 72cc3d8. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Results for commit 91be1ca ♻️ This comment has been updated with latest results. |
| if (!requireNamespace(.x, quietly = TRUE)) { | ||
| if (use_testthat) { |
There was a problem hiding this comment.
What about inverting the order of the if else clause, so if there is no testthat and it is not in use the TealAppDriver class won't check for the suggested package used for testing (this uses && shortcut) .
Here is my suggestion (indentation would need to change accordingly):
| if (!requireNamespace(.x, quietly = TRUE)) { | |
| if (use_testthat) { | |
| if (use_testthat && !requireNamespace(.x, quietly = TRUE)) { |
There was a problem hiding this comment.
Am I misunderstanding or would we still need an else if after for the current else clause with your suggestion?
if (use_testthat && !requireNamespace(.x, quietly = TRUE)) {
testthat::skip(sprintf("%s is not installed", .x))
} else if (!requireNamespace(.x, quietly = TRUE)) {
stop("Please install '", .x, "' package to use this class.", call. = FALSE)
}I dislike the repetition of the suggestion, maybe a tiny bit more than the nested if clause
Given it's a personal style, if you feel the suggestion is better I'll make the change
There was a problem hiding this comment.
Note that we could remove the else from the current code to make it a bit leaner
if (!requireNamespace(.x, quietly = TRUE)) {
if (use_testthat) {
testthat::skip(sprintf("%s is not installed", .x))
}
stop("Please install '", .x, "' package to use this class.", call. = FALSE)
}Second note: for a moment I considered the following, but the tryCatch would be too broad and from all the options I dislike this the most
if (!requireNamespace(.x, quietly = TRUE)) {
tryCatch(
testthat::skip(sprintf("%s is not installed", .x)),
error = function(err) stop("Please install '", .x, "' package to use this class.", call. = FALSE)
)
}There was a problem hiding this comment.
I was thinking this code will be executed mostly by developers checking & testing the package, so that should be checked first as it can lead to speed improvements and code readability. I think t
I would expect developers to know they need to install a package if it cannot be loaded and a message reported this.
What do you think about this simplification?
if (requireNamespace("testthat", quietly = TRUE) && testthat::is_testing() identical(Sys.getenv("RTESTS"), "true")) {
lapply(c("shinytest2", "rvest"), testthat::skip_if_not_installed)
}
The identical(Sys.getenv("RTESTS"), "true") is to ensure that when one does devtools::check it is triggered (but can be omitted).
I tested several snippets but I'm not happy with any regarding readability, perhaps it would be easier if we had a little helper/uril function to solve this that we can reuse across packages.
There was a problem hiding this comment.
There's some needed context that I forgot to add to the original post. This chunk of code serves dual-purposes:
- On
{teal}if the developer uses the class manually, but didn't install the *OPTIONAL* dependencies - On
{tmc},{tmg}, and other (possibly), asTealAppDriverdepends on{teal}Suggested packages to work, but cannot be added toDESCRIPTIONto{tmc},{tmg},{...}- It doesn't make sense (and will trigger a CRAN) warning if we just add them to Suggest and not use it in the package itself
That's why we need skip and stop conditions. To be complete, I've also added testthat to that small list.
if (requireNamespace("testthat", quietly = TRUE) && testthat::is_testing() identical(Sys.getenv("RTESTS"), "true")) { lapply(c("shinytest2", "rvest"), testthat::skip_if_not_installed) }
This suggestion will allow the creation of the class if we don't have {testthat} installed. Almost an impossibility, but it could happen.
| if (!requireNamespace(.x, quietly = TRUE)) { | ||
| if (use_testthat) { |
There was a problem hiding this comment.
I was thinking this code will be executed mostly by developers checking & testing the package, so that should be checked first as it can lead to speed improvements and code readability. I think t
I would expect developers to know they need to install a package if it cannot be loaded and a message reported this.
What do you think about this simplification?
if (requireNamespace("testthat", quietly = TRUE) && testthat::is_testing() identical(Sys.getenv("RTESTS"), "true")) {
lapply(c("shinytest2", "rvest"), testthat::skip_if_not_installed)
}
The identical(Sys.getenv("RTESTS"), "true") is to ensure that when one does devtools::check it is triggered (but can be omitted).
I tested several snippets but I'm not happy with any regarding readability, perhaps it would be easier if we had a little helper/uril function to solve this that we can reuse across packages.
Pull Request
Part of insightsengineering/teal.modules.general#774
See insightsengineering/teal.modules.general#774 (comment)