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

Make testthat tests more reliable when called from the interpreter. #80

Merged
merged 1 commit into from Jul 19, 2013

Conversation

Projects
None yet
3 participants
@craigcitro
Contributor

craigcitro commented Jun 27, 2013

Currently, running test_dir on the testthat tests will fail on many systems when run interactively, due to what seems to be a race condition in the watcher code. (I'm waffling a little there, since the "race" seems to be 100% repeatable on some machines.) These patches clean that up, and do one more bit of cleanup-on-failure.

R/watcher.r Outdated
if (hash) {
sapply(files, digest::digest, file = TRUE)
vapply(files, safe_digest)

This comment has been minimized.

@hadley

hadley Jun 27, 2013

Member

I think that should be vapply(files, safe_digest, character(1))

This comment has been minimized.

@craigcitro

craigcitro Jun 27, 2013

Contributor

Ah, I'd forgotten to give the template. Done.

Any strong feelings on character(1) vs. just ""? I only ask because I use "" below, so I figure they might as well match -- I prefer character(1), especially below where it's mixed in with string arguments, but I could see a strong argument for "" on brevity.

This comment has been minimized.

@hadley

hadley Jun 27, 2013

Member

I prefer character(1) because it's more evocative of creating a data structure to fill - the important thing is that it's a character and length 1, not the contents.

R/watcher.r Outdated
#' @return a digest of the file, or NA if it doesn't exist.
#' @keywords internal
safe_digest <- function(path) {
result <- try(digest::digest(path, file = TRUE), silent = TRUE)

This comment has been minimized.

@hadley

hadley Jun 27, 2013

Member

I think I slightly prefer this style:

result <- NA_character_
try(result <- digest(path, file = TRUE), silent = TRUE)
result

(also note use of character NA and relying on imports to find digest)

This comment has been minimized.

@craigcitro

craigcitro Jun 27, 2013

Contributor

Done -- I really like folding the assignment into the try. I also didn't know about NA_character_. ;)

messages <- paste0("* ", messages, collapse = "\n")
}
failures <<- list()
tryCatch({

This comment has been minimized.

@hadley

hadley Jun 27, 2013

Member

When does this fail? And it seems odd to put failures <<- list() in the finally block. Can you explain more?

This comment has been minimized.

@craigcitro

craigcitro Jun 27, 2013

Contributor

So there are two parts: (1) why this might fail, and (2) why bother with the tryCatch.

  1. This one fails in a situation like this:

    test_that('error', { fake_name })
    Error in vapply(failures, "[[", "", "message") : values must be length 1,
    but FUN(X[[1]]) result is length 0

    The issue here is that a testthat::expectation object doesn't have a message field. This tickles the problem.

  2. Now once this fails, failures is never reset, so that expectation object stays in the list, whence successive calls from the interpreter will hit the same problem -- even in the case of a totally unrelated test. The issue is that you want failures reset no matter what, since this is getting called because we're stopping the reporter.

This comment has been minimized.

@hadley

hadley Jun 28, 2013

Member

Ah, ok, in that case I think it would be better to fix it in the underlying code so that message is always a character of length one. (Regardless, you don't need the finally, right? As long as the error is in the try block, all the code after it gets run)

This comment has been minimized.

@craigcitro

craigcitro Jul 3, 2013

Contributor

So I've changed the code up, and now stripped out the try. I should note that there's still some small chance that an error occurs -- if the user code raises something that confuses as.character, it'll become an error. That's a pretty unlikely corner, I think, so it shouldn't make a big difference.

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Jul 3, 2013

PTAL -- I think I've picked up everything.

@@ -33,7 +33,7 @@ StopReporter <- setRefClass("StopReporter", contains = "Reporter",
test <<- NULL
if (length(failures) == 0) return()
messages <- vapply(failures, "[[", "", "message")
messages <- vapply(failures, as.character, "")

This comment has been minimized.

@hadley

hadley Jul 18, 2013

Member

Should have said other comment here: character(1) instead of "" please

@hadley

This comment has been minimized.

Member

hadley commented Jul 18, 2013

Apart from one last tiny change, looks good!

@unDocUMeantIt

This comment has been minimized.

unDocUMeantIt commented Jul 18, 2013

this hoepfully fixes some issues i'm having right now: tests fail when run by "R CMD check", but if i run them from an R session, everything is fine. the package tested creates some files in a tempdir and i want to check for their existance.

i've noticed errors like
"Error in file(filename, "r", encoding = encoding) : cannot open the connection
Calls: local ... eval.parent -> eval -> eval -> eval -> eval -> source -> file
In addition: Warning message:
In file(filename, "r", encoding = encoding): cannot open file 'startup.Rs': No such file or directory"
my package functions use some setwd() calls. when i create a dummy "startup.Rs" file in the directory the functions jump to, these errors disappear. i have no clue what this file is supposed to do or why it's only missing when "R CMD check" is running the tests. anyway, the tests still fail after all.

in addition, testthat didn't load all dependencies, i had do manually add a require() call to the test suite. here's the package, if you want to try to replicate the problem: http://reaktanz.de/stuff/R/roxyPackage_0.03-11.tar.gz

hope this helps.

@hadley

This comment has been minimized.

Member

hadley commented Jul 18, 2013

@unDocUMeantIt That's unlikely to be the problem - can you please file a separate bug report?

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Jul 18, 2013

@hadley Made the last change -- do you want me to squash?

@hadley

This comment has been minimized.

Member

hadley commented Jul 18, 2013

Yes please!

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Jul 18, 2013

Should be squashed and ready to go.

@hadley

This comment has been minimized.

Member

hadley commented Jul 18, 2013

Sorry, forgot one last thing - can you please add a bullet point to NEWS briefly describing the change and thanking yourself?

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Jul 18, 2013

Will do -- I'm just walking out the door, but I'll --amend in some NEWS items to both pull requests after lunch.

Remove a race condition in `watch`.
Currently, `dir_state` can fail in unfortuate ways when a file disappears
between reading a directory listing and calculating a hash of its contents;
this patch fixes the problem by catching the failure and returning NA.
@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Jul 18, 2013

NEWS updated -- should be good to go.

hadley added a commit that referenced this pull request Jul 19, 2013

Merge pull request #80 from craigcitro/tests
Make testthat tests more reliable when called from the interpreter.

@hadley hadley merged commit 492b4e0 into r-lib:master Jul 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment