#125 Write unit tests for shiny.i18n package #126
Conversation
There was a problem hiding this comment.
In general this is going in the right direction, but I have some comments in the code.
I also have a more general comment. I understand, that with the test descriptions you have followed the convention of the existing tests, but I want to ask you to provide more descriptive information. For example:
Instead of test_that("Test update_lang", { I would expect something like test_that("update_lang changes the selected language in session$userData$shiny.i18n$lang", {.
This is especially important for more complex tests like the one for the RStudio addin.
tests/testthat/test_automatic.R
Outdated
| test_that("translate_with_google_cloud", { | ||
| it("Should check if API is set", { |
There was a problem hiding this comment.
- I don't see a reason to use BDD language here, since there is only a single test.
- If you want to stick to BDD, use
describe-it- more details here: https://testthat.r-lib.org/reference/describe.html
There was a problem hiding this comment.
Thanks Kuba! Updated!
tests/testthat/test_automatic.R
Outdated
| withr::with_envvar( | ||
| new = c( | ||
| GL_AUTH = "None" | ||
| ), { | ||
| # Arrange | ||
| txt <- "Hello, how are you?" | ||
| target_lang <- "fr" | ||
| # Act | ||
| result <- evaluate_promise( | ||
| translate_with_google_cloud(txt, target_lang) | ||
| ) | ||
| # Assert | ||
| expect_equal(length(result$messages), 7) | ||
| }) |
There was a problem hiding this comment.
This test makes little sense. Since there is no authorization, the result is just information that there is no authorization. This would require end-to-end tests if we want to ensure the automated translation works.
I suggest checking if the error part in the tryCatch produces the desired message for unit tests. But for that, you should mock gl_translate function with a simple one that emits an error.
tests/testthat/test_preproc.R
Outdated
| test_that("Test create_translation_addin", { | ||
| temp <- tempfile() | ||
| # Mock the behavior of RStudio API calls | ||
| with_mock( | ||
| `rstudioapi::showDialog` = function(...) { | ||
| message("Mock: showDialog called") | ||
| TRUE | ||
| }, | ||
| `rstudioapi::getActiveDocumentContext` = function() { | ||
| message("Mock: getActiveDocumentContext called") | ||
| list(path = temp) | ||
| }, | ||
| `rstudioapi::showQuestion` = function(...) { | ||
| message("Mock: showQuestion called") | ||
| TRUE | ||
| }, | ||
| `create_translation_file` = function(path, format) { | ||
| # Mock the behavior of create_translation_file | ||
| expect_equal(path, temp) | ||
| expect_equal(format, "json") | ||
| }, | ||
| { | ||
| # Call the function | ||
| create_translation_addin() | ||
| } | ||
| ) | ||
|
|
||
| }) |
There was a problem hiding this comment.
I really like how you were able to use mocking here. Nice work!
One small comment - with_mock is marked as superseded. The alternatives are still in the experimental stage, so I'm ok with not using them, but it might be worth adding a comment about this.
There was a problem hiding this comment.
Thanks! I also saw this and tried the other approaches, but couldn't succeed and there were less examples to learn from
tests/testthat/test_ui.R
Outdated
| class(i18ntag), | ||
| c("shiny.tag.list", "list") | ||
| ) | ||
| expect_equal(length(i18ntag[[1]]$children), 2) |
There was a problem hiding this comment.
You are checking only the length. It would be great to also check the content. The first child should include translations, second should be the js file.
There was a problem hiding this comment.
I tried doing this with shin.tag with tags$script but couldn't make expect_equal work. Because of this, I set them as characters
tests/testthat/test_zzz.R
Outdated
| expect_equal(length(.i18_config), 3) | ||
| expect_equal( | ||
| names(.i18_config), | ||
| c("cultural_date_format", "cultural_bignumer_mark", "cultural_punctuation_mark") |
There was a problem hiding this comment.
Since the function takes values from this config file, you can also check if they are passed correctly.
tests/testthat/test_ui.R
Outdated
| # Assert | ||
| testServer(server, { | ||
| session$setInputs(selected_language = "pl") | ||
| expect_equal(reactive_language(), "pl") |
There was a problem hiding this comment.
This test makes no sense. You are only checking if the Shiny input and Shiny reactive are working, and you should test the functionality of shiny.i18n. Since this function 1. creates a reactiveVal in `session$userData and 2. sets its value using the chosen language, you should test that.
There was a problem hiding this comment.
Thank you! Updated
|
I had to do some updates in the code due to |
#125 Write unit tests for shiny.i18n package
Added unit tests for the functions:
translate_with_google_cloudcreate_translation_addinusei18nupdate_lang.onLoadCode coverage increased from 62% to 83%