Fix broken examples

[vedhav]

Closes insightsengineering/teal.slice#492

We don't accept data in module UI anymore & there was a missing dataname in one example.

[github-actions]

Code Coverage Summary

Filename              Stmts    Miss  Cover    Missing
------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------
R/adtteSpec.R           170     123  27.65%   253-400
R/assaySpec.R            53      39  26.42%   104-146
R/barplot.R             173     140  19.08%   38-62, 120-260
R/boxplot.R             176     148  15.91%   39-63, 116-261
R/checkmate.R            18       9  50.00%   110-118
R/experimentSpec.R       90      57  36.67%   97, 215-283
R/forestplot.R          200     175  12.50%   57-88, 143-309
R/geneSpec.R            256     153  40.23%   154-169, 296-480
R/km.R                  192     161  16.15%   60-89, 148-303
R/pca.R                 357     276  22.69%   33-53, 158-463
R/quality.R             302     232  23.18%   18-107, 200-432
R/sampleVarSpec.R       236     104  55.93%   299, 318-327, 333-340, 342, 350-362, 364-365, 367, 370, 378-382, 384-399, 404-428, 431-435, 437, 444-454, 456-457, 465, 510-527
R/scatterplot.R         171     141  17.54%   38-62, 119-259
R/utils.R                16       0  100.00%
R/volcanoplot.R         196     167  14.80%   33-54, 107-280
R/zzz.R                   1       1  0.00%    2
TOTAL                  2607    1926  26.12%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 01b2ca3

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[kartikeyakirar]

Hey @vedhav this look great, I came across some examples of files in the directory that use data as a UI argument. Could you please take a look and let me know if we need it? if yes then we need to update those as well

testthat/sampleVarSpec/app.r
testthat/geneSpec/app.r
testthat/experimentSpec/app.r
testthat/assaySpec/app.r
testthat/adtteSpec/app.r

also in design director

[vedhav]

Let's change the app files inside the tests directory as those apps are broken. However, it's a part of some shinytest2 or end-to-end test implementation that is not active anymore. Implementing those tests can be part of an other task. However, let's fix the apps at least so there are no "incorrect" examples of custom modules lying around the package. Same for design/.

[danielinteractive]

thanks @vedhav , can we please include the test updates as suggested in this PR? Then we can get to a clean main branch again after merging this. The tests are still very relevant and used in regularly run integration tests (@cicdguy for fyi)

[github-actions]

Unit Tests Summary

  1 files   15 suites   11s ⏱️
 56 tests  43 ✅ 13 💤 0 ❌
538 runs  525 ✅ 13 💤 0 ❌

Results for commit a46ab68.

♻️ This comment has been updated with latest results.

[vedhav]

@danielinteractive Can you please check if we can remove the design? They don't seem like they are relevant anymore, at least most of them. Also, asking because there is a use of CDSE over there.

When it comes to testing have a plan to implement framework-wide testing. And we will make the changes to this package when we get to it. I don't think these apps are regularly run right now because they are failing and we would have caught the test failures in that case (I might be wrong. But, I don't see them running anywhere).

These test app.R files just seem like they are running the example of the modules, if this is the case I think they can be completely removed and we can run them using example("tm_*", "teal.modules.hermes") or just running the sample_tm_* without needing to maintain multiple app.R files. Please have a look at this draft PR in tmc with a POC implementation of this test. Or do we need these app.R files because they do something?

[danielinteractive]

@vedhav We need to keep the design folder, this is part of Statistical Engineering standard workflow. Note that this is part of the repo, not part of the R package.

Re: the app.R in the test directories, these are needed for the shinytest2 tests, see e.g. https://github.com/insightsengineering/teal.modules.hermes/blob/main/tests/testthat/test-adtteSpec.R#L195
This is executed (or should be executed) and we have this in place in agreement with the IDR team, @insightsengineering/idr .

Again, we will need to clean up those test apps before this PR can be merged.

[vedhav]

Thanks for pointing it out. I realized I was testing without setting an appropriate test depth so these tests were always skipped. After setting appropriate depth I do see them running, some of them do fail.
I agree that it should be fixed before merging 👍🏽
I'll also investigate why the CI does not catch this, we need to set options(TESTING_DEPTH = 5) before running the tests, I'm not sure if the CI does this.

[vedhav]

I should have been more clear. I meant reviewing the content from the design folder to see if we can remove something that's not needed anymore. It's okay if we need everything to stay.

[danielinteractive]

Thanks @vedhav , that helps - yes, https://github.com/insightsengineering/teal.modules.hermes/blob/main/design/cdse_connector_datasets_tests.R can be removed, the other files can stay.

[danielinteractive]

@vedhav just re-request review when ready

[danielinteractive]

@vedhav is this ready for re-review already?

[vedhav]

Sorry, this slipped under the radar with the CRAN release chores. Let me have a look now.

[vedhav]

@danielinteractive Fixed the broken tests now (almost). I do not have a windows machine so I was unable to update snapshots for that. However, I've updated the snapshots for mac although this can be removed after you've reviewed them, just there for reference. I think the CI just needs one set of snapshots as we have in the tlg-catalog
A couple of snapshots are still changing a little due to randomness even after specifying app-level seed but other errors are resolved now.

[danielinteractive]

thanks @vedhav !