Theme element unit tests #2081
Theme element unit tests #2081
Conversation
There was a problem hiding this comment.
Thank you @Meghansaha for the PR!!! I left a few comments to address.
| ) | ||
|
|
||
| # Set the theme | ||
| set_gtsummary_theme(my_theme_1) |
There was a problem hiding this comment.
Instead of setting a theme in semi-open code, let's use with_gtsummary_theme(), because it's well-confined to the operations that are called within it.
| # Test that the p-value has 3 digits# | ||
| expect_true( | ||
| inline_text(gts_1, variable = age, column = "p.value") |> | ||
| grepl(pattern = "p=0.\\d{3}", x = _) |
There was a problem hiding this comment.
This will be easier to read as an expect_equal(). Can we update to avoid the regex pattern matching?
| grepl(pattern = "p=0.\\d{3}", x = _) | ||
| ) | ||
| # Reset the theming# | ||
| reset_gtsummary_theme() |
There was a problem hiding this comment.
Because we're now using with_gtsummary_theme(), we can remove this
| ) | ||
|
|
||
| # Set the theme | ||
| set_gtsummary_theme(my_theme_2) |
There was a problem hiding this comment.
Use with_gtsummary_theme() as above
| test_that("pkgwide-fn:pvalue_fun works", { | ||
|
|
||
| # Test that the p-value has the correct digits and prefix# | ||
| expect_true( |
There was a problem hiding this comment.
as above, let's use expect_equal()
| # This only works when a theme is set explicitly | ||
| # And not when it is just temporarily set with 'with_gtsummary_theme' | ||
| # Is this intentional behavior? |
There was a problem hiding this comment.
The conversion happens in the print method, which doesn't run until we print. So you can update the above code with
with_gtsummary_theme(
my_theme_4,
trial |>
dplyr::select(death, trt) |>
tbl_summary(by = trt)
) |>
print()| # This only works when a theme is set explicitly | ||
| # And not when it is just temporarily set with 'with_gtsummary_theme' | ||
| # Is this intentional behavior? | ||
| expect_true(grepl("|:-", gts_4) |> any()) |
There was a problem hiding this comment.
This may be easier to read as a snapshot test
| expect_true(grepl("|:-", gts_4) |> any()) | ||
|
|
||
| # Reset the theme | ||
| reset_gtsummary_theme() |
| ) | ||
|
|
||
| ## pkgwide-str:theme_name------------------------------------------------------- | ||
| test_that("pkgwide-str:theme_name works", { |
There was a problem hiding this comment.
We may be able to test this with with_gtsummary_theme(). can you try to update to not directly call the theme?
|
|
||
|
|
||
| ## pkgwide-fun:pre_conversion--------------------------------------------------- | ||
| test_that("pkgwide-fun:pre_conversion works", { |
There was a problem hiding this comment.
Apply the changes listed above to this code chunk as well, please.
What changes are proposed in this pull request?
Added unit test coverage for pkgwide theme elements
If there is an GitHub issue associated with this pull request, please provide link.
#1959
Reviewer Checklist (if item does not apply, mark is as complete)
pkgdown::build_site(). Check the R console for errors, and review the rendered website.devtools::test_coverage()usethis::use_spell_check()runs with no spelling errors in documentationWhen the branch is ready to be merged into master:
NEWS.mdwith the changes from this pull request under the heading "# gtsummary (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (seeNEWS.mdfor examples).usethis::use_version(which = "dev")usethis::use_spell_check()again