-
Notifications
You must be signed in to change notification settings - Fork 7
Add tests for translations in translations.R #28
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
Comments
I'm thinking there are two options for "indirect" translations:
test_that("ifnull() function translates correctly", {
SQL <- "ifnull(a, b)"
R <- "ifelse(is.na(a), b, a)"
expect_equal(parse_expression(SQL, tidy = FALSE), str2lang(R))
expect_equal(parse_expression(SQL, tidy = TRUE), str2lang(R))
})
test_that("ifnull() function translates correctly", {
a <- c(1, 2, NA)
b <- c(7, 8, 9)
SQL <- "ifnull(a, b)"
answer <- c(1, 2, 9)
untidy <- eval(parse_expression(SQL, tidy = FALSE))
tidy <- eval(parse_expression(SQL, tidy = TRUE))
expect_equal(untidy, answer)
expect_equal(tidy, answer)
}) I think I prefer the 2nd option, though it is more involved. Thoughts? |
Thanks for putting some thought into this. To reduce the repetition of the same boilerplate code in every test, we could define some helper functions. For example, we could have a trio of functions |
Regarding whether to (1) test the translation or (2) test the outcome: I think (1) is safer and more general. It eliminates the possibility that the particular input values used in the test coincidentally worked but will not work in general. |
There are data types, operators, functions, and aggregates that need testing. The thought process was laid out by Ian in this comment. How should we handle testing "generic" translations (where
tidy = FALSE
andtidy = TRUE
return the same results)? Here's my current idea:Maybe extracting the expressions would be better:
The text was updated successfully, but these errors were encountered: