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

disable lockfile generation in callr, testthat::tests and R CMD CHECK #1346

Merged
merged 8 commits into from
Sep 19, 2024
Merged

disable lockfile generation in callr, testthat::tests and R CMD CHECK #1346

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2d2883c
disable lockfile generation in callr, testthat::tests and R CMD CHECK
m7pr Sep 17, 2024
9e5a8d3
Merge branch '479_mirai_lockfile@main' into disable@479_mirai_lockfil…
m7pr Sep 17, 2024
7bc4ab4
Merge branch '479_mirai_lockfile@main' into disable@479_mirai_lockfil…
m7pr Sep 18, 2024
d54cb80
disable lockfile creation during Quarto rendering
m7pr Sep 18, 2024
1862043
use is_lockfile_enable as function inside srv_teal_lockfile
m7pr Sep 18, 2024
1dd4758
remove cat statements
m7pr Sep 18, 2024
14d4a14
remove cats
m7pr Sep 18, 2024
adb8440
remove more cats
m7pr Sep 18, 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
13 changes: 12 additions & 1 deletion R/zzz.R
View file
Open in desktop
pawelru marked this conversation as resolved.
Show resolved Hide resolved
pawelru marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
.onLoad <- function(libname, pkgname) {
# adapted from https://github.com/r-lib/devtools/blob/master/R/zzz.R

lockfile_flag <-
!(
identical(Sys.getenv("CALLR_IS_RUNNING"), "true") || # inside callr process
identical(Sys.getenv("TESTTHAT"), "true") || # inside devtools::test
Copy link
Contributor

Choose a reason for hiding this comment

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

a question
what would happen if we do load_all() and then test()?
Do I understand correctly that the option would be set to true and then lockfile created inside the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check, good question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think that Sys.getenv("TESTTHAT") flag is set after the package is loaded during tests, and the initial value of getOption('teal.lockfile.enable') is set to TRUE during tests. Not as I would expect it to work

I added this snippet to check the behavior of this flag during checks.

cat('\n teal.lockfile.enable: ', getOption('teal.lockfile.enable'), '\n')

This made me think we actually need to disable the creation of the lockfile inside srv_teal_lockfile, if Sys.getenv("TESTTHAT") gets set later after the package is loaded

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sys.getenv("TESTTHAT") is set at testthat::test_that, not at devtools::test()

image

Copy link
Contributor

Choose a reason for hiding this comment

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

This made me think we actually need to disable the creation of the lockfile inside srv_teal_lockfile, if Sys.getenv("TESTTHAT") gets set later after the package is loaded

I got a similar idea. And I was thinking about this particular example in my previous comment here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this commit 1862043 I moved the functionality inside srv_teal_lockfile so the checks are made on lockfile execution.
If the option was set before the lockfile was called, then the option is superior to the circumstances check

!identical(Sys.getenv("QUARTO_PROJECT_ROOT"), "") || # inside Quarto process
(
("CheckExEnv" %in% search()) || any(c("_R_CHECK_TIMINGS_", "_R_CHECK_LICENSE_") %in% names(Sys.getenv()))
) # inside R CMD CHECK
)

teal_default_options <- list(
teal.show_js_log = FALSE,
teal.lockfile.enable = TRUE,
teal.lockfile.enable = lockfile_flag,
shiny.sanitize.errors = FALSE
)

Expand Down
6 changes: 0 additions & 6 deletions tests/testthat/setup-options.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
withr::local_options(
# we should't run lockfile process multiple times in tests as it starts background process which is not
# possible to kill once run. It means that background R sessions are being cumulated
list(teal.lockfile.enable = FALSE),
.local_envir = testthat::teardown_env()
)

# `opts_partial_match_old` is left for exclusions due to partial matching in dependent packages (i.e. not fixable here)
# it might happen that it is not used right now, but it is left for possible future use
Expand Down
Loading