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

test.data.table() in non-English R session #3553

Merged
merged 5 commits into from
May 17, 2019
Merged

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented May 9, 2019

Closes #3039
Closes #630

This is an approach that would work to solve #3039.

But, it feels a bit hacky/manual, so filing this as a test balloon for now.

Less-than-ideal alternative would be something like:

was_translated = if (tryCatch(get(x, envir = new.env()), error = identity)$message == "object 'x' not found") FALSE else TRUE

then branch on if (was_translated) for subsequent gettext-dependent tests...

@codecov
Copy link

codecov bot commented May 9, 2019

Codecov Report

Merging #3553 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3553      +/-   ##
==========================================
+ Coverage   97.55%   97.55%   +<.01%     
==========================================
  Files          66       66              
  Lines       12667    12672       +5     
==========================================
+ Hits        12357    12362       +5     
  Misses        310      310
Impacted Files Coverage Δ
R/test.data.table.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8719a1f...d5d1cf7. Read the comment docs.

@jangorecki
Copy link
Member

I would go the other way, during test.data.table force R to use english error messages. Much less to support and we still test exactly what we need.

@MichaelChirico
Copy link
Member Author

MichaelChirico commented May 10, 2019

@jangorecki I believe that's what testthat tried but gave up due to caching:

r-lib/testthat#565 (comment)

@jangorecki
Copy link
Member

In such case I would close #3039 as won't fix, it unnecessarily adds complexity. We don't want to test error translations. Eventually we can fill R bugzilla issue for providing such capability.

@mattdowle mattdowle added this to the 1.12.4 milestone May 16, 2019
@mattdowle mattdowle changed the title [Exploratory] seed of attempt at #3039 test.data.table() in non-English R session May 17, 2019
Copy link
Member Author

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

# are R's messages being translated to a foreign language? #3039, #630
txt = eval(parse(text="tryCatch(mean(not__exist__), error = function(e) e$message)"), envir=.GlobalEnv)
foreign = txt != "object 'not__exist__' not found"
if (foreign) cat("\n**** This R session's language is not English. Each test will still check that the correct number of errors and/or\n",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice idea. excellent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. I did try (again) to change the language to English from within test.data.table() but it quickly got complicated and OS-specific. Plus with the link you found about caching the translations. Yep so I figured it would be cleaner to inform user how to change their language and rerun if they want the full-strength test, otherwise just checking the number of errors/warnings is still pretty fine.

@MichaelChirico
Copy link
Member Author

MichaelChirico commented May 17, 2019 via email

@Rdatatable Rdatatable deleted a comment from codecov bot May 17, 2019
@mattdowle mattdowle merged commit 5bc17d8 into master May 17, 2019
@mattdowle mattdowle deleted the foreign_messages branch May 17, 2019 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants