insightsengineering
1299 do not show datanames error if there are transformers passed to modules
Fixes the top part of https://github.com/insightsengineering/teal/issues/1299
The current behavior raised a warning that datanames are not sufficient based on comparison of data@datanames and modules$datanames. This now have a new condition - warning is only displayed when the above is satisfied & there are no transformers.
Code for testing
Tested with
pkgload::load_all('teal.data')
pkgload::load_all('teal.slice')
pkgload::load_all('teal')
my_transformers <- list(
teal_transform_module(
label = "Custom transform for iris",
ui = function(id) {
ns <- NS(id)
tags$div(
numericInput(ns("n_rows"), "Subset n rows", value = 6, min = 1, max = 150, step = 1)
)
},
server = function(id, data) {
moduleServer(id, function(input, output, session) {
reactive({
within(data(),
{
iris <- head(iris, num_rows)
},
num_rows = input$n_rows
)
})
})
}
)
)
app1 <- init(
data = teal_data(iris = iris),
modules = example_module(datanames = c("iris", "mtcars"))
)
shinyApp(app1$ui, app1$server)
app2 <- init(
data = teal_data(iris = iris),
modules = example_module(datanames = c("iris", "mtcars"), transformers = my_transformers)
)
shinyApp(app2$ui, app2$server)
No transformers - warning
Transformers exist - no warning
Results of local tests
Results of local tests
> devtools::test()
ℹ Testing teal
[INFO] 2024-08-14 11:40:00.4037 pid:11652 token:[] teal You are using teal version 0.15.2.9052
✔ | F W S OK | Context
✔ | 10 | init [1.1s]
✔ | 6 129 | module_teal [57.3s]
✔ | 95 | modules
✔ | 7 | rcode_utils
✔ | 8 | report_previewer_module
✔ | 6 0 | shinytest2-data_summary
✔ | 3 0 | shinytest2-filter_panel
✔ | 3 0 | shinytest2-init
✔ | 5 0 | shinytest2-landing_popup
✔ | 4 0 | shinytest2-module_bookmark_manager
✔ | 4 0 | shinytest2-modules
✔ | 5 0 | shinytest2-reporter
✔ | 1 0 | shinytest2-show-rcode
✔ | 6 0 | shinytest2-teal_data_module
✔ | 2 0 | shinytest2-teal_slices
✔ | 1 0 | shinytest2-utils
✔ | 2 0 | shinytest2-wunder_bar
✔ | 16 | teal_data_module-eval_code
✔ | 4 | teal_data_module
✔ | 25 | teal_reporter
✔ | 15 | teal_slices-store
✔ | 18 | teal_slices
✔ | 36 | utils [9.7s]
✔ | 17 | validate_has_data
✔ | 36 | validate_inputs
══ Results ════════════════════════════════════════════════════════════════════════════
Duration: 74.8 s
── Skipped tests (48) ─────────────────────────────────────────────────────────────────
• need a fix in a .slicesGlobal (1): test-module_teal.R:1177:11
• testing depth 3 is below current testing specification 5 (42):
test-shinytest2-data_summary.R:2:3, test-shinytest2-data_summary.R:21:3,
test-shinytest2-data_summary.R:40:3, test-shinytest2-data_summary.R:62:3,
test-shinytest2-data_summary.R:92:5, test-shinytest2-data_summary.R:139:3,
test-shinytest2-filter_panel.R:5:3, test-shinytest2-filter_panel.R:32:3,
test-shinytest2-filter_panel.R:71:3, test-shinytest2-init.R:5:3,
test-shinytest2-init.R:15:3, test-shinytest2-init.R:70:3,
test-shinytest2-landing_popup.R:5:3, test-shinytest2-landing_popup.R:25:3,
test-shinytest2-landing_popup.R:43:3, test-shinytest2-landing_popup.R:67:5,
test-shinytest2-landing_popup.R:146:3,
test-shinytest2-module_bookmark_manager.R:5:3,
test-shinytest2-module_bookmark_manager.R:19:3,
test-shinytest2-module_bookmark_manager.R:33:3,
test-shinytest2-module_bookmark_manager.R:44:3, test-shinytest2-modules.R:5:3,
test-shinytest2-modules.R:44:3, test-shinytest2-modules.R:60:3,
test-shinytest2-modules.R:77:3, test-shinytest2-reporter.R:5:3,
test-shinytest2-reporter.R:25:3, test-shinytest2-reporter.R:48:3,
test-shinytest2-reporter.R:87:3, test-shinytest2-reporter.R:103:3,
test-shinytest2-show-rcode.R:5:3, test-shinytest2-teal_data_module.R:5:3,
test-shinytest2-teal_data_module.R:42:3, test-shinytest2-teal_data_module.R:78:3,
test-shinytest2-teal_data_module.R:130:3, test-shinytest2-teal_data_module.R:175:3,
test-shinytest2-teal_data_module.R:215:3, test-shinytest2-teal_slices.R:5:3,
test-shinytest2-teal_slices.R:48:3, test-shinytest2-utils.R:5:3,
test-shinytest2-wunder_bar.R:5:3, test-shinytest2-wunder_bar.R:24:3
• todo (5): test-module_teal.R:1442:7, test-module_teal.R:1449:5,
test-module_teal.R:1452:5, test-module_teal.R:1710:5, test-module_teal.R:1768:5
[ FAIL 0 | WARN 0 | SKIP 48 | PASS 416 ]
Code Coverage Summary
Filename Stmts Miss Cover Missing
------------------------------ ------- ------ ------- ----------------------------------------------------------------------------------------------------------------------------------------
R/checkmate.R 24 0 100.00%
R/dummy_functions.R 49 11 77.55% 30, 32, 44, 55-62
R/get_rcode_utils.R 12 0 100.00%
R/include_css_js.R 22 17 22.73% 12-38, 76-82
R/init.R 112 52 53.57% 107-114, 163-172, 174, 186-207, 218, 236-239, 246-253, 256-257, 259
R/landing_popup_module.R 25 25 0.00% 61-87
R/module_bookmark_manager.R 158 127 19.62% 47-68, 88-138, 143-144, 156, 203, 238-315
R/module_data_summary.R 193 68 64.77% 24-52, 94, 192, 232-272
R/module_filter_data.R 52 2 96.15% 22-23
R/module_filter_manager.R 230 57 75.22% 56-62, 73-82, 90-95, 108-112, 117-118, 291-314, 340, 367, 379, 386-387
R/module_init_data.R 108 11 89.81% 44-53, 67
R/module_nested_tabs.R 167 70 58.08% 33-121, 150, 200, 258, 297
R/module_snapshot_manager.R 216 146 32.41% 89-95, 104-113, 121-133, 152-153, 170-180, 184-199, 201-208, 215-230, 234-238, 240-246, 249-262, 265-273, 304-318, 321-332, 335-341, 355
R/module_teal_data.R 114 21 81.58% 42-48, 87-90, 114-123
R/module_teal_with_splash.R 12 12 0.00% 22-38
R/module_teal.R 141 92 34.75% 42-145, 182-183, 191
R/module_transform_data.R 56 32 42.86% 17-51
R/modules.R 162 21 87.04% 148-152, 207-210, 299-300, 352, 364-372
R/reporter_previewer_module.R 19 2 89.47% 30, 34
R/show_rcode_modal.R 24 24 0.00% 17-42
R/tdata.R 14 14 0.00% 19-61
R/teal_data_module-eval_code.R 24 0 100.00%
R/teal_data_module-within.R 7 0 100.00%
R/teal_data_module.R 42 0 100.00%
R/teal_data_utils.R 43 0 100.00%
R/teal_lockfile.R 56 22 60.71% 34-38, 43-51, 72, 91, 94-102, 109
R/teal_reporter.R 68 6 91.18% 69, 77, 125-126, 129, 146
R/teal_slices-store.R 29 0 100.00%
R/teal_slices.R 63 0 100.00%
R/TealAppDriver.R 353 353 0.00% 52-735
R/utils.R 193 0 100.00%
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 2890 1230 57.44%
Diff against main
Filename Stmts Miss Cover
-------------------- ------- ------ --------
R/teal_data_module.R +4 0 +100.00%
TOTAL +4 0 +0.06%
Results for commit: aa860354d0b6347e00acd52d3ed1e5f5d4509cf3
Minimum allowed coverage is 80%
:recycle: This comment has been updated with latest results
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Unit Tests Summary
1 files 25 suites 9m 27s :stopwatch: 254 tests 248 :white_check_mark: 6 :zzz: 0 :x: 529 runs 523 :white_check_mark: 6 :zzz: 0 :x:
Results for commit aa860354.
:recycle: This comment has been updated with latest results.
Unit Test Performance Difference
| Test Suite | $Status$ | Time on main |
$±Time$ | $±Tests$ | $±Skipped$ | $±Failures$ | $±Errors$ |
|---|---|---|---|---|---|---|---|
| shinytest2-reporter | 💔 | $70.78$ | $+1.52$ | $0$ | $0$ | $0$ | $0$ |
Additional test case details
| Test Suite | $Status$ | Time on main |
$±Time$ | Test Case |
|---|---|---|---|---|
| init | 👶 | $+0.03$ | init_does_not_throw_warning_when_datanames_in_modules_incompatible_w_datanames_in_data_and_there_are_transformers | |
| init | 💀 | $0.03$ | $-0.03$ | init_throws_warning_when_datanames_in_modules_incompatible_w_datanames_in_data |
| init | 👶 | $+0.03$ | init_throws_warning_when_datanames_in_modules_incompatible_w_datanames_in_data_and_there_is_no_transformers |
Results for commit 51abb890e13425fc309d0abb40e970ac4d8810ae
♻️ This comment has been updated with latest results.
Hey @gogonzo I added a test in init for the case with incomplete datanames but with transformers passed in modules. I also edited the text for the existing test that verified that in the previous scenario.
I tried to check if we have any more tests verifying this warning message displayed on UI, but couldn't find any. Maybe we could extend tests with one or two shinytest2 tests?
I also see a test in test-module_teal.R that uses teal_modules-module_1-validate_datanames-shiny_warnings. The test starts with testthat::it("throws warning when dataname is not available". I think we could add a test in there as well.
I also see a test in
test-module_teal.Rthat usesteal_modules-module_1-validate_datanames-shiny_warnings. The test starts withtestthat::it("throws warning when dataname is not available". I think we could add a test in there as well.
I don't know if it is feasible. This PR is about an init where init throws a warning and passes a data further to srv_teal. In test-module_teal.R you'll have to skip init and just pass insufficient data, which is already tested.
Yeah, but we also modified module_teal_data https://github.com/insightsengineering/teal/pull/1319/files#diff-8f20f15c7749d517fd977941dfda6eaa293268f76f57eb733bf4971273ea4e68R165 so the shiny_warnings is restricted with the extended condition. That's why I though an extra test https://github.com/insightsengineering/teal/pull/1319/commits/d2a658fa74f2286ec813bd7d1936987aa1cc4832 in there could be helpful
Yeah, but we also modified
module_teal_datahttps://github.com/insightsengineering/teal/pull/1319/files#diff-8f20f15c7749d517fd977941dfda6eaa293268f76f57eb733bf4971273ea4e68R165 so theshiny_warningsis restricted with the extended condition. That's why I though an extra test d2a658f in there could be helpful
Good point, could you please include test in test-teal_module.R?
Yes @gogonzo - it's already included https://github.com/insightsengineering/teal/commit/d2a658fa74f2286ec813bd7d1936987aa1cc4832 in test-module_teal.R
@gogonzo maybe there is a missunderstanding in what I thought we wanted to achieve with this issue. If any transformers are passed, then insufficient data warning will not be shown.
I treat the conditions from the initial card as AND condition
So the warning will be displayed if both are satisified. Here you have transformers in example_module so warning is omitted.
Sorry if I got this wrong. Happy to discuss when I'm back from holidays.
Here you have transformers in example_module so warning is omitted.
We shouldn't omit a warning if data returned to the module didn't produce sufficient datanames. Issue is to skip warning on init if transform is provided and not to ignore datanames warning completely when a module has any transformer.
Make sense @gogonzo and thank you for the clarification. Will be able to edit the PR today
Sorry for the initial extension of what we planned. Initially I provided restriction to init and to the UI of the module. I just removed the restriciton from the UI. The warning is not displayed in the init, but is visible in the UI.
Results of tests
[ FAIL 0 | WARN 0 | SKIP 48 | PASS 417 ]
Tested with
This app https://github.com/insightsengineering/teal/pull/1319#pullrequestreview-2239860749 and the app below
pkgload::load_all('teal.data')
pkgload::load_all('teal.slice')
pkgload::load_all('teal')
my_transformers <- list(
teal_transform_module(
label = "Custom transform for iris",
ui = function(id) {
ns <- NS(id)
tags$div(
numericInput(ns("n_rows"), "Subset n rows", value = 6, min = 1, max = 150, step = 1)
)
},
server = function(id, data) {
moduleServer(id, function(input, output, session) {
reactive({
within(data(),
{
iris <- head(iris, num_rows)
},
num_rows = input$n_rows
)
})
})
}
)
)
app1 <- init(
data = teal_data(iris = iris),
modules = example_module(datanames = c("iris", "mtcars"))
)
shinyApp(app1$ui, app1$server)
app2 <- init(
data = teal_data(iris = iris),
modules = example_module(datanames = c("iris", "mtcars"), transformers = my_transformers)
)
shinyApp(app2$ui, app2$server)
Print screens of functionality in action
app1 - no transformers - warning displayed
app2 - transformers exist - warning not displayed
app2 - transformers exist - warning displayed in UI
Second app - no warning on init, visible warning on UI
Thanks for the patience @gogonzo - just checked with 2 apps and run tests locally. This now only displays the warning in UI and hides the warning for the console when you run init.