Follow-up changes to TealAppDriver #1653

m7pr · 2025-11-24T15:35:42Z

Companion to

bring back tests teal.modules.clinical#1441

Changes

Changed the way get_active_module_plot_output access namespaces
Fixed issue where set_active_ns did not set the namespace

Signed-off-by: Marcin <[email protected]>

github-actions · 2025-11-24T15:50:01Z

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ---------------------------------------------------------------------------------------
R/after.R                            59      21  64.41%   42-52, 64, 69, 77-79, 81-89, 100, 104-105
R/checkmate.R                        24       0  100.00%
R/dummy_functions.R                  61       2  96.72%   54, 56
R/include_css_js.R                   11       0  100.00%
R/init.R                            136       0  100.00%
R/module_bookmark_manager.R          99      62  37.37%   54-58, 78-133, 138-139, 151
R/module_data_summary.R             177       8  95.48%   40, 50, 205, 236-240
R/module_filter_data.R               64       0  100.00%
R/module_filter_manager.R           207       7  96.62%   116-117, 313, 340, 352, 359-360
R/module_init_data.R                 84       0  100.00%
R/module_nested_tabs.R              371      36  90.30%   163, 267-282, 302-306, 324, 361, 479-482, 486-489, 493-496
R/module_session_info.R              18       0  100.00%
R/module_snapshot_manager.R         272       9  96.69%   302-306, 373, 376-378
R/module_source_code.R               69       0  100.00%
R/module_teal_lockfile.R            131      53  59.54%   45-57, 60-62, 76, 86-88, 100-102, 110-119, 122, 124, 126-127, 142-146, 161-162, 177-186
R/module_teal_reporter.R            122       9  92.62%   60, 77-78, 81, 98, 128, 142, 144, 158
R/module_teal.R                     211       7  96.68%   127, 142-143, 183, 217, 260-261
R/module_transform_data.R           116       6  94.83%   48, 132-136
R/module_validate_error.R            73       0  100.00%
R/modules.R                         291      61  79.04%   170-174, 229-232, 357-377, 385, 391, 568-574, 587-595, 610-625, 658, 670-678
R/reporter_previewer_module.R        41      12  70.73%   41, 45, 68-85
R/teal_data_module-eval_code.R       23       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                 20       0  100.00%
R/teal_data_utils.R                  49       0  100.00%
R/teal_modifiers.R                   57       0  100.00%
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      63       0  100.00%
R/teal_transform_module.R            45       0  100.00%
R/TealAppDriver.R                   307     307  0.00%    50-651
R/utils.R                           291      48  83.51%   403-452, 540-549
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58       0  100.00%
R/zzz.R                              19       0  100.00%
TOTAL                              3637     648  82.18%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  --------
R/TealAppDriver.R       +9      +9  +100.00%
TOTAL                   +9      +9  -0.20%

Results for commit: 9f231ca

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2025-11-24T15:50:25Z

Unit Tests Summary

1 files 32 suites 2m 45s ⏱️
404 tests 343 ✅ 61 💤 0 ❌
620 runs 559 ✅ 61 💤 0 ❌

Results for commit 9f231ca.

♻️ This comment has been updated with latest results.

github-actions · 2025-11-24T15:50:32Z

Unit Test Performance Difference

Test Suite	$Status$	Time on `main`	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
module_teal	💔	$124.09$	$+9.18$	$0$	$0$	$0$	$0$

Results for commit ca193a6

♻️ This comment has been updated with latest results.

llrs-roche

I don't see any test failing and I agree this might be more sensible.

Is there a test case where the difference that makes on tests are clear?

What I noticed is that with TRUE it will have #:

app_driver$namespaces(FALSE)$module("a > img")
## [1] "teal-teal_modules-nav-summary_by_row_groups_table-module-a-plot_main > img"
app_driver$namespaces(TRUE)$module("a > img")
## [1] "#teal-teal_modules-nav-summary_by_row_groups_table-module-a-plot_main > img"

Couldn't that be an option of get_active_module_plot_output to control for this?

R/TealAppDriver.R

# Fix e2e tests Fixes #1440 Companion #1441 To verify the result of tests, you should do the following: 1. Install [this branch](insightsengineering/teal#1653) of teal 2. Install teal modules clinical on this version 3. ``` devtools::load_all() library(shinytest2) source("tests/testthat/helper-TealAppDriver.R") source("tests/testthat/helper-testing-depth.R") testthat::test_file("tests/testthat/test-shinytest2-tm_g_pp_patient_timeline.R") testthat::test_file("tests/testthat/test-shinytest2-tm_g_pp_vitals.R") testthat::test_file("tests/testthat/test-shinytest2-tm_g_pp_therapy.R") testthat::test_file("tests/testthat/test-shinytest2-tm_g_pp_adverse_events.R") ``` --------- Co-authored-by: osenan <[email protected]> Co-authored-by: Lluís Revilla <[email protected]> Co-authored-by: Marcin <[email protected]>

Copilot

Pull request overview

This PR modifies the TealAppDriver class to improve the robustness of namespace detection by changing when and how the active module namespace is determined. The changes address timing issues where namespace extraction was failing due to the page not being fully loaded or stable.

Key changes:

Moved set_active_ns() call to after wait_for_idle() in the initialization sequence
Updated get_active_module_plot_output() to use namespaces(TRUE) for proper CSS selector generation
Added comprehensive fallback logic in set_active_ns() to handle cases where active module detection fails

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

R/TealAppDriver.R

osenan

This PR fixes our issues of reviewing plot content and other problems while executing e2e tests.
I only added a suggestion to control better the additional Sys.sleep as suggested by @copilot.
I think in the future we should do a performance analysis of teal app and of teal_app_driver in order to optimize the performance execution of e2e tests.

R/TealAppDriver.R

Copilot · 2025-12-02T14:24:15Z

@osenan I've opened a new pull request, #1654, to work on those changes. Once the pull request is ready, I'll request review from you.

osenan · 2025-12-02T14:25:16Z

@copilot we do not need any new PR, please do not do any action for the moment

Copilot · 2025-12-02T14:25:27Z

@osenan I've opened a new pull request, #1655, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: Oriol Senan <[email protected]> Signed-off-by: Lluís Revilla <[email protected]>

averissimo

I have some concerns on this, please don't merge yet

averissimo · 2025-12-03T09:50:51Z

R/TealAppDriver.R

-          selector = sprintf(".teal-modules-tree li a.module-button[data-value='%s']", active_tab_inputs),
-          attribute = "href"
+      # If no active_module_id input found, find the selected/active tab button directly
+      if (!length(active_tab_inputs) || active_tab_inputs == "") {


This clause makes more sense if we start with the positive case when active_tab_inputs is available

averissimo · 2025-12-03T09:52:59Z

R/TealAppDriver.R

+        # Try one more time after a short wait - the page might still be loading
+        Sys.sleep(sleep_time)
+        active_wrapper_id <- private$extract_wrapper_id(
+          ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])"
+        )


It seems to make more sense to sleep and call the method again with sleep_time = sleep_time - 1 that skips this part if time is negative (or NULL to simply skip it).

averissimo · 2025-12-03T09:54:15Z

R/TealAppDriver.R

+        stop(sprintf(
+          paste0(
+            "Could not determine active module namespace. ",
+            "Make sure a module tab is selected and the page has finished loading. Found wrapper IDs: %s"
+          ),
+          if (length(found_ids) > 0) found_ids else "none"
+        ))


This is not good clear and simple code, please refactor and take advantage that stop pastes together all its elements.

Suggested change

stop(sprintf(

paste0(

"Could not determine active module namespace. ",

"Make sure a module tab is selected and the page has finished loading. Found wrapper IDs: %s"

),

if (length(found_ids) > 0) found_ids else "none"

))

stop(

"Could not determine active module namespace. ",

"Make sure a module tab is selected and the page has finished loading. Found wrapper IDs: "

if (length(found_ids) > 0) found_ids else "none"

))

averissimo · 2025-12-03T10:17:18Z

R/TealAppDriver.R

-        self$get_attr(
-          selector = sprintf(".teal-modules-tree li a.module-button[data-value='%s']", active_tab_inputs),
-          attribute = "href"
+      # If no active_module_id input found, find the selected/active tab button directly


I don't get why would DOM have the input id if the input does not exist in shiny.

How would that work?

Is it because of bslib doing some magic? This creates additional overhead when/if the navigation selectors are changed.

I am not sure I follow your thought. Note that self$get_attr is also used by extract_wrapper_id

llrs-roche

Double checking the logic of the changes I have some concerns too:

We omit some selectors on extract_wrapper_id and then we have to try so hard on set_active_ns to get new ones.
On set_active_ns we duplicate searching with extract_wrapper_id(".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])"). This suggest the order or logic could be improved.
set_active_ns has a waiting period that might affects public methods initialize, navigate_teal_tab, add_filter_var. This requires longer testing time without changing the application itself.

I think extract_wrapper_id could return multiple ids, and then it is the responsibility of set_active_ns to pick the right one of the valid ones.

averissimo · 2025-12-04T14:24:28Z

R/TealAppDriver.R

+      private$wait_for_page_stability()
+
      all_inputs <- self$get_values()$input
      active_tab_inputs <- all_inputs[grepl("-active_module_id$", names(all_inputs))]


We could use AppDriver$wait_for_value to make sure we have a value and then discard the rest of the added complexity.

The ignore = list(NULL, "") argument will allow us to avoid any of the initialization

spoiler... same can be done with javascript code and AppDriver$wait_for_js (let's say for is_visible())... I'm trying it out in widgets.

m7pr added 7 commits November 6, 2025 13:09

module_bookmark_manager tests

133d6f7

Merge branch 'main' of https://github.com/insightsengineering/teal

8cc34bb

Merge branch 'main' of https://github.com/insightsengineering/teal

7221be2

Merge branch 'main' of https://github.com/insightsengineering/teal

b328fac

Merge branch 'main' of https://github.com/insightsengineering/teal

4627fc7

Merge branch 'main' of https://github.com/insightsengineering/teal

ea0d6ef

bring tests

c7c960e

m7pr added the core label Nov 24, 2025

Delete tests/testthat/test-module_bookmark_manager.R

a32a9d9

Signed-off-by: Marcin <[email protected]>

revert 2 changes

2f6ffdf

m7pr mentioned this pull request Nov 24, 2025

bring back tests insightsengineering/teal.modules.clinical#1441

Open

llrs-roche approved these changes Nov 24, 2025

View reviewed changes

llrs-roche self-assigned this Nov 24, 2025

unsure if all this is needed

f8ea37e

m7pr commented Nov 25, 2025

View reviewed changes

R/TealAppDriver.R Show resolved Hide resolved

[skip style] [skip vbump] Restyle files

001ae6f

osenan mentioned this pull request Nov 26, 2025

wip: fix e2e test on tm_g_pp functions insightsengineering/teal.modules.clinical#1446

Merged

m7pr requested a review from Copilot November 27, 2025 15:26

Copilot started reviewing on behalf of m7pr November 27, 2025 15:27 View session

Copilot finished reviewing on behalf of m7pr November 27, 2025 15:28

Copilot AI reviewed Nov 27, 2025

View reviewed changes

simplify logic, based on feedback

7a2a004

m7pr changed the title ~~Change namespace() default behavior for get_active_input/output~~ Follow-up changes to TealAppDriver Nov 28, 2025

Fix lintr issue

8a5d57b

averissimo self-assigned this Dec 2, 2025

osenan approved these changes Dec 2, 2025

View reviewed changes

R/TealAppDriver.R Show resolved Hide resolved

R/TealAppDriver.R Show resolved Hide resolved

R/TealAppDriver.R Outdated Show resolved Hide resolved

R/TealAppDriver.R Outdated Show resolved Hide resolved

Copilot AI mentioned this pull request Dec 2, 2025

[WIP] Update TealAppDriver to improve Sys.sleep control and performance analysis #1654

Closed

4 tasks

osenan self-assigned this Dec 2, 2025

Copilot AI mentioned this pull request Dec 2, 2025

[WIP] Follow up changes to TealAppDriver #1655

Closed

llrs-roche and others added 3 commits December 2, 2025 15:42

Logic simplification

8aad92f

[skip style] [skip vbump] Restyle files

7aaa547

Apply suggestions from code review

a20a471

Co-authored-by: Oriol Senan <[email protected]> Signed-off-by: Lluís Revilla <[email protected]>

averissimo requested changes Dec 3, 2025

View reviewed changes

llrs-roche requested changes Dec 3, 2025

View reviewed changes

llrs-roche and others added 2 commits December 3, 2025 14:09

Simplify the logic of the function

6e662e8

[skip style] [skip vbump] Restyle files

9f231ca

averissimo reviewed Dec 4, 2025

View reviewed changes

Uh oh!

Follow-up changes to TealAppDriver #1653

Are you sure you want to change the base?

Follow-up changes to TealAppDriver #1653

Uh oh!

Conversation

m7pr commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Diff against main

Uh oh!

github-actions bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Tests Summary

Uh oh!

github-actions bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Performance Difference

Uh oh!

llrs-roche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

osenan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Dec 2, 2025

Uh oh!

osenan commented Dec 2, 2025

Uh oh!

Copilot AI commented Dec 2, 2025

Uh oh!

averissimo left a comment

Choose a reason for hiding this comment

Uh oh!

averissimo Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

averissimo Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

averissimo Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

averissimo Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

averissimo Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llrs-roche Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

llrs-roche left a comment

Choose a reason for hiding this comment

Uh oh!

averissimo Dec 4, 2025

Choose a reason for hiding this comment

m7pr commented Nov 24, 2025 •

edited

Loading

github-actions bot commented Nov 24, 2025 •

edited

Loading

github-actions bot commented Nov 24, 2025 •

edited

Loading

github-actions bot commented Nov 24, 2025 •

edited

Loading

averissimo Dec 3, 2025 •

edited

Loading