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

Parallel tests use different language #1213

Closed
flying-sheep opened this issue Oct 31, 2020 · 23 comments · Fixed by #1258
Closed

Parallel tests use different language #1213

flying-sheep opened this issue Oct 31, 2020 · 23 comments · Fixed by #1258
Assignees
Labels
bug an unexpected problem or unintended behavior parallel ⇶

Comments

@flying-sheep
Copy link

flying-sheep commented Oct 31, 2020

I have Config/testthat/parallel: true in my DESCRIPTION

data_uris <- function(..., mime = '', encoding = 'base64', files) {
	stopifnot(length(list(...)) == 0L)
	vapply(
		files,
		function(f) base64enc::dataURI(mime = mime, encoding = encoding, file = f),
		character(1L))
}

test_that('Extra arguments to data_uris error', {
	expect_error(
		data_uris('one', mime = 'text/html'),
		'length(list(...)) == 0L is not TRUE',
		fixed = TRUE
	)
})

When I run it (edit: with german locale set), I get

Error (test_utils.r:3:2): Extra arguments to data_uris error
Error: length(list(...)) == 0L ist nicht TRUE
Backtrace:
 1. testthat::expect_error(...)
 7. repr:::f()
 8. repr::data_uris("one", mime = "text/html")
 9. base::stopifnot(length(list(...)) == 0L) R/utils.r:137:8
@gaborcsardi
Copy link
Member

I cannot reproduce this with testthat 3.0.0 from CRAN.

@gaborcsardi gaborcsardi added the reprex needs a minimal reproducible example label Oct 31, 2020
@gaborcsardi
Copy link
Member

I have put this code in testthat, in this branch: https://github.com/r-lib/testthat/tree/fix/issue-1213 and the tests run fine in parallel on macOS.

@flying-sheep Does this work for you?

@flying-sheep
Copy link
Author

Aha! The problem seems to be that other than in sequential mode, in parallel mode tests don’t get run in a normalized locale. Therefore I get ist nicht TRUE.

@flying-sheep flying-sheep changed the title Parallel mode in 3.0 fails with expect_error Parallel mode in 3.0 fails to set locale Nov 1, 2020
@gaborcsardi
Copy link
Member

Oh, that is a testthat bug indeed then!

@gaborcsardi gaborcsardi added bug an unexpected problem or unintended behavior and removed reprex needs a minimal reproducible example labels Nov 1, 2020
@gaborcsardi gaborcsardi self-assigned this Nov 1, 2020
@hadley
Copy link
Member

hadley commented Dec 1, 2020

@gaborcsardi Should we be setting LANGUAGE / LANG in local_reproducible_output()?

@hadley
Copy link
Member

hadley commented Dec 1, 2020

No that doesn't seem right, because it would be a rather large change. Why aren't the child processes inheriting LANG from the parent?

@gaborcsardi
Copy link
Member

Yeah, they do inherit all env vars. And testthat does not set the language afaik. So I am not sure why it is different in the main process. Let me try to reproduce this.

@hadley hadley changed the title Parallel mode in 3.0 fails to set locale Parallel mode in 3.0 fails to set language Dec 1, 2020
@hadley hadley changed the title Parallel mode in 3.0 fails to set language Parallel tests fail to set language Dec 1, 2020
@gaborcsardi
Copy link
Member

Where does testthat actually set the language?

@gaborcsardi
Copy link
Member

This is really weird:

f <- function(...) stopifnot(length(list(...)) == 0L)
f("foo")
#> Fehler in f("foo") : length(list(...)) == 0L is not TRUE

callr::r(f, list("foo"))
#> Fehler: callr subprocess failed: length(list(...)) == 0L ist nicht TRUE
#> Type .Last.error.trace to see where the error occured

stopifnot(length(list("foo")) == 0L)
#> Fehler: length(list("foo")) == 0L ist nicht TRUE

Why is the message is not TRUE for f(), but not in the subprocess, and also not for the direct stopifnot() call.

@gaborcsardi
Copy link
Member

Even simpler example:

f <- function() stopifnot(FALSE)
f()
#> Fehler in f() : FALSE is not TRUE

callr::r(f)
#> Fehler: callr subprocess failed: FALSE ist nicht TRUE
#> Type .Last.error.trace to see where the error occured

stopifnot(FALSE)
#> Fehler: FALSE ist nicht TRUE

@gaborcsardi
Copy link
Member

Probably boils down to this:

f <- function() stopifnot(FALSE)

f()
#> Fehler in f() : FALSE is not TRUE

do.call(f, list())
#> Fehler in (function ()  : FALSE ist nicht TRUE

@hadley
Copy link
Member

hadley commented Dec 1, 2020

WAT

@gaborcsardi
Copy link
Member

Started R with LANGUAGE=de R btw.

@gaborcsardi
Copy link
Member

It is probably some gettext thing, they match different strings, and one of them is translated. Maybe.

@gaborcsardi
Copy link
Member

In any case, I think people should either run tests in an English locale, or not match error messages.

IDK if testthat should set LANGUAGE or not. FWIW R CMD check seems to force English: https://github.com/wch/r-source/blob/a52d621364ea76cb039c3897b7c910e2e12c2f41/src/library/tools/R/check.R#L3736
So maybe testthat could do the same?

@gaborcsardi
Copy link
Member

So, to summarize, testthat does not set the language currently, irrespectively of whether the tests are parallel or not. (The title of the issue is a bit misleading currently.)

The behavior in #1213 (comment) seems to be a base R translation "thing" or a buglet. (It is the same from R 3.3 to R 4.1.)

R CMD check always uses English, so the original code by the OP should be fine in R CMD check.

It is possible that testthat should set English as well, for the whole test suite, or in the 3rd edition reproducible output.

@hadley hadley changed the title Parallel tests fail to set language Parallel tests use different language Dec 1, 2020
@hadley
Copy link
Member

hadley commented Dec 1, 2020

If R CMD check always uses English, then it's probably safe for local_reproducible_output() to set it too.

@gaborcsardi
Copy link
Member

R CMD check sets it for the whole check process, but it does not seem to be an issue to change back and forth within a session:

❯ Sys.setenv(LANGUAGE = "de")
❯ 1+""
Fehler in 1 + "" : nicht-numerisches Argument für binären Operator

❯ Sys.setenv(LANGUAGE = "en")
❯ 1+""
Error in 1 + "" : non-numeric argument to binary operator

❯ Sys.setenv(LANGUAGE = "de")
❯ 1+""
Fehler in 1 + "" : nicht-numerisches Argument für binären Operator

@hadley
Copy link
Member

hadley commented Dec 2, 2020

Does "en" work on windows too?

@gaborcsardi
Copy link
Member

Yep, same result. The linked base R check code runs on Windows as well.

@gaborcsardi
Copy link
Member

@flying-sheep So the error you see is a bug in base R: https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17998 I would also suggest not to test against the error messages of somebody else's packages (e.g. R core), because of these issues.

@hadley it seems like a big change to change the language in a patch release. But since R CMD check is doing it already, it is probably the right thing to do. We would only do it in the 3rd edition, that minimizes the impact as well. Do you want me to submit a PR?

@hadley
Copy link
Member

hadley commented Dec 2, 2020

@gaborcsardi I can do the PR — it's just a one line change, right?

@gaborcsardi
Copy link
Member

I think so, yeah. Well, plus the NEWS and the docs. You can use the fix/issue-1213 branch to test.

hadley added a commit that referenced this issue Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior parallel ⇶
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants