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.

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

New failure with dev testthat #150

Closed
hadley opened this issue Sep 10, 2020 · 19 comments
Closed

New failure with dev testthat #150

hadley opened this issue Sep 10, 2020 · 19 comments

Comments

@hadley
Copy link

hadley commented Sep 10, 2020

FAILURE (testthat.style.R:71:3): auto style selection
...@NULL inherits from `StyleHtmlLightYb` not `StyleRaw`.

FAILURE (testthat.style.R:85:3): auto style selection
...@NULL inherits from `StyleHtmlLightYb` not `StyleAnsi8NeutralYb`.

FAILURE (testthat.style.R:92:3): auto style selection
...@NULL inherits from `StyleHtmlLightYb` not `StyleAnsi256LightYb`.

Would you mind taking a look please?

@brodieG
Copy link
Owner

brodieG commented Sep 10, 2020

I suspect it is again thinking it's running under Rstudio based on the message (i.e. Html output which would be used for the viewer, instead of ANSI CSI SGR for a standard terminal). I'll have some time next week to look at it, but in the meantime, are you sure the issue that came up in #149 was actually addressed in testthat?

Looking at 'local.R' I think I still see the RSTUDIO env var getting set to "0", but I haven't run the code to confirm that's actually the case and could definitely be wrong.

Thanks for reaching out.

@hadley
Copy link
Author

hadley commented Sep 11, 2020

Yes, but now that's not only called automatically in the 3rd edition: https://github.com/r-lib/testthat/blob/master/R/local.R#L62-L64

@brodieG
Copy link
Owner

brodieG commented Sep 11, 2020

I'm still seeing the variable set. I haven't looked into this in full, but I would have imagined this would not happen:

* installing *source* package ‘testthat’ ...
** using staged installation
** libs
clang -mmacosx-version-min=10.13 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -DCOMPILING_TESTTHAT  -I/usr/local/include   -fPIC  -Wall -g -O2  -fno-common -c init.c -o init.o
clang -mmacosx-version-min=10.13 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -DCOMPILING_TESTTHAT  -I/usr/local/include   -fPIC  -Wall -g -O2  -fno-common -c reassign.c -o reassign.o
clang++ -mmacosx-version-min=10.13 -std=gnu++11 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -DCOMPILING_TESTTHAT  -I/usr/local/include   -fPIC  -Wall -g -O2  -c test-catch.cpp -o test-catch.o
clang++ -mmacosx-version-min=10.13 -std=gnu++11 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -DCOMPILING_TESTTHAT  -I/usr/local/include   -fPIC  -Wall -g -O2  -c test-example.cpp -o test-example.o
clang++ -mmacosx-version-min=10.13 -std=gnu++11 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -DCOMPILING_TESTTHAT  -I/usr/local/include   -fPIC  -Wall -g -O2  -c test-runner.cpp -o test-runner.o
clang++ -mmacosx-version-min=10.13 -std=gnu++11 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L/Library/Frameworks/R.framework/Resources/lib -L/usr/local/lib -o testthat.so init.o reassign.o test-catch.o test-example.o test-runner.o -F/Library/Frameworks/R.framework/.. -framework R -Wl,-framework -Wl,CoreFoundation
installing to /Library/Frameworks/R.framework/Versions/4.0/Resources/library/00LOCK-testthat/00new/testthat/libs
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
*** copying figures
** building package indices
** installing vignettes
** testing if installed package can be loaded from temporary location
** checking absolute paths in shared objects and dynamic libraries
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (testthat)
Warning message:
In di() : Unable to unload package; it may not be loaded.
> library(testthat)
> test_that("hello", {browser()})
Called from: eval(code, test_env)
Browse[1]> Sys.get
Sys.getenv     Sys.getlocale  Sys.getpid     
Browse[1]> Sys.get
Sys.getenv     Sys.getlocale  Sys.getpid     
Browse[1]> Sys.getenv('RSTUDIO')
[1] "0"
Browse[1]> c
> packageVersion('testthat')
[1] ‘2.99.0.9000’
> system('git log -1')
commit 11866e255220b9667e0016150943fd41797a3cb6 (HEAD -> master, upstream/master)
Author: Hadley Wickham <h.wickham@gmail.com>
Date:   Sun Jul 5 08:57:19 2020 -0500

    Mention setup-/teardown- files in setup()/teardown() docs
    
    Fixes #1046
> 

If you're sure this is fixed don't worry about it, I'll look under the full test set-up to confirm that is the actual issue.

@brodieG
Copy link
Owner

brodieG commented Sep 11, 2020

Aside, I find it useful to increment my .9000 numbers to know for sure I'm on the correct dev version. I realize this is a bit harder to do when you have multiple people working on the tree, but just a thought.

@brodieG
Copy link
Owner

brodieG commented Sep 11, 2020

Actually, obviously something is wrong in how I'm updating my git repo as I'm realizing the version is quite old, so hold on a sec.

@hadley
Copy link
Author

hadley commented Sep 11, 2020

As a matter of policy, we only bump development versions when we need to use a specific feature in another package. If you want to check exactly which version you have installed, look at the sha.

@brodieG
Copy link
Owner

brodieG commented Sep 11, 2020

FWIW, my upstream was 'hadley/testthat' and it look like that stopped synchronizing.

I updated to the most recent version and I see:

> remotes::install_github('r-lib/testthat')
Downloading GitHub repo r-lib/testthat@HEAD
...snip...
** testing if installed package keeps a record of temporary installation path
* DONE (testthat)
> library(testthat)
> packageVersion('testthat')
[1] ‘2.99.0.9000’
> test_that('hello',browser())
Called from: eval(code, test_env)
Browse[1]> Sys.getenv('RSTUDIO')
[1] "0"
Browse[1]> c
── Skip (???): hello ───────────────────────────────────────────────────────────
Reason: empty test

Obviously this is run outside of Rstudio.

@hadley
Copy link
Author

hadley commented Sep 11, 2020

When I create a reprex I see:

testthat::test_that("", {
  print(Sys.getenv("RSTUDIO"))
  testthat::succeed()
})
#> [1] "1"

I think that's expected since it's run inside of RStudio and test_that() is not overriding it.

And when I run in a terminal I see:

> testthat::test_that("", {
+   print(Sys.getenv("RSTUDIO"))
+   testthat::succeed()
+ })
[1] ""

Did you restart R after reinstalling?

@brodieG
Copy link
Owner

brodieG commented Sep 11, 2020

Hmm, I hadn't, but now I did and still get:

testthat(master)$ R

R version 4.0.2 (2020-06-22) -- "Taking Off Again"
Copyright (C) 2020 The R Foundation for Statistical Computing
Platform: x86_64-apple-darwin17.0 (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

   Package WARN OK
1  diffobj      12
2    fansi    1 11
3    oshka      12
4 unitizer      12
5     vetr    1 11

cached CRAN status (46 mins old).

> library(testthat)
> grep('sha', packageDescription('testthat'), value=TRUE, ignore.case=TRUE)
named character(0)
> desc <- capture.output(packageDescription('testthat'))
> grep('sha', desc, value=TRUE, ignore.case=TRUE)
[1] "RemoteSha: 03377aca45982219052a629b255729ba04edf80c" 
[2] "GithubSHA1: 03377aca45982219052a629b255729ba04edf80c"
> test_that('', browser())
Called from: eval(code, test_env)
Browse[1]> Sys.getenv('RSTUDIO')
[1] "0"
Browse[1]> c
── Skip (???):  ────────────────────────────────────────────────────────────────
Reason: empty test

> 

@hadley
Copy link
Author

hadley commented Sep 11, 2020

Oh, you're running from inside the testthat folder? testthat has the third edition turned on, so you'll get different results there.

@brodieG
Copy link
Owner

brodieG commented Sep 11, 2020

Ah, good to know. Yes, I was because I was building my local copy originally instead of remotes::install_github.

@brodieG
Copy link
Owner

brodieG commented Sep 14, 2020

Sorry to sound like a broken record, but I remotes::install_github('r-lib/testthat'), restarted R, and then:

> test_that('', print(Sys.getenv('RSTUDIO')))
[1] "0"
── Skip (???):  ────────────────────────────────────────────────────────────────
Reason: empty test

It looks like 3rd ed is turned on for the remotes install:

> grep('sha|edition', capture.output(packageDescription('testthat')), value=TRUE, ignore.case=TRUE)
[1] "Config/testthat/edition: 3"                          
[2] "RemoteSha: fbbd667b533a0d1093f45b69ceeb6f713220f0d7" 
[3] "GithubSHA1: fbbd667b533a0d1093f45b69ceeb6f713220f0d7"

I know nothing about this edition business (so don't know if it is actually turned on). Is it the intent that 3rd ed will be on by default? If so, is it surprising that we're seeing the above? It seems not. I might be missing something entirely here.

@hadley
Copy link
Author

hadley commented Sep 14, 2020

You can read about it here: https://testthat.r-lib.org/articles/third-edition.html — it's activated on a package-by-package basis, and it's activated for testthat. To figure out what's going wrong with your package, you need to run it in a package that doesn't have the 3rd edition enabled.

@brodieG
Copy link
Owner

brodieG commented Sep 14, 2020

I'm not sure what you mean by "activated for testthat". I'm just using a test_that block, but running it in a directory that is not related to test_that (I wasn't previously).

I'll have time later this week to dig into what test_that is doing, but the simple test of adding:

writeLines(sprintf("\n\033[31mRSTUDIO=%s\033[m", Sys.getenv('RSTUDIO')))

To the file in question (style.R) and running the tests while in the diffobj source directory (I source a file that uses test_dir) produces (<<< added by me):

> source('run.R')
✔ |  OK F W S | Context
⠴ |   6       | style                                                           
RSTUDIO=0                             <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
✖ |  23 5     | style [2.9 s]                                                   
────────────────────────────────────────────────────────────────────────────────
Failure (testthat.style.R:51:3): auto style selection
...@NULL inherits from `StyleHtmlLightYb` not `StyleAnsi256LightYb`.

Failure (testthat.style.R:58:3): auto style selection
...@NULL inherits from `StyleHtmlLightYb` not `StyleAnsi8NeutralYb`.

Failure (testthat.style.R:72:3): auto style selection
...@NULL inherits from `StyleHtmlLightYb` not `StyleRaw`.

Failure (testthat.style.R:86:3): auto style selection
...@NULL inherits from `StyleHtmlLightYb` not `StyleAnsi8NeutralYb`.

Failure (testthat.style.R:93:3): auto style selection
...@NULL inherits from `StyleHtmlLightYb` not `StyleAnsi256LightYb`.
────────────────────────────────────────────────────────────────────────────────

══ Results ═════════════════════════════════════════════════════════════════════
Duration: 2.9 s

[ FAIL 5 | WARN 0 | SKIP 0 | PASS 23 ]
Error: Test failures

@brodieG
Copy link
Owner

brodieG commented Sep 14, 2020

Hmm, I tried again and now I don't get this... I guess now that I've seen both behaviors I can try to figure out what's going on.

@hadley
Copy link
Author

hadley commented Sep 14, 2020

It might be something to do with the way you run tests interacting with how testthat determines if you've opted-in to the third edition or not. As far as I can tell, this is the only package out of ~5000 revdeps with this problem.

@brodieG
Copy link
Owner

brodieG commented Sep 14, 2020

This is my fault. What really confused me is that running the tests changed the state of the RSTUDIO variable permanently, so any subsequent inspection would have the variable set.

I have a lazy mock in there that's tripping up local_reproducible_output->withr::local_envvar:

library(testthat)
Sys.getenv('RSTUDIO')
# [1] ""
test_that("",
  with_mock(Sys.getenv=function(...) NA_character_, expect_true(TRUE))
)
Sys.getenv('RSTUDIO')
# [1] "0"

While I don't love having to resubmit for this, I can hardly expect that withr should be robust to bad redefinitions of base functions.

Thanks for your patience with this.

@hadley
Copy link
Author

hadley commented Sep 14, 2020

No problems, glad you got to the bottom of it 😄

@brodieG
Copy link
Owner

brodieG commented Oct 6, 2020

V0.3.2 released fixes this.

@brodieG brodieG closed this as completed Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants