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

TealAppDriver skips tests when shinytest2 or rvest are not installed #1432

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+9 −6
Open

TealAppDriver skips tests when shinytest2 or rvest are not installed #1432

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8541273
feat: TealAppDriver skips tests when shinytest2 or rvest are not inst…
averissimo Dec 18, 2024
8bcaa47
Merge branch 'main' into 518-pkgs_imports@main
averissimo Dec 18, 2024
669420e
Merge branch 'main' into 518-pkgs_imports@main
averissimo Dec 19, 2024
21bc010
feat: also check that testthat is installed
averissimo Dec 19, 2024
0b6af0d
Merge branch 'main' into 518-pkgs_imports@main
averissimo Dec 19, 2024
d02aab8
Merge branch 'main' into 518-pkgs_imports@main
averissimo Dec 19, 2024
72cc3d8
Merge branch 'main' into 518-pkgs_imports@main
averissimo Dec 19, 2024
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
15 changes: 9 additions & 6 deletions R/TealAppDriver.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,15 @@
TealAppDriver <- R6::R6Class( # nolint: object_name.
"TealAppDriver",
inherit = {
if (!requireNamespace("shinytest2", quietly = TRUE)) {
stop("Please install 'shinytest2' package to use this class.")
}
if (!requireNamespace("rvest", quietly = TRUE)) {
stop("Please install 'rvest' package to use this class.")
}
lapply(c("testthat", "shinytest2", "rvest"), function(.x, use_testthat) {
if (!requireNamespace(.x, quietly = TRUE)) {
if (use_testthat) {
Comment on lines +14 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

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):

Suggested change
if (!requireNamespace(.x, quietly = TRUE)) {
if (use_testthat) {
if (use_testthat && !requireNamespace(.x, quietly = TRUE)) {

Copy link
Contributor Author

@averissimo averissimo Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
  )
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's some needed context that I forgot to add to the original post. This chunk of code serves dual-purposes:

  1. On {teal} if the developer uses the class manually, but didn't install the *OPTIONAL* dependencies
  2. On {tmc}, {tmg}, and other (possibly), as TealAppDriver depends on {teal} Suggested packages to work, but cannot be added to DESCRIPTION to {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.

testthat::skip(sprintf("%s is not installed", .x))
} else {
stop("Please install '", .x, "' package to use this class.", call. = FALSE)
}
}
}, use_testthat = requireNamespace("testthat", quietly = TRUE) && testthat::is_testing())
shinytest2::AppDriver
},
# public methods ----
Expand Down
Loading