Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Follow-up changes to TealAppDriver#1653

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
m7pr wants to merge22 commits intomain
base:main
Choose a base branch
Loading
frombring_tests
Open

Follow-up changes to TealAppDriver#1653

m7pr wants to merge22 commits intomainfrombring_tests

Conversation

@m7pr
Copy link
Collaborator

@m7prm7pr commentedNov 24, 2025•
edited
Loading

Companion to

Changes

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

@m7prm7pr added the core labelNov 24, 2025
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actionsbot commentedNov 24, 2025•
edited
Loading

badge

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-105R/checkmate.R                        24       0  100.00%R/dummy_functions.R                  61       2  96.72%   54, 56R/include_css_js.R                   11       0  100.00%R/init.R                            136       0  100.00%R/module_bookmark_manager.R          99      54  45.45%   78-133R/module_data_summary.R             177       8  95.48%   40, 50, 205, 236-240R/module_filter_data.R               64       0  100.00%R/module_filter_manager.R           207       7  96.62%   116-117, 313, 340, 352, 359-360R/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-496R/module_session_info.R              18       0  100.00%R/module_snapshot_manager.R         272       9  96.69%   302-306, 373, 376-378R/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-186R/module_teal_reporter.R            122       9  92.62%   60, 77-78, 81, 98, 128, 142, 144, 158R/module_teal.R                     211       7  96.68%   127, 142-143, 183, 217, 260-261R/module_transform_data.R           116       6  94.83%   48, 132-136R/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-678R/reporter_previewer_module.R        41      12  70.73%   41, 45, 68-85R/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                   309     309  0.00%    50-657R/utils.R                           291      48  83.51%   403-452, 540-549R/validate_inputs.R                  32       0  100.00%R/validations.R                      58       0  100.00%R/zzz.R                              19       0  100.00%TOTAL                              3639     642  82.36%

Diff against main

Filename             Stmts    Miss  Cover-----------------  -------  ------  --------R/TealAppDriver.R      +11     +11  +100.00%TOTAL                  +11     +11  -0.25%

Results for commit:5bdede4

Minimum allowed coverage is80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actionsbot commentedNov 24, 2025•
edited
Loading

Unit Tests Summary

  1 files   33 suites   2m 41s ⏱️
415 tests 354 ✅ 61 💤 0 ❌
638 runs  577 ✅ 61 💤 0 ❌

Results for commit5bdede4.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actionsbot commentedNov 24, 2025•
edited
Loading

Unit Test Performance Difference

Test Suite$Status$Time onmain$±Time$$±Tests$$±Skipped$$±Failures$$±Errors$
module_teal💚$130.01$$-2.39$$0$$0$$0$$0$
shinytest2-disable_report💀$0.38$$-0.38$$-4$$-4$$0$$0$
shinytest2-disable_src💀$0.18$$-0.18$$-2$$-2$$0$$0$
shinytest2-show-rcode💀$0.29$$-0.29$$-3$$-3$$0$$0$
shinytest2-teal_modifiers💀$0.51$$-0.51$$-7$$-7$$0$$0$
Additional test case details
Test Suite$Status$Time onmain$±Time$Test Case
shinytest2-decorators👶$+0.04$unnamed
shinytest2-disable_report💀$0.09$$-0.09$Add_to_report_button_is_not_disabled_by_default.
shinytest2-disable_report💀$0.09$$-0.09$Report_button_is_active_on_a_nested_module_by_default
shinytest2-disable_report💀$0.09$$-0.09$Report_button_is_disabled_on_a_module_changed_by_disable_report_
shinytest2-disable_report💀$0.11$$-0.11$Report_button_is_disabled_on_nested_modules_changed_by_disable_report_
shinytest2-disable_src💀$0.09$$-0.09$Show_R_Code_button_is_disabled_on_a_module
shinytest2-disable_src💀$0.09$$-0.09$Show_R_Code_is_disabled_on_nested_modules_changed_with_disable_src
shinytest2-reporter👶$+0.01$unnamed
shinytest2-show-rcode💀$0.09$$-0.09$e2e_Module_with_Show_R_Code_has_code
shinytest2-show-rcode💀$0.09$$-0.09$e2e_Module_with_Show_R_Code_has_modal_with_two_dismiss_and_two_copy_to_clipboard_buttons
shinytest2-show-rcode💀$0.11$$-0.11$e2e_Module_with_Show_R_Code_initializes_with_visible_button
shinytest2-teal_data_module👶$+0.01$unnamed
shinytest2-teal_modifiers💀$0.07$$-0.07$e2e_add_landing_modal_displays_landing_modal_on_app_startup
shinytest2-teal_modifiers💀$0.07$$-0.07$e2e_add_landing_modal_modal_can_be_dismissed
shinytest2-teal_modifiers💀$0.07$$-0.07$e2e_combined_modifiers_displays_all_customizations_when_chained_together
shinytest2-teal_modifiers💀$0.09$$-0.09$e2e_modify_footer_displays_custom_footer_in_the_app
shinytest2-teal_modifiers💀$0.07$$-0.07$e2e_modify_header_displays_custom_header_in_the_app
shinytest2-teal_modifiers💀$0.07$$-0.07$e2e_modify_title_sets_custom_title_in_the_page_title_head_title_displays_custom_favicon_in_the_app
shinytest2-teal_modifiers💀$0.08$$-0.08$e2e_modify_title_sets_custom_title_in_the_page_title_head_title_displays_custom_title_in_the_app

Results for commite09e0ff

♻️ This comment has been updated with latest results.

Copy link
Contributor

@llrs-rochellrs-roche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 ofget_active_module_plot_output to control for this?

@llrs-rochellrs-roche self-assigned thisNov 24, 2025
m7pr added a commit to insightsengineering/teal.modules.clinical that referenced this pull requestNov 27, 2025
# Fix e2e testsFixes#1440 Companion#1441To verify the result of tests, you should do the following:1. Install [thisbranch](insightsengineering/teal#1653) of teal2. Install teal modules clinical on this version3.```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 <oriol@praenoscere.com>Co-authored-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com>Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
@m7prm7pr requested a review fromCopilotNovember 27, 2025 15:26
Copy link
Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Pull request overview

This PR modifies theTealAppDriver 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:

  • Movedset_active_ns() call to afterwait_for_idle() in the initialization sequence
  • Updatedget_active_module_plot_output() to usenamespaces(TRUE) for proper CSS selector generation
  • Added comprehensive fallback logic inset_active_ns() to handle cases where active module detection fails

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

@m7prm7pr changed the titleChange namespace() default behavior forget_active_input/outputFollow-up changes to TealAppDriverNov 28, 2025
@averissimoaverissimo self-assigned thisDec 2, 2025
Copy link
Contributor

@osenanosenan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 additionalSys.sleep as suggested by @copilot.
I think in the future we should do a performance analysis of teal app and ofteal_app_driver in order to optimize the performance execution of e2e tests.

Copy link
Contributor

CopilotAI commentedDec 2, 2025

@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.

@osenanosenan self-assigned thisDec 2, 2025
@osenan
Copy link
Contributor

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

Copilot reacted with eyes emoji

Copy link
Contributor

CopilotAI commentedDec 2, 2025

@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.

llrs-rocheand others added3 commitsDecember 2, 2025 15:42
Co-authored-by: Oriol Senan  <35930244+osenan@users.noreply.github.com>Signed-off-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com>
Copy link
Contributor

@averissimoaverissimo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

@llrs-rochellrs-roche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

  • We omit some selectors onextract_wrapper_id and then we have to try so hard onset_active_ns to get new ones.
  • Onset_active_ns we duplicate searching withextract_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 methodsinitialize,navigate_teal_tab,add_filter_var. This requires longer testing time without changing the application itself.

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

averissimo reacted with thumbs up emoji
Copy link
Contributor

@llrs-rochellrs-roche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I haven't tested yet but I left several comments to simplify the code. I'll test it and update the status of the review

averissimoand others added4 commitsDecember 16, 2025 15:58
Co-authored-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com>Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
m7prand others added3 commitsDecember 17, 2025 16:12
Add tests for bookmarking options and behavior in Shiny modules.Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Copy link
Contributor

@llrs-rochellrs-roche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

After discussing with Marcin about tests/testthat/test-module_bookmark_manager.R we decided to bring it back.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@averissimoaverissimoaverissimo requested changes

Copilot code reviewCopilotCopilot left review comments

@osenanosenanosenan approved these changes

@llrs-rochellrs-rochellrs-roche approved these changes

Requested changes must be addressed to merge this pull request.

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@m7pr@osenan@averissimo@llrs-roche
[8]ページ先頭
©2009-2025 Movatter.jp