Introduce shinytest2

[vedhav]

Part of https://github.com/insightsengineering/coredev-tasks/issues/503

[vedhav]

What is the overhead of starting headless browser here? Is it seconds or fractions of seconds?

It takes about 2 seconds to initialize theAppDriver object. I see your point in splitting the tests into smaller and more precise test desc. I'm hesitant about starting them outside the test_that scope as we will add a skip_on_cran in every shinytest2 tests. I think ~2 seconds of app start is not that bad.

[gogonzo]

It takes about 2 seconds to initialize theAppDriver object ... I'm hesitant about starting them outside the test_that scope as we will add a skip_on_cran in every shinytest2 tests. I think ~2 seconds of app start is not that bad.

Let's do this inside test_that - I think it is more readable.

I see your point in splitting the tests into smaller and more precise test desc.

The point is to minimize time to detect failure and understand what is tested and what not. This file will soon have thousands of lines and imprecise test_that description (which doesn't describe expectation) will make it very hard for us developers to track the cause of the bug and monitor true-coverage. I suggest to be as precise as possible even if it affects performance (but in the same time shinytest2 can't take 10 minutes - or can?)

[averissimo]

I've found it helpful to create a custom helper to define namespaces with an arbitrary number of arguments. Instead of building it manually every time.

This could be integrated with get_active_ns so that it returns a function instead of a string.

WDYT? @vedhav

helper_ns <- function(id) {
  function(..., .css_prefix = "") {
    args <- rlang::list2(...)
    checkmate::assert_list(args, types = "character")
    (shiny::NS(sprintf("%s%s", .css_prefix, id)))(paste(args, collapse = "-"))
  }
}

ns <- helper_ns("teal-main_ui-filter_manager")

a_name <- "snapshot_manager"
output_id <- ns("filter_manager", a_name, "snapshot_add")
output_id
#> [1] "teal-main_ui-filter_manager-filter_manager-snapshot_manager-snapshot_add"

selector <- ns("filter_manager", "snapshot_manager", "snapshot_add", .css_prefix = "#")
selector
#> [1] "#teal-main_ui-filter_manager-filter_manager-snapshot_manager-snapshot_add"

[vedhav]

This could be integrated with get_active_ns so that it returns a function instead of a string.

Thanks @averissimo it can be a new component to that function. Do you want to do the honors or shall I add it?

[averissimo]

@vedhav I can take the honors :laughing:

[vedhav]

@averissimo Thanks for branching off of this and providing the helper_NS and the namespace for the filter_manager I've merged your changes here inside the R6 class. Please have a look.

[github-actions[bot]]

CLA Assistant Lite bot ✅ All contributors have signed the CLA

[cicdguy]

I have read the CLA Document and I hereby sign the CLA

You silly CLA bot and your silly rules!

[cicdguy]

I have read the CLA Document and I hereby sign the CLA

[cicdguy]

I have read the CLA Document and I hereby sign the CLA

You silly CLA bot and your silly rules!

Apparently I can't say mean stuff while agreeing to the CLA :(

[vedhav]

I have read the CLA Document and I hereby sign the CLA

[averissimo]

I have read the CLA Document and I hereby sign the CLA

[averissimo]

Apparently I can't say mean stuff while agreeing to the CLA :(

@cicdguy that's no fun!!

Before I signed the list of contributors was incorrectly formatted with the users that have already signed

[cicdguy]

@averissimo - yes I saw this too. I'll report this to the CLA Assistant maintainers.

[m7pr]

I have read the CLA Document and I hereby sign the CLA

[vedhav]

@averissimo What do you think about writing a wrapper over TealAppDriver$helper_NS method as we're constantly using it for:

ns <- app$helper_NS(
    app$filter_manager_ns(),
    "filter_manager",
    "snapshot_manager"
  )

Would be nice to do something simple like this:

ns <- NS(app$snapshot_manager_ns())

Also, it feels weird to have two namespaces with the same id filter_manager. Perhaps there is something we can change in teal? Is there a good reason to have these two namespaces?

[github-actions[bot]]

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------
R/dummy_functions.R                  30       0  100.00%
R/get_rcode_utils.R                  31       1  96.77%   50
R/include_css_js.R                   22       0  100.00%
R/init.R                             86      25  70.93%   108-115, 161-162, 164, 179-185, 192-197, 228
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_filter_manager.R           107      29  72.90%   50-58, 67-72, 195, 200-213
R/module_nested_tabs.R              154       3  98.05%   47, 128, 228
R/module_snapshot_manager.R         209     157  24.88%   87-99, 127-136, 140-152, 154-161, 168-182, 186-188, 190-195, 198-208, 211-227, 236-251, 265-288, 291-302, 305-311, 325, 343-366
R/module_tabs_with_filters.R         76       0  100.00%
R/module_teal_with_splash.R         114       2  98.25%   110, 131
R/module_teal.R                     106       1  99.06%   57
R/modules.R                         153      24  84.31%   127-130, 147-151, 206-210, 292-293, 345, 357-365
R/reporter_previewer_module.R        18       0  100.00%
R/show_rcode_modal.R                 19      19  0.00%    17-36
R/tdata.R                            53       1  98.11%   154
R/teal_data_module-eval_code.R       27       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                  6       0  100.00%
R/teal_reporter.R                    62       5  91.94%   69, 118-119, 122, 139
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      59      12  79.66%   137-150
R/TealAppDriver.R                   194      61  68.56%   66-77, 124-127, 135, 146-147, 155, 167-168, 190-196, 278-325, 361, 394
R/utils.R                           173       1  99.42%   255
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   110-377
R/zzz.R                              11       7  36.36%   3-14
TOTAL                              1861     410  77.97%

Diff against main

Filename                         Stmts    Miss  Cover
-----------------------------  -------  ------  -------
R/dummy_functions.R                  0     -21  +70.00%
R/init.R                             0      -6  +6.98%
R/module_filter_manager.R            0      -7  +6.54%
R/module_nested_tabs.R               0     -55  +35.71%
R/module_tabs_with_filters.R         0     -33  +43.42%
R/module_teal_with_splash.R          0      -2  +1.75%
R/module_teal.R                      0     -28  +26.42%
R/modules.R                          0      -3  +1.96%
R/reporter_previewer_module.R        0      -2  +11.11%
R/TealAppDriver.R                 +194     +61  +68.56%
R/utils.R                           +1       0  +0.00%
TOTAL                             +195     -96  +8.34%

Results for commit: 66dd0b915aea0afefd1ee72d4ceaf8032e1787ef

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

[github-actions[bot]]

Unit Tests Summary

  1 files   25 suites   1m 23s :stopwatch: 224 tests 224 :white_check_mark: 0 :zzz: 0 :x: 462 runs  462 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 66dd0b91.

:recycle: This comment has been updated with latest results.

[github-actions[bot]]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
shinytest2-filter_panel	👶		$+19.08$	$+5$	$0$	$0$	$0$
shinytest2-init	👶		$+6.61$	$+5$	$0$	$0$	$0$
shinytest2-modules	👶		$+16.79$	$+6$	$0$	$0$	$0$
shinytest2-reporter	👶		$+14.38$	$+4$	$0$	$0$	$0$
shinytest2-teal_slices	👶		$+12.51$	$+19$	$0$	$0$	$0$
shinytest2-utils	👶		$+3.34$	$+4$	$0$	$0$	$0$

Additional test case details

| Test Suite | $Status$ | Time on `main` | $±Time$ | Test Case | |:-----|:----:|:----:|:----:|:-----| | shinytest2-filter_panel | 👶 | | $+6.85$ | e2e_filtering_a_module_specific_filter_is_not_refected_in_other_unshared_modules | | shinytest2-filter_panel | 👶 | | $+6.80$ | e2e_filtering_a_module_specific_filter_is_refected_in_other_shared_module | | shinytest2-filter_panel | 👶 | | $+5.43$ | e2e_module_content_is_updated_when_a_data_is_filtered_in_filter_panel | | shinytest2-init | 👶 | | $+3.35$ | e2e_init_creates_UI_containing_specified_title_favicon_header_and_footer | | shinytest2-init | 👶 | | $+3.26$ | e2e_teal_app_initializes_with_no_errors | | shinytest2-modules | 👶 | | $+2.87$ | e2e_all_the_nested_teal_modules_are_initiated_as_expected | | shinytest2-modules | 👶 | | $+3.15$ | e2e_filter_panel_is_not_displayed_when_datanames_is_NULL | | shinytest2-modules | 👶 | | $+3.25$ | e2e_filter_panel_only_shows_the_data_supplied_using_datanames | | shinytest2-modules | 👶 | | $+3.39$ | e2e_filter_panel_shows_all_the_datasets_when_datanames_is_all | | shinytest2-modules | 👶 | | $+4.12$ | e2e_the_module_server_logic_is_only_triggered_when_the_teal_module_becomes_active | | shinytest2-reporter | 👶 | | $+8.83$ | e2e_adding_a_report_card_in_a_module_adds_it_in_the_report_previewer_tab | | shinytest2-reporter | 👶 | | $+2.80$ | e2e_reporter_tab_is_created_when_a_module_has_reporter | | shinytest2-reporter | 👶 | | $+2.75$ | e2e_reporter_tab_is_not_created_when_a_module_has_no_reporter | | shinytest2-teal_slices | 👶 | | $+4.62$ | e2e_teal_slices_filters_are_initialized_when_global_filters_are_created | | shinytest2-teal_slices | 👶 | | $+7.89$ | e2e_teal_slices_filters_are_initialized_when_module_specific_filters_are_created | | shinytest2-utils | 👶 | | $+3.34$ | e2e_show_hide_hamburger_works_as_expected |

Results for commit 46c60452b892fad42193603944bb640d987d28df

♻️ This comment has been updated with latest results.

[averissimo]

@averissimo What do you think about writing a wrapper over TealAppDriver$helper_NS method as we're constantly using it for:

@vedhav sounds good! Right now all it's being used the same as NS so we can scrap it :+1:

I'm creating a PR

[m7pr]

Hey @vedhav hey @averissimo, do you already have a function that merges the name of the element with the name of the current active namespace?

>   sprintf2 <- function(x, y = app$active_module_ns()){
+     sprintf("#%s-%s", y, x)
+   }
> sprintf2("button")
[1] "#teal-main_ui-root-module_with_verbatim_popup-module-button"
> app$active_module_ns()
[1] "teal-main_ui-root-module_with_verbatim_popup-module"

[m7pr]

Is this helper_ns any of help?

[m7pr]

Whoa. Great job