Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

specify min deps #104

Closed
wants to merge 56 commits into from
Closed

specify min deps #104

wants to merge 56 commits into from

Conversation

pawelru
Copy link
Contributor

@pawelru pawelru commented Jan 31, 2023

This PR specified minimal version requirement to pass the R CMD CHECK (incl. tests). If it is not specified then it means that it would work with any historical version which I have found not true.

This specification makes R CMD CHECK pass as well as make the installation plan executable. E.g. [email protected] works perfectly fine with the codebase but it won't be used as [email protected] requires [email protected] so essentially this is the result of truly installed minimal dependencies.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2023

badge

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  ---------------------------------------------------------------------------------------------------------------------------------------------
R/chunk.R                  150       3  98.00%   363-369
R/chunks.R                 416      41  90.14%   334, 424, 430-433, 439, 459, 466-467, 484-491, 500, 506-515, 561, 587, 593-594, 596, 612, 621, 630, 672, 675, 699-704, 1070, 1092, 1149, 1202
R/get_eval_details.R       118     118  0.00%    17-188
R/include_css_js.R           7       7  0.00%    12-19
R/qenv-concat.R             10       0  100.00%
R/qenv-constructor.R        12       0  100.00%
R/qenv-eval_code.R          48       2  95.83%   89, 98
R/qenv-get_code.R           16       0  100.00%
R/qenv-get_var.R            17       0  100.00%
R/qenv-get_warnings.R       22       0  100.00%
R/qenv-join.R               46       0  100.00%
R/qenv-show.R                1       1  0.00%    16
R/utils.R                   16       0  100.00%
TOTAL                      879     172  80.43%

Diff against main

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

Results for commit: 4b5b0c9

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2023

Unit Tests Summary

    1 files    11 suites   6s ⏱️
110 tests 110 ✔️ 0 💤 0
441 runs  441 ✔️ 0 💤 0

Results for commit 1c16a06.

♻️ This comment has been updated with latest results.

@pawelru
Copy link
Contributor Author

pawelru commented Feb 8, 2023

Hi @walkowif. I looked briefly into the workflow log and please find my comments:

  • I can see that the install plan has need successfully resolved (that's the most problematic part) and it failed on the compilation error of not-so-old package '[email protected]'. That's quite surprising. Can you check it please if it is installable on our CI image outside of this workflow?
  • would it be possible (and does that make sense?) to split the script into the three steps executed one after another. The first one is resolving min deps. For debugging/troubleshooting we can call (uncomment) 'x$draw()' as well as add lock file as an artifact. Step 2 would be download and install (this is where we are failing now). This step would require object from step 1. Step 3 would be r cmd check of the current repo. What do you think about it?
  • on a separate note: can we make this check non obligatory? At least for the very beginning until we make everything green. I am expecting lots of errors.

I will have a deeper look tomorrow.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
chunks 💔 $2.52$ $+1.59$ $0$ $0$ $0$ $0$

Results for commit 706f290

♻️ This comment has been updated with latest results.

@walkowif
Copy link
Contributor

walkowif commented Feb 8, 2023

I've checked and running docker run --rm ghcr.io/insightsengineering/rstudio_4.2.2_bioc_3.16:latest R -e 'install.packages("https://cran.r-project.org/src/contrib/Archive/testthat/testthat_2.3.0.tar.gz")' returns the same compilation error as the one in the workflow.

We could split the script but I'm not sure what value this would add and I'm also not sure how to pass the object from step 1. to 2.
I moved and uncommented the lockfile creation and draw function, and ran the workflow again - the lockfile is available as an artifact there.

The check can of course be made non-obligatory until it's tested properly.

@walkowif walkowif mentioned this pull request Apr 6, 2023
cicdguy added a commit that referenced this pull request Jun 1, 2023
Propagate changes from:
insightsengineering/idr-tasks#522,
insightsengineering/idr-tasks#533 and
insightsengineering/idr-tasks#543.

Verdepcheck update
(insightsengineering/idr-tasks#513) is added
in #104.

---------

Co-authored-by: Davide Garolini <[email protected]>
Co-authored-by: cicdguy <[email protected]>
@averissimo averissimo mentioned this pull request Jul 26, 2023
11 tasks
@averissimo
Copy link
Contributor

Closed in favor of #123 that implements new strategies min_isolated and min_cohort

@averissimo averissimo closed this Jul 27, 2023
averissimo added a commit that referenced this pull request Sep 5, 2023
WIP :: parent issue:
insightsengineering/nestdevs-tasks#7

Supersede:
* #104

### 🔴 Checklist for PR Reviewer

- [x] Tag yourself next to this repo on
insightsengineering/nestdevs-tasks#7
- [x] Package versions are the same or higher than `main`
- [x] Package list is the same
  - Only exception is `rmarkdown` (on `Suggests`)
- [x] All packages in `Imports`, `Depends` & `Suggests` are in new
section `Config/Needs/verdepcheck`
- [x] Added entry to `NEWS.md`
- [x] Last `scheduled.yaml` action was run succesfully _(all 4
strategies)_
- important: it's not the last commit, it's the one that runs 4
`Scheduled 🕰️ / Dependency` actions
- [x] `scheduled.yaml` SHOULD NOT have any push on any branches

### 🔴 What's needed before merging?

This PR depends on some upstream changes that need to be
finalized/merged before being ready to review.

#### Change in code

* `verdepcheck.yml` action (see comments)
  - [x] Remove `on: push` section 
  - [x] Change branch to main

#### PRS

- [x] verdepcheck
  * insightsengineering/verdepcheck#24
  * insightsengineering/verdepcheck#26
- [x] verdepcheck-action
  * insightsengineering/r-verdepcheck-action#16

### Changes description

* Adds minimum version for packages `DESCRIPTION`
* Adds `Config/Need/verdepcheck` section in `DESCRIPTION`
* Updates verdepcheck action

---------

Signed-off-by: André Veríssimo <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
@pawelru pawelru deleted the min_deps branch November 24, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants