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

update testthat version #359

Merged
merged 6 commits into from
Jan 8, 2025
Merged

update testthat version #359

merged 6 commits into from
Jan 8, 2025

Conversation

donyunardi
Copy link
Contributor

@donyunardi donyunardi commented Jan 3, 2025

We need to bump up testthat version because we're using testthat::it function in unit tests.

This function was introduced in 3.1.8:
https://github.com/r-lib/testthat/blob/v3.1.8/R/describe.R#L89

This update will fix the Dependency Test - min isolated:
https://github.com/insightsengineering/teal.data/actions/runs/12532231366/job/34950746107#step:9:774

@donyunardi donyunardi added the core label Jan 3, 2025
@donyunardi
Copy link
Contributor Author

Got another failure about compare_proxy which seems to be related to waldo package:
https://github.com/insightsengineering/teal.data/actions/runs/12605104075/job/35133232553#step:9:235

Updating testthat to 3.2.0

@donyunardi
Copy link
Contributor Author

Still fails:
https://github.com/insightsengineering/teal.data/actions/runs/12605422042/job/35134030219#step:9:853

This may have something to do with the default_cdisc_join_keys object.

@donyunardi
Copy link
Contributor Author

Issue still persist. I think it may have something to do with other dependencies based on the min_isolated strategy.

@averissimo @m7pr
Any chance you can shed a light on this based on your experience working with verdepcheck?
For sure we need to update the testthat version to 3.1.8 but not sure how to address this compare_proxy issue.

@averissimo averissimo marked this pull request as ready for review January 7, 2025 14:24
Copy link
Contributor

github-actions bot commented Jan 7, 2025

Unit Tests Summary

  1 files   15 suites   1s ⏱️
163 tests 151 ✅ 12 💤 0 ❌
213 runs  201 ✅ 12 💤 0 ❌

Results for commit 01ab8dd.

♻️ This comment has been updated with latest results.

@averissimo
Copy link
Contributor

I'm not able to reproduce the error on my machine (locally with building from source) as verdepcheck never resolves down to 3.1.8 (using 3.2.2 with same strategy)

I'm testing with the CI virtual image and will update this issue when possible, it takes some time on every run 😅

@averissimo averissimo self-assigned this Jan 7, 2025
@llrs-roche
Copy link
Contributor

I was also checking this issue: compare_proxy comes from waldo which introduced it very early, but since its origin it has been adding different methods. S4 methods appeared on early, but there have been improvements on its methods as recently as November.

Latest testthat (December 2024) increased the dependency version of waldo to a waldo version with improvements on S4 methods of compare (also November 2024), so we might require a high waldo version (despite what testthat says).

@averissimo
Copy link
Contributor

averissimo commented Jan 7, 2025

There's a problem with waldo and glue version combo. waldo pre-0.5.3 needs glue 1.7.0 and fails with others that are more recent.

r-lib/waldo#212

tidyverse/glue#331

We can bump to the current version of testthat or use extra-deps argument for the verdepcheck action.

I don't think there's a problem with bumping to latest as this is not a validated package AFAIK.

@averissimo
Copy link
Contributor

Both solutions work, I'm leaning towards bumping {testthat} to v3.2.2 as that solves the problem cleanly

image

Note: release and max fails are expected (unreleased version and CRAN 0-day old package respectively)

@donyunardi
Copy link
Contributor Author

Thanks for looking into this @averissimo @llrs-roche
I would prefer to update this to 3.2.2.

@averissimo
Do you think we still need my update to the unit test? Or should we revert this?

testthat::expect_true(exists("default_cdisc_join_keys", where = asNamespace("teal.data")))

@averissimo
Copy link
Contributor

That is a great addition here @donyunardi (today I learned about that parameter)

@donyunardi donyunardi merged commit 7e65b44 into main Jan 8, 2025
42 of 61 checks passed
@donyunardi donyunardi deleted the fix_scheduled_workflow@main branch January 8, 2025 16:06
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants