318: Increase Coverage beyond 80% #322
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
November 4, 2025 23:59
…to improve the coverage
Contributor
|
I have read the CLA Document and I hereby sign the CLA osenan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
Contributor
Unit Tests Summary 1 files 16 suites 9s ⏱️ Results for commit fc77004. ♻️ This comment has been updated with latest results. |
Contributor
Unit Test Performance Difference
Additional test case details
Results for commit 6c62921 ♻️ This comment has been updated with latest results. |
llrs-roche
requested changes
Nov 7, 2025
Contributor
There was a problem hiding this comment.
Many new tests! Thanks!
There is no code coverage report because the CI check fails. Please fix it and report the code coverage improvement to check that we get to the 80% goal.
In order to test everything you'll need to have TESTING_DEPTH=5 on your .Renviron (usethis::edit_r_environ("user")). This is to avoid running all the tests and be faster checking the package on some environments .
Some general comments:
- Do not add nocoverage tags.
- I see several
expect_error()that do not have the error message it expects.
Add just a couple of key words of the expected error. - I see some test with
local_edition(3)what does it bring that we can't test otherwise?
…er random elements
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
Contributor
|
Code Coverage SummaryDiff against mainResults for commit: fc77004 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Contributor
Author
|
Hi @llrs-roche I fixed the test. I removed the Thank you for all your comments, I was able to do it. I have not added expect_error messages for errors that are not generated in |
llrs-roche
reviewed
Nov 12, 2025
Contributor
llrs-roche
left a comment
llrs-roche
left a comment
There was a problem hiding this comment.
Thanks for increasing the test coverage via other ways. I've left some new comments.
- Add error messages for package functions - Skip tests if they use suggested packages - Improve disabled button test diff --git a/tests/testthat/test-draggable_buckets.R b/tests/testthat/test-draggable_buckets.R index 2decf62..2fbdbc7 100644 --- a/tests/testthat/test-draggable_buckets.R +++ b/tests/testthat/test-draggable_buckets.R @@ -104,11 +104,17 @@ testthat::test_that( ) testthat::test_that("fails when inputId is not from the expected type", { - expect_error(draggable_buckets(numeric(1), "test_label", "element_1", "bucket_1")) + expect_error( + draggable_buckets(numeric(1), "test_label", "element_1", "bucket_1"), + "Assertion on 'input_id' failed: Must be of type 'string', not 'double'." + ) }) testthat::test_that("fails when label is not from the expected type", { - expect_error(draggable_buckets("my_input_id", numeric(), "element_1", "bucket_1")) + expect_error( + draggable_buckets("my_input_id", numeric(), "element_1", "bucket_1"), + 'Assertion on \'inherits(label, "character") || inherits(label, "shiny.tag")\' failed: Must be TRUE.' + ) }) testthat::test_that("fails when buckets is not from the expected type", { diff --git a/tests/testthat/test-optionalSliderInputValMinMax_ui.R b/tests/testthat/test-optionalSliderInputValMinMax_ui.R index b2f24c3..c9d466f 100644 --- a/tests/testthat/test-optionalSliderInputValMinMax_ui.R +++ b/tests/testthat/test-optionalSliderInputValMinMax_ui.R @@ -36,6 +36,7 @@ testthat::test_that("we create label help for optionalSliderInputValMinMax", { testthat::test_that("snapshot test for optionalSliderInput", { testthat::local_edition(3) + testthat::skip_if_not_installed("whitr") withr::local_seed(1) testthat::expect_snapshot(as.character(optionalSliderInput("my slider", "my label", 0, 10, 2))) }) diff --git a/tests/testthat/test-standard_layout.R b/tests/testthat/test-standard_layout.R index 6f7b18e..9813a74 100644 --- a/tests/testthat/test-standard_layout.R +++ b/tests/testthat/test-standard_layout.R @@ -44,6 +44,7 @@ describe("Tests for standard_layout options", { it("checks snapshot with encoding and null forms", { # Given mock_layout <- standard_layout(output = mock_output, encoding = mock_form, forms = NULL) + testthat::skip_if_not_installed("withr") withr::local_seed(1) # Then testthat::local_edition(3) diff --git a/tests/testthat/test-verbatim_popup.R b/tests/testthat/test-verbatim_popup.R index 1d75926..70307ba 100644 --- a/tests/testthat/test-verbatim_popup.R +++ b/tests/testthat/test-verbatim_popup.R @@ -211,25 +211,6 @@ testthat::test_that("verbatim_popup_ui passes additional arguments to actionButt testthat::expect_true(grepl("custom-class", ui_char)) }) -# Test verbatim_popup_srv with testServer -testthat::test_that("verbatim_popup_srv creates observers correctly", { - shiny::testServer( - app = verbatim_popup_srv, - args = list( - verbatim_content = "Test content", - title = "Test Title", - style = FALSE, - disabled = shiny::reactiveVal(FALSE) - ), - expr = { - # Verify session is created - testthat::expect_type(session, "environment") - # Verify namespace function exists - testthat::expect_type(session$ns, "closure") - } - ) -}) - # Test verbatim_popup_srv with reactive content testthat::test_that("verbatim_popup_srv works with reactive verbatim_content", { test_content <- shiny::reactiveVal("Initial content") diff --git a/tests/testthat/test-verbatim_popup_ui.R b/tests/testthat/test-verbatim_popup_ui.R index b9ecf33..b4c37a4 100644 --- a/tests/testthat/test-verbatim_popup_ui.R +++ b/tests/testthat/test-verbatim_popup_ui.R @@ -86,8 +86,73 @@ testthat::test_that( } ) +testthat::test_that( + "e2e: verbatim_popup button can be disabled and enabled", + { + skip_if_too_deep(5) + + app <- shiny::shinyApp( + ui = bslib::page_fluid( + shinyjs::useShinyjs(), + verbatim_popup_ui( + id = "verbatim_popup", + button_label = "Show Popup" + ), + shiny::actionButton("toggle_disable", "Toggle Disable") + ), + server = function(input, output, session) { + disabled_state <- shiny::reactiveVal(FALSE) + + verbatim_popup_srv( + id = "verbatim_popup", + verbatim_content = "Test content", + title = "Test Title", + style = FALSE, + disabled = disabled_state + ) + + shiny::observeEvent(input$toggle_disable, { + disabled_state(!disabled_state()) + }) + } + ) + + app_driver <- shinytest2::AppDriver$new( + app, + name = "verbatim_popup_disabled", + variant = "app_driver_vpu_disabled" + ) + app_driver$wait_for_idle(timeout = default_idle_timeout) + + popup_button_selector <- "#verbatim_popup-button" + + # Initially button should be enabled (not disabled) + button_html <- app_driver$get_html(popup_button_selector) + testthat::expect_false(grepl("disabled", button_html, ignore.case = TRUE)) + + # Toggle to disabled state + app_driver$click("toggle_disable") + app_driver$wait_for_idle(timeout = default_idle_timeout) + + # Button should now be disabled + button_html <- app_driver$get_html(popup_button_selector) + testthat::expect_true(grepl("disabled", button_html, ignore.case = TRUE)) + + # Toggle back to enabled state + app_driver$click("toggle_disable") + app_driver$wait_for_idle(timeout = default_idle_timeout) + + # Button should be enabled again + button_html <- app_driver$get_html(popup_button_selector) + testthat::expect_false(grepl("disabled", button_html, ignore.case = TRUE)) + + app_driver$stop() + } +) + testthat::test_that("snapshot test for verbatim_popup_ui", { testthat::local_edition(3) + testthat::skip_if_not_installed("withr") withr::local_seed(1) testthat::expect_snapshot(verbatim_popup_ui("STH", "STH2")) })
llrs-roche
reviewed
Nov 13, 2025
Contributor
llrs-roche
left a comment
llrs-roche
left a comment
There was a problem hiding this comment.
I checked with André and other packages and we could move the package to the 3rd edition adding the line on DESCRIPTION.
Signed-off-by: Oriol Senan <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
Fixes #318
Changes
I have added checks for the ui components and modules. For the ui functions, I added input validation, component class and snapshot tests.
For the modules, I added input validation and small unit tests. All modules already have e2e tests.
The e2e tests were already existing but it seems they do not count for the coverage as they are skipped.
We have not reach 100% due to soft_deprecated functions and mainly verbatim_popup_ui which I was not able to increase that much from the previous state.