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

add wrapper for wait_for_value for output value. #1172

Merged
merged 11 commits into from
Mar 27, 2024

Conversation

kartikeyakirar
Copy link
Contributor

This PR introduces a wrapper for the wait_for_value method, specifically designed for output values.

@m7pr
Copy link
Contributor

m7pr commented Mar 21, 2024

ignore my previous comment!

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                   244     244  0.00%    29-506
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                              1911     750  60.75%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  --------
R/TealAppDriver.R      +10     +10  +100.00%
TOTAL                  +10     +10  -0.32%

Results for commit: d8a3352

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 1s ⏱️
234 tests 234 ✅ 0 💤 0 ❌
497 runs  497 ✅ 0 💤 0 ❌

Results for commit d8a3352.

♻️ This comment has been updated with latest results.

@averissimo averissimo self-assigned this Mar 21, 2024
Copy link
Contributor

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
shinytest2-filter_panel 💚 $20.11$ $-6.72$ $-2$ $0$ $0$ $+2$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
shinytest2-filter_panel 💚 $7.32$ $-3.33$ e2e_filtering_a_module_specific_filter_is_not_refected_in_other_unshared_modules
shinytest2-filter_panel 💚 $7.18$ $-3.20$ e2e_filtering_a_module_specific_filter_is_refected_in_other_shared_module

Results for commit 767a7c2

♻️ This comment has been updated with latest results.

Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

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

I think this still requires some improvements.

  • typo
  • better *_timeout to be used
  • character vector is hardcoded twice

Consider reducing the logic on this wrapper and just passing modified input/output/export and leave the deep checks to the parent method

    wait_for_active_module_value = function(input = rlang::missing_arg(),
                                            output = rlang::missing_arg(),
                                            export = rlang::missing_arg(),
                                            ...) {
      ns <- shiny::NS(self$active_module_ns())
      
      if (!rlang::is_missing(input) && checkmate::test_string(input, min.chars = 1)) input <- ns(input)
      if (!rlang::is_missing(output) && checkmate::test_string(output, min.chars = 1)) output <- ns(output)
      if (!rlang::is_missing(export) && checkmate::test_string(export, min.chars = 1)) export <- ns(export)

      
      self$wait_for_value(input = input, output = output, export = export, ...)
    }

@kartikeyakirar
Copy link
Contributor Author

Consider reducing the logic on this wrapper and just passing modified input/output/export and leave the deep checks to the parent method

I like this idea, thanks @averissimo! Do you think we should use self$get_active_module_input(input) instead of ns(input)? Also, we need to introduce the self$get_active_module_export method. WDYT?

@averissimo
Copy link
Contributor

averissimo commented Mar 27, 2024

Yes, we should! Great catch

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

# Conflicts:
#	R/TealAppDriver.R
@kartikeyakirar
Copy link
Contributor Author

Following our discussion with @averissimo, it appears that self$get_active_module_... methods are not useful as they return a value that is not being used. However, I have added the rest of the great suggestions here: a2f4392.

Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

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

Looks good.

One small comment about timeout. After that it can be merged :-)

Signed-off-by: kartikeya kirar <[email protected]>
@kartikeyakirar kartikeyakirar enabled auto-merge (squash) March 27, 2024 15:58
@kartikeyakirar kartikeyakirar merged commit 72a81da into main Mar 27, 2024
24 checks passed
@kartikeyakirar kartikeyakirar deleted the wrapper_method_for_TealAppDriver branch March 27, 2024 15:58
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2024
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.

3 participants