Ensures required packages are in Imports section #774

averissimo · 2024-08-14T15:30:51Z

Pull Request

Fixes #518

Note on removals:

Remove broom as the broom:glances method can be replaced with generics::glances
- According to my tests (methods are being deprecated as well)

Changes description

Removed conditional use of packages
Removes {broom} dependency
Uses rlang conditionally
Uses Suggested packages conditional on tests

R/tm_missing_data.R

github-actions · 2024-08-14T15:46:40Z

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  -------------------------------------------------
R/tm_a_pca.R                    884     884  0.00%    138-1155
R/tm_a_regression.R             772     772  0.00%    162-1036
R/tm_data_table.R               215     215  0.00%    110-380
R/tm_file_viewer.R              173     173  0.00%    47-255
R/tm_front_page.R               133     122  8.27%    73-231
R/tm_g_association.R            340     340  0.00%    143-556
R/tm_g_bivariate.R              685     421  38.54%   315-794, 835, 946, 963, 981, 992-1014
R/tm_g_distribution.R          1103    1103  0.00%    155-1401
R/tm_g_response.R               364     364  0.00%    161-601
R/tm_g_scatterplot.R            729     729  0.00%    244-1075
R/tm_g_scatterplotmatrix.R      294     275  6.46%    182-511, 572, 586
R/tm_missing_data.R            1115    1115  0.00%    122-1410
R/tm_outliers.R                1031    1031  0.00%    162-1342
R/tm_t_crosstable.R             259     259  0.00%    148-454
R/tm_variable_browser.R         824     819  0.61%    89-1070, 1108-1291
R/utils.R                       156     125  19.87%   81-266, 296-327, 348, 351, 356, 371-392, 403, 408
R/zzz.R                           2       2  0.00%    2-3
TOTAL                          9079    8749  3.63%

Diff against main

Filename                      Stmts    Miss  Cover
--------------------------  -------  ------  --------
R/tm_g_distribution.R            -7      -7  +100.00%
R/tm_g_scatterplot.R             -7      -7  +100.00%
R/tm_g_scatterplotmatrix.R       -2      -2  +0.04%
R/tm_missing_data.R              +4      +4  +100.00%
R/tm_t_crosstable.R              -2      -2  +100.00%
R/tm_variable_browser.R          -6      -6  +0.00%
TOTAL                           -20     -20  +0.01%

Results for commit: ad8ba68

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2024-08-14T15:59:40Z

Unit Tests Summary

1 files 22 suites 12m 50s ⏱️
145 tests 107 ✅ 38 💤 0 ❌
476 runs 438 ✅ 38 💤 0 ❌

Results for commit 27a75ad.

♻️ This comment has been updated with latest results.

github-actions · 2024-08-14T15:59:44Z

Unit Test Performance Difference

Test Suite	$Status$	Time on `main`	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
shinytest2-tm_g_distribution	💚	$60.65$	$-2.18$	$0$	$0$	$0$	$0$
shinytest2-tm_variable_browser	💚	$59.10$	$-1.05$	$0$	$0$	$0$	$0$

Results for commit bab0579

♻️ This comment has been updated with latest results.

pawelru · 2024-08-15T06:39:14Z

tests/testthat/test-shinytest2-tm_data_table.R

@@ -44,6 +44,7 @@ test_that("e2e - tm_data_table: Verify checkbox displayed over data table", {
 })

 test_that("e2e - tm_data_table: Verify module displays data table", {
+  testthat::skip_if_not_installed("rvest")


I have recently pushed changes into the teal due to noSuggest check and the class definition looks like this:
https://github.com/insightsengineering/teal/blob/7eac4d58a5373aa511f363a2f259d850749501f7/R/TealAppDriver.R#L10-L20

Just thinking loud now, would it help if we would have the following instead?

if (!requireNamespace("shinytest2", quietly = TRUE)) { if (requireNamespace("testthat", quietly = TRUE) && getNamespace("testthat")$is_testing()) { getNamespace("testthat")$skip("shinytest2 is not installed") } else { stop("Please install 'shinytest2' package to use this class.") } }

My motivation is to avoid a lot of skip statements in the upstream packages like here.

That's awesome, I'll create the PR in {teal} to do just this. I think the $is_testing() is not required as skip() throws an error and it's on testthat to capture it and handle

m7pr · 2024-11-19T07:59:34Z

@averissimo is this still relevant?
Should we add to this sprint and get someone to review?

averissimo · 2024-12-03T13:51:46Z

@m7pr it is, although it needs a new pass to make sure that nothing new has created a conflict here

I'm moving it to draft

* main: (42 commits) [skip actions] Bump version to 0.3.0.9061 Stop R process if AppDriver fails in `test-examples` (#820) [skip actions] Bump version to 0.3.0.9060 87 remove datasets - decrease package size (#818) [skip actions] Bump version to 0.3.0.9059 Fix documentation note about links on r-devel (#817) [skip actions] Bump version to 0.3.0.9058 get back staged deps config (#816) [skip actions] Bump version to 0.3.0.9057 add setup-r-dependencies (#815) [skip actions] Bump version to 0.3.0.9056 Teal version bump (#814) [skip actions] Bump version to 0.3.0.9055 🗃️ `decorators` feature branch (#795) [skip actions] Bump version to 0.3.0.9054 `teal.data::datanames()` is deprecated in favor of dot-prefix and `names()` (#794) [skip actions] Bump version to 0.3.0.9053 Adds `roxy.shinylive` to pre-commit configuration (#793) [skip actions] Bump version to 0.3.0.9052 add rmarkdown to VignetteBuilder (#792) ...

averissimo · 2024-12-18T15:50:54Z

DESCRIPTION

@@ -31,16 +31,30 @@ Depends:
    teal.transform (>= 0.5.0.9015)


Teal requirement needs to be bumped with insightsengineering/teal#1432

Possibly to teal (>= 0.15.2.9091),

Keeping conversation open until the PR is closed

I approved that PR, so working on this PR can continue.

pawelru · 2024-12-19T09:13:29Z

DESCRIPTION

I see changes in dependencies and I immediately think about verdepcheck. Can you please confirm that it is passing on this? If not - let's work on the deps versions.

This might need to wait for https://github.com/insightsengineering/teal.modules.general/pull/774/files#r1890465398

llrs-roche

I left a couple of comments, as I haven't checked the code locally yet

llrs-roche · 2024-12-20T08:37:19Z

DESCRIPTION

@@ -31,16 +31,30 @@ Depends:
    teal.transform (>= 0.5.0.9015)


I approved that PR, so working on this PR can continue.

DESCRIPTION

…1432) # Pull Request Part of insightsengineering/teal.modules.general#774 See insightsengineering/teal.modules.general#774 (comment)

This reverts commit 8d1c172.

llrs-roche · 2024-12-20T17:15:09Z

teal.modules.general.Rproj

@@ -1,4 +1,5 @@
 Version: 1.0
+ProjectId: 003390c8-e31f-4b87-957d-7943778020cc


New Rstudio version :) I wonder what this is used for, as it was not disclosed on the release notes

averissimo added 2 commits August 14, 2024 17:29

chore: move required packages to imports

e2275ab

fix: ensure optional requirements in tests

4590f44

averissimo added the core label Aug 14, 2024

averissimo mentioned this pull request Aug 14, 2024

Incorrect soft dependencies #518

Open

averissimo commented Aug 14, 2024

View reviewed changes

R/tm_missing_data.R Show resolved Hide resolved

averissimo marked this pull request as ready for review August 14, 2024 15:39

pawelru reviewed Aug 15, 2024

View reviewed changes

averissimo marked this pull request as draft December 3, 2024 13:51

averissimo added 4 commits December 18, 2024 14:30

chore: make sure all github are included

a87ccd1

chore: remove require namespaces from examples

3015749

chore: remove whitespace

b083bc4

averissimo mentioned this pull request Dec 18, 2024

TealAppDriver skips tests when shinytest2 or rvest are not installed insightsengineering/teal#1432

Merged

chore: remove unecessary skips

fcedb3c

averissimo commented Dec 18, 2024

View reviewed changes

averissimo marked this pull request as ready for review December 18, 2024 18:41

[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

98ae21c

pawelru reviewed Dec 19, 2024

View reviewed changes

empty: trigger ci

27a75ad

llrs-roche reviewed Dec 20, 2024

View reviewed changes

averissimo added 2 commits December 20, 2024 15:51

test: action with custom max additional repos

8d1c172

Revert "test: action with custom max additional repos"

ad8ba68

This reverts commit 8d1c172.

llrs-roche reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensures required packages are in Imports section #774

Ensures required packages are in Imports section #774

averissimo commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

pawelru Aug 15, 2024 •

edited

Loading

averissimo Dec 18, 2024

m7pr commented Nov 19, 2024

averissimo commented Dec 3, 2024

averissimo Dec 18, 2024 •

edited

Loading

llrs-roche Dec 20, 2024

pawelru Dec 19, 2024

llrs-roche left a comment

llrs-roche Dec 20, 2024

llrs-roche Dec 20, 2024

		@@ -1,4 +1,5 @@
		Version: 1.0
		ProjectId: 003390c8-e31f-4b87-957d-7943778020cc

Ensures required packages are in Imports section #774

Are you sure you want to change the base?

Ensures required packages are in Imports section #774

Conversation

averissimo commented Aug 14, 2024 • edited Loading

Pull Request

Changes description

github-actions bot commented Aug 14, 2024 • edited Loading

Code Coverage Summary

Diff against main

github-actions bot commented Aug 14, 2024 • edited Loading

Unit Tests Summary

github-actions bot commented Aug 14, 2024 • edited Loading

Unit Test Performance Difference

pawelru Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

averissimo Dec 18, 2024

Choose a reason for hiding this comment

m7pr commented Nov 19, 2024

averissimo commented Dec 3, 2024

averissimo Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

llrs-roche Dec 20, 2024

Choose a reason for hiding this comment

pawelru Dec 19, 2024

Choose a reason for hiding this comment

llrs-roche left a comment

Choose a reason for hiding this comment

llrs-roche Dec 20, 2024

Choose a reason for hiding this comment

llrs-roche Dec 20, 2024

Choose a reason for hiding this comment

averissimo commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

pawelru Aug 15, 2024 •

edited

Loading

averissimo Dec 18, 2024 •

edited

Loading