Closed vedhav closed 8 months ago
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%
Filename Stmts Miss Cover
---------- ------- ------ --------
TOTAL 0 0 +100.00%
Results for commit: 01b2ca31c5c094d4d402c9ce53d74bd8e1de025d
Minimum allowed coverage is 80%
:recycle: This comment has been updated with latest results
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
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/
.
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)
1 files 15 suites 11s :stopwatch: 56 tests 43 :white_check_mark: 13 :zzz: 0 :x: 538 runs 525 :white_check_mark: 13 :zzz: 0 :x:
Results for commit a46ab685.
:recycle: This comment has been updated with latest results.
@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?
@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.
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.
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.
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.
@vedhav just re-request review when ready
@vedhav is this ready for re-review already?
Sorry, this slipped under the radar with the CRAN release chores. Let me have a look now.
@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.
Closes https://github.com/insightsengineering/teal.slice/issues/492
We don't accept data in module UI anymore & there was a missing dataname in one example.