239 utilize logger::log_shiny_input_change

[m7pr]

Similar to
insightsengineering/teal.modules.general#750
insightsengineering/teal.modules.clinical#227

[github-actions]

CLA Assistant Lite bot ✅ All contributors have signed the CLA

[m7pr]

Hi @danielinteractive just checking if this is ok with you to provide such logging for all shiny input changes?

[m7pr]

I have read the CLA Document and I hereby sign the CLA

[m7pr]

recheck

[github-actions]

Unit Tests Summary

 1 files  15 suites   8s ⏱️
56 tests 43 ✅ 13 💤 0 ❌
94 runs  81 ✅ 13 💤 0 ❌

Results for commit 4e3fb2d.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
adtteSpec	💚	$12.01$	$-3.84$	$-444$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
adtteSpec	💚	$8.26$	$-1.94$	h_km_mae_to_adtte_function_works_as_expected_with_a_single_gene

Results for commit 3ffd19f

♻️ This comment has been updated with latest results.

[danielinteractive]

thanks @m7pr , lgtm

[m7pr]

@pawelru check out tests failining here after introducing logger::log_shiny_input_changes. https://github.com/insightsengineering/teal.modules.hermes/actions/runs/9061416222/job/24893121013?pr=382#step:41:206
This functionality fails, when used in shiny::testServer

library(shiny)
server <- function(input, output, session) {
  logger::log_shiny_input_changes()
  x <- reactive(input$a * input$b)
}

testServer(server, {
  session$setInputs(a = 2, b = 3)
  stopifnot(x() == 6)
})

Error in logger::log_shiny_input_changes() :
No Shiny app running, it makes no sense to call this function outside of a Shiny app

[pawelru]

Hmmm... Very interesting finding. Looks that the core functionality is not prepared for handling the test. Please have a look at this: https://github.com/daroczig/logger/blob/2ff43d0cb87b0c0e896a165fdec20ca290a9142c/R/hooks.R#L149-L151 Can you prepare a PR that could enhance it? This and assume it got merged (and released) soon. Unfortunately I don't have control on this. In the past it took a while to get this completed but last time it was very smooth one.
Alternatively, you can introduce if-statement on logger::log_shiny_input_changes(). Actually let's do both.

[m7pr]

Hey @pawelru I created a PR in logger daroczig/logger#154 which overcomes the issue with inability to use log_shiny_input_changes function outside of Shiny App. I also extened log_shiny_input_changes so that we can provide custom messages, so that we can fullfill the need to append log-messages with function names for teal.slice insightsengineering/teal.slice#590

[m7pr]

Created an alternative PR that handles shiny in testing mode daroczig/logger#155

[m7pr]

Actually let's do both.

For our case then I think we need to change

logger::log_shiny_input_changes(input, namespace = "teal.modules.hermes")

into

if (shiny:isRunning()) {
  logger::log_shiny_input_changes(input, namespace = "teal.modules.hermes")
}

Once daroczig/logger#155 is merged, we can remove if (shiny:isRunning()) { and add session = session parameter.

[github-actions]

Code Coverage Summary

Filename              Stmts    Miss  Cover    Missing
------------------  -------  ------  -------  ------------------------------------------------------------------------------------------------------------------------------------
R/adtteSpec.R           171     124  27.49%   253-401
R/assaySpec.R            54      40  25.93%   104-147
R/barplot.R             188     152  19.15%   39-66, 129-280
R/boxplot.R             190     159  16.32%   40-67, 125-279
R/checkmate.R            18       9  50.00%   110-118
R/experimentSpec.R       91      58  36.26%   97, 215-284
R/forestplot.R          214     186  13.08%   58-92, 152-327
R/geneSpec.R            257     154  40.08%   154-169, 296-481
R/km.R                  208     174  16.35%   61-92, 156-326
R/pca.R                 367     284  22.62%   34-56, 165-477
R/quality.R             320     247  22.81%   18-110, 208-454
R/sampleVarSpec.R       237     105  55.70%   291, 300, 319-328, 334-341, 343, 351-363, 365-366, 368, 371, 379-383, 385-400, 405-429, 432-436, 438, 445-455, 457-458, 466, 511-528
R/scatterplot.R         181     148  18.23%   39-65, 127-273
R/utils.R                16       0  100.00%
R/volcanoplot.R         204     174  14.71%   34-57, 113-293
R/zzz.R                   2       2  0.00%    2-3
TOTAL                  2718    2016  25.83%

Diff against main

Filename              Stmts    Miss  Cover
------------------  -------  ------  -------
R/adtteSpec.R            +1      +1  -0.16%
R/assaySpec.R            +1      +1  -0.49%
R/barplot.R              +1      +1  -0.10%
R/boxplot.R              +1      +1  -0.09%
R/experimentSpec.R       +1      +1  -0.40%
R/forestplot.R           +1      +1  -0.06%
R/geneSpec.R             +1      +1  -0.16%
R/km.R                   +1      +1  -0.08%
R/pca.R                  +1      +1  -0.06%
R/quality.R              +1      +1  -0.07%
R/sampleVarSpec.R        +1      +1  -0.24%
R/scatterplot.R          +1      +1  -0.10%
R/volcanoplot.R          +1      +1  -0.07%
TOTAL                   +13     +13  -0.12%

Results for commit: 4e3fb2d

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[m7pr]

Created an issue, for the future, to keep in mind that the if statement might not be needed anymore at some point #383