From 03481e9b6b34fe79532621ddeb7821e7de5c5697 Mon Sep 17 00:00:00 2001 From: Matt Dowle Date: Tue, 17 Aug 2021 19:37:11 -0600 Subject: [PATCH 1/4] attempt 1 doesn't work, see comments in PR --- R/test.data.table.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/test.data.table.R b/R/test.data.table.R index a8a19522f4..34aef30cf2 100644 --- a/R/test.data.table.R +++ b/R/test.data.table.R @@ -6,11 +6,13 @@ test.data.table = function(script="tests.Rraw", verbose=FALSE, pkg=".", silent=F if ("package:data.table" %chin% search()) stopf("data.table package is loaded. Unload or start a fresh R session.") rootdir = if (pkg!="." && pkg %chin% dir()) file.path(getwd(), pkg) else Sys.getenv("PROJ_PATH") subdir = file.path("inst","tests") + env = new.env(parent=.GlobalEnv) # in dev cc() sources all functions in .GlobalEnv # nocov end } else { # i) R CMD check and ii) user running test.data.table() rootdir = getNamespaceInfo("data.table","path") subdir = "tests" + env = new.env(parent=parent.env(.GlobalEnv)) # when user runs test.data.table() we don't want their variables in .GlobalEnv affecting tests, #3705 } fulldir = file.path(rootdir, subdir) @@ -93,7 +95,6 @@ test.data.table = function(script="tests.Rraw", verbose=FALSE, pkg=".", silent=F cat("getDTthreads(verbose=TRUE):\n") # for tracing on CRAN; output to log before anything is attempted getDTthreads(verbose=TRUE) # includes the returned value in the verbose output (rather than dangling '[1] 4'); e.g. "data.table is using 4 threads" catf("test.data.table() running: %s\n", fn) # print fn to log before attempting anything on it (in case it is missing); on same line for slightly easier grep - env = new.env(parent=.GlobalEnv) assign("testDir", function(x) file.path(fulldir, x), envir=env) # are R's messages being translated to a foreign language? #3039, #630 From 9057cd223203eac53dd46de59147eb550581e21f Mon Sep 17 00:00:00 2001 From: Matt Dowle Date: Tue, 17 Aug 2021 20:03:20 -0600 Subject: [PATCH 2/4] attach at the end of search() --- R/test.data.table.R | 2 +- inst/tests/tests.Rraw | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/R/test.data.table.R b/R/test.data.table.R index 34aef30cf2..0c7fbeb23a 100644 --- a/R/test.data.table.R +++ b/R/test.data.table.R @@ -1,6 +1,6 @@ test.data.table = function(script="tests.Rraw", verbose=FALSE, pkg=".", silent=FALSE, showProgress=interactive()&&!silent) { stopifnot(isTRUEorFALSE(verbose), isTRUEorFALSE(silent), isTRUEorFALSE(showProgress)) - if (exists("test.data.table", .GlobalEnv,inherits=FALSE)) { + if (exists("test.data.table", .GlobalEnv, inherits=FALSE)) { # package developer # nocov start if ("package:data.table" %chin% search()) stopf("data.table package is loaded. Unload or start a fresh R session.") diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index fb3cb363da..91393e3b26 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -96,8 +96,10 @@ sugg = c( # zoo # In DESCRIPTION:Suggests otherwise R CMD check warning: '::' or ':::' import not declared from: 'zoo'; it is tested in other.Rraw though ) for (s in sugg) { - assign(paste0("test_",s), loaded<-suppressWarnings(suppressMessages(require(s, character.only=TRUE)))) - if (!loaded) cat("\n**** Suggested package",s,"is not installed. Tests using it will be skipped.\n\n") + assign(paste0("test_",s), loaded<-suppressWarnings(suppressMessages( + library(s, character.only=TRUE, logical.return=TRUE, quietly=TRUE, pos="package:base") # attach at the end for #5101 + ))) + if (!loaded) cat("\n**** Suggested package",s,"is not installed or has dependencies missing. Tests using it will be skipped.\n\n") } test_longdouble = isTRUE(capabilities()["long.double"]) && identical(as.integer(.Machine$longdouble.digits), 64L) From cdc72d61e48f9b9a8ca7cc4d1fd9e70ba1c73612 Mon Sep 17 00:00:00 2001 From: Matt Dowle Date: Tue, 17 Aug 2021 21:05:53 -0600 Subject: [PATCH 3/4] attach last in other .Rraw too --- inst/tests/nafill.Rraw | 6 ++++-- inst/tests/other.Rraw | 4 +++- inst/tests/tests.Rraw | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/inst/tests/nafill.Rraw b/inst/tests/nafill.Rraw index dcaa0f40d4..1e5107fb71 100644 --- a/inst/tests/nafill.Rraw +++ b/inst/tests/nafill.Rraw @@ -15,8 +15,10 @@ sugg = c( "nanotime" ) for (s in sugg) { - assign(paste0("test_",s), loaded<-suppressWarnings(suppressMessages(require(s, character.only=TRUE)))) - if (!loaded) cat("\n**** Suggested package",s,"is not installed. Tests using it will be skipped.\n\n") + assign(paste0("test_",s), loaded<-suppressWarnings(suppressMessages( + library(s, character.only=TRUE, logical.return=TRUE, quietly=TRUE, warn.conflicts=FALSE, pos="package:base") # attach at the end for #5101 + ))) + if (!loaded) cat("\n**** Suggested package",s,"is not installed or has dependencies missing. Tests using it will be skipped.\n\n") } x = 1:10 diff --git a/inst/tests/other.Rraw b/inst/tests/other.Rraw index 89ce49b00f..a346a9e56b 100644 --- a/inst/tests/other.Rraw +++ b/inst/tests/other.Rraw @@ -15,7 +15,9 @@ INT = data.table:::INT if (any(duplicated(pkgs))) stop("Packages defined to be loaded for integration tests in 'inst/tests/other.Rraw' contains duplicates.") -f = function(pkg) suppressMessages(isTRUE(require(pkg, character.only=TRUE, quietly=TRUE, warn.conflicts=FALSE))) +f = function(pkg) suppressWarnings(suppressMessages(isTRUE( + library(pkg, character.only=TRUE, logical.return=TRUE, quietly=TRUE, warn.conflicts=FALSE, pos="package:base") # attach at the end for #5101 +))) loaded = sapply(pkgs, f) if (any(!loaded)) { stop("test.data.table('other.Rraw') is missing required package(s): ", paste(names(loaded)[!loaded], collapse=", "), ". If you can't install them and this is R CMD check, please set environment variable TEST_DATA_TABLE_WITH_OTHER_PACKAGES back to the default, false.") diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index 91393e3b26..0578b5b30f 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -97,7 +97,7 @@ sugg = c( ) for (s in sugg) { assign(paste0("test_",s), loaded<-suppressWarnings(suppressMessages( - library(s, character.only=TRUE, logical.return=TRUE, quietly=TRUE, pos="package:base") # attach at the end for #5101 + library(s, character.only=TRUE, logical.return=TRUE, quietly=TRUE, warn.conflicts=FALSE, pos="package:base") # attach at the end for #5101 ))) if (!loaded) cat("\n**** Suggested package",s,"is not installed or has dependencies missing. Tests using it will be skipped.\n\n") } From 0cc919e60b88ae5e9f8f0ba263b07e1af8b4fc7e Mon Sep 17 00:00:00 2001 From: Matt Dowle Date: Tue, 17 Aug 2021 21:27:12 -0600 Subject: [PATCH 4/4] add news item --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index 73e8fd5608..ef19ccd191 100644 --- a/NEWS.md +++ b/NEWS.md @@ -167,6 +167,8 @@ 31. `fwrite(DF, row.names=TRUE)` where `DF` has specific integer rownames (e.g. using `rownames(DF) <- c(10L,20L,30L)`) would ignore the integer rownames and write the row numbers instead, [#4957](https://github.com/Rdatatable/data.table/issues/4957). Thanks to @dgarrimar for reporting and @ColeMiller1 for the PR. Further, when `quote='auto'` (default) and the rownames are integers (either default or specific), they are no longer quoted. +32. `test.data.table()` would fail on test 1894 if the variable `z` was defined by the user, [#3705](https://github.com/Rdatatable/data.table/issues/3705). The test suite already ran in its own separate environment. That environment's parent is no longer `.GlobalEnv` to isolate it further. Thanks to Michael Chirico for reporting, and Matt Dowle for the PR. + ## NOTES 1. New feature 29 in v1.12.4 (Oct 2019) introduced zero-copy coercion. Our thinking is that requiring you to get the type right in the case of `0` (type double) vs `0L` (type integer) is too inconvenient for you the user. So such coercions happen in `data.table` automatically without warning. Thanks to zero-copy coercion there is no speed penalty, even when calling `set()` many times in a loop, so there's no speed penalty to warn you about either. However, we believe that assigning a character value such as `"2"` into an integer column is more likely to be a user mistake that you would like to be warned about. The type difference (character vs integer) may be the only clue that you have selected the wrong column, or typed the wrong variable to be assigned to that column. For this reason we view character to numeric-like coercion differently and will warn about it. If it is correct, then the warning is intended to nudge you to wrap the RHS with `as.()` so that it is clear to readers of your code that a coercion from character to that type is intended. For example :