Closed m7pr closed 5 months ago
CLA Assistant Lite bot ✅ All contributors have signed the CLA
Hi @danielinteractive just checking if this is ok with you to provide such logging for all shiny input changes?
I have read the CLA Document and I hereby sign the CLA
recheck
1 files 15 suites 8s :stopwatch: 56 tests 43 :white_check_mark: 13 :zzz: 0 :x: 94 runs 81 :white_check_mark: 13 :zzz: 0 :x:
Results for commit 4e3fb2de.
:recycle: This comment has been updated with latest results.
Test Suite | $Status$ | Time on main |
$±Time$ | $±Tests$ | $±Skipped$ | $±Failures$ | $±Errors$ |
---|---|---|---|---|---|---|---|
adtteSpec | 💚 | $12.01$ | $-3.84$ | $-444$ | $0$ | $0$ | $0$ |
Results for commit 3ffd19ff9c89513023a54334145e4902ee9aef8f
♻️ This comment has been updated with latest results.
@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
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.
Hey @pawelru I created a PR in logger
https://github.com/daroczig/logger/pull/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
https://github.com/insightsengineering/teal.slice/pull/590
Created an alternative PR that handles shiny in testing mode https://github.com/daroczig/logger/pull/155
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 https://github.com/daroczig/logger/pull/155 is merged, we can remove if (shiny:isRunning()) {
and add session = session
parameter.
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%
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: 4e3fb2de1e6077a0f930d3ca8447e72015fe5240
Minimum allowed coverage is 80%
:recycle: This comment has been updated with latest results
Created an issue, for the future, to keep in mind that the if statement might not be needed anymore at some point https://github.com/insightsengineering/teal.modules.hermes/issues/383
Similar to https://github.com/insightsengineering/teal.modules.general/pull/750 https://github.com/insightsengineering/teal.modules.clinical/issues/227