Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1163 extend TealAppDriver$new() and TealAppDriver$click() with $wait_for_idle() calls #1171

Merged
merged 14 commits into from
Mar 27, 2024

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Mar 21, 2024

Close #1163

  • removed all $wait_for_idle() calls after TealAppDrvier$new() as I extended the method to call $wait_for_idle()
  • removed all $wait_for_idle() calls after app$click() as I extended the method to call $wait_for_idle()
  • removed all $wait_for_idle() calls after app$navigate_teal_tab() as it is called inside the method already

Questions:

  • Should we add timeout parameter to initialize that will be passed to self$wait_for_idle() or add ... in self$wait_for_idle(...) at the end of the call. So that during TealAppDriver$new() you can pass timeout = .
  • app$navigate_teal_tab uses private$idle_timeout (self$wait_for_idle(timeout = private$idle_timeout)). Should we use private$idle_timeout in TealAppDrvier$new where we call $wait_for_idle(). Also, should we set private$idle_timeout in TealAppDrvier$new

@m7pr m7pr added the core label Mar 21, 2024
@m7pr m7pr marked this pull request as ready for review March 21, 2024 12:40
@m7pr m7pr changed the title 1163 extend TealAppDriver$initialize/new with wait_for_idle() call 1163 extend TealAppDriver$new() and TealAppDriver$click() with $wait_for_idle() calls Mar 21, 2024
@averissimo
Copy link
Contributor

averissimo commented Mar 21, 2024

  • Should we add timeout parameter to initialize that will be passed to self$wait_for_idle() or add ... in self$wait_for_idle(...) at the end of the call. So that during TealAppDriver$new() you can pass timeout = .

  • app$navigate_teal_tab uses private$idle_timeout (self$wait_for_idle(timeout = private$idle_timeout)). Should we use private$idle_timeout in TealAppDrvier$new where we call $wait_for_idle(). Also, should we set private$idle_timeout in TealAppDrvier$new

I think we should change the default value of the private element private$timeout to 20s instead of this non-default field that's already on main and reimplementing wait_for_idle.

@vedhav was there a specific rationale to create private$idle_timeout instead of changing the default value of private$timeout?

Copy link
Contributor

github-actions bot commented Mar 21, 2024

badge

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------
R/dummy_functions.R                  30      21  30.00%   21-33, 36-43
R/get_rcode_utils.R                  31       1  96.77%   50
R/include_css_js.R                   22       0  100.00%
R/init.R                             86      31  63.95%   108-115, 161-162, 164, 176-197, 227-228, 230
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_filter_manager.R           107      36  66.36%   37-43, 50-58, 67-72, 195, 200-213
R/module_nested_tabs.R              154      58  62.34%   39-112, 128, 180, 202, 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      33  56.58%   33-68, 100, 116
R/module_teal_with_splash.R         114       4  96.49%   110, 131, 197-198
R/module_teal.R                     106      29  72.64%   57, 68, 77, 150-151, 157, 176-207
R/modules.R                         152      26  82.89%   127-130, 147-151, 206-209, 291-292, 344, 356-364, 418-421
R/reporter_previewer_module.R        18       2  88.89%   30, 34
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                   249     249  0.00%    43-531
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                              12       8  33.33%   3-15
TOTAL                              1916     755  60.59%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  --------
R/TealAppDriver.R       +5      +5  +100.00%
TOTAL                   +5      +5  -0.16%

Results for commit: 13dc24e

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Mar 21, 2024

Unit Tests Summary

  1 files   28 suites   2m 21s ⏱️
234 tests 234 ✅ 0 💤 0 ❌
497 runs  497 ✅ 0 💤 0 ❌

Results for commit 13dc24e.

♻️ This comment has been updated with latest results.

@averissimo
Copy link
Contributor

Ps. I know it was me that suggested the reimplemented wait_for_idle, but changing the default private field might be enough to achieve the same purpose 😅

@m7pr
Copy link
Contributor Author

m7pr commented Mar 21, 2024

@averissimo no worries, but the part of adding $wait_for_idle() calls to the $new() and $click() methods is still valid : )

@averissimo
Copy link
Contributor

@averissimo no worries, but the part of adding $wait_for_idle() calls to the $new() and $click() methods is still valid : )

For sure! This will help us avoid a bunch of statements on tests 👍

R/TealAppDriver.R Outdated Show resolved Hide resolved
Signed-off-by: André Veríssimo <[email protected]>
Copy link
Contributor

github-actions bot commented Mar 25, 2024

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
shinytest2-filter_panel 💚 $19.86$ $-2.10$ $0$ $0$ $0$ $0$
shinytest2-reporter 💔 $14.64$ $+18.33$ $0$ $0$ $0$ $0$
shinytest2-teal_data_module 💔 $10.37$ $+1.07$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
shinytest2-filter_panel 💚 $7.18$ $-1.12$ e2e_filtering_a_module_specific_filter_is_not_refected_in_other_unshared_modules
shinytest2-filter_panel 💚 $7.10$ $-1.09$ e2e_filtering_a_module_specific_filter_is_refected_in_other_shared_module
shinytest2-reporter 💔 $8.94$ $+17.14$ e2e_adding_a_report_card_in_a_module_adds_it_in_the_report_previewer_tab

Results for commit 5441cd1

♻️ This comment has been updated with latest results.

R/TealAppDriver.R Outdated Show resolved Hide resolved
Copy link
Contributor

@vedhav vedhav left a comment

Choose a reason for hiding this comment

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

Thanks both of you for the changes! It's a nice improvement for the class. Just felt it will be nice to add this to the doc. Apart from this LGTM.

@averissimo averissimo merged commit 3bdb696 into main Mar 27, 2024
24 checks passed
@averissimo averissimo deleted the 1163_wait_for_idle@main branch March 27, 2024 16:21
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2024
@m7pr
Copy link
Contributor Author

m7pr commented Apr 2, 2024

Thanks guys for taking care of this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Adding wait_for_idle in the constructor of TealAppDriver
3 participants