Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the configuration and runner infrastructure for the 'scrutin' testing tool. Key additions include a .gitignore update to track runner scripts while ignoring state files, a detailed config.toml for execution parameters, and a specialized R runner script for the testthat framework. Review feedback suggests enhancing the runner's startup checks to verify all required R packages (R6 and testthat) are installed and ensuring that calls to external functions like trace() use the :: operator as per the project's style guidelines.
| if (!requireNamespace("pkgload", quietly = TRUE)) { | ||
| .scrutin_env$emit(.scrutin_env$event( | ||
| file = "<worker_startup>", | ||
| outcome = "error", | ||
| subject_kind = "engine", | ||
| subject_name = "<worker_startup>", | ||
| message = paste( | ||
| "scrutin's default R runner requires 'pkgload', which is not installed.", | ||
| "Fix: install.packages('pkgload')", | ||
| "Or edit .scrutin/runners/<tool>.R to use library() instead of pkgload::load_all().", | ||
| sep = "\n" | ||
| ) | ||
| )) | ||
| quit(save = "no", status = 2) | ||
| } |
There was a problem hiding this comment.
The runner script relies on R6 and testthat (used in the ScrutinReporter class and run_test function), but it only checks for the presence of pkgload. To ensure a clean failure with an actionable message if these dependencies are missing, they should be included in the pre-flight check.
missing_deps <- c()
for (pkg in c("pkgload", "R6", "testthat")) {
if (!requireNamespace(pkg, quietly = TRUE)) missing_deps <- c(missing_deps, pkg)
}
if (length(missing_deps) > 0) {
.scrutin_env$emit(.scrutin_env$event(
file = "<worker_startup>",
outcome = "error",
subject_kind = "engine",
subject_name = "<worker_startup>",
message = paste(
sprintf("scrutin's default R runner requires the following missing packages: %s", paste(missing_deps, collapse = ", ")),
sprintf("Fix: install.packages(c(%s))", paste(sprintf("'%s'", missing_deps), collapse = ", ")),
sep = "\n"
)
))
quit(save = "no", status = 2)
}| # the search path, and calls resolve there (not in the namespace). | ||
| # Tracing in the namespace doesn't intercept search-path calls. | ||
| tryCatch( | ||
| suppressMessages(trace(fn_name, tracer = tracer_expr, print = FALSE)), |
There was a problem hiding this comment.
According to the repository style guide (Rule 73), functions from other packages should always be called using the :: operator. trace() is part of the methods package and should be prefixed accordingly.
suppressMessages(methods::trace(fn_name, tracer = tracer_expr, print = FALSE)),References
- Always use the :: operator to call functions from other packages (e.g., stats::shapiro.test, insight::model_info). (link)
1 participant
No description provided.