test.data.table() creates DT in .GlobalEnv

[mattdowle]

It shouldn't be touching .GlobalEnv

> require(data.table)  # 1.14.4 and before
> DT
Error: object 'DT' not found   # correct error
> test.data.table()
All 10039 tests (last 2163) in tests/tests.Rraw.bz2 completed ok in 36.8s elapsed (48.6s cpu)
> DT
   a
1: 1

Tests are already run in their own environment to isolate from .GobalEnv. That works well. But test 2036 uses source() which needs a local=TRUE adding.

[MichaelChirico]

It would be great if we could lockEnvironment(.GlovalEnv), but for some strange reason R apparently doesn't offer any way to unlockEnvironment() afterwards. There are apparently some ways to hack together an unlockEnvironment() in C, see e.g. here, but I'd rather not do that.

[mattdowle]

Good idea. Agree that's strange unlockEnvironment() doesn't exist.
Then how about save.image() before and after and binary compare. That could be a concern in a user's environment perhaps if they had large or sensitive data in .GlobalEnv and they ran test.data.table(). So it could be done in CRAN_Release.cmd and/or GLCI.

[mattdowle]

Using 1.14.4 I checked that those commands added to CRAN_Release would have found this DT being written, and that there are no others. No others in dev as of now either.

[MichaelChirico]

Agree it's probably best to handle in GLCI (with an environment variable) or in CRAN_release, because it's fine to lockEnvironment(.GlobalEnv) as long as the session exits after running the test.

[mattdowle]

Good point. In that case it would need to be lockEnvironment(.GlobalEnv, bindings=TRUE) otherwise it appears from reading ?lockEnvironment that existing variables could still be changed with the default bindings=FALSE.

I wonder if lockEnvironment(.GlobalEnv, bindings=TRUE) would prevent .Last and .Random.seed from being created/changed. Those aren't assigned by us but by base R. Those were picked up by the diff method so I excluded them in e956716.

[mattdowle]

Maybe lockEnvironment(.GlobalEnv, bindings=TRUE); unlockBinding(".Last", .GlobalEnv); unlockBinding(".Random.seed") would work; i.e. lock all bindings other than .Last and .Random.seed. They could be created first by dummy calls to the R functions that create them, or setting them to NULL might be enough just to ensure the bindings exist before locking the environment after which new bindings can't be created.

[MichaelChirico]

otherwise it appears... existing variables could still be changed...

Oh, good catch, yes, I assumed bindings=TRUE was the default.

They could be created first by dummy calls to the R functions that create them, or setting them to NULL

Either way, should be easy enough to play. The latter looks cleaner but the risk is if some R code assume's they're non-NULL or length()>0.