-
Notifications
You must be signed in to change notification settings - Fork 56
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
Use testthat 3 + remove deprecated features #577
Conversation
… ignoring whether the object is a list * Revert order of `expect_warning()` / `expect_message()` to be compliant with testthat 3e https://www.tidyverse.org/blog/2021/10/testthat-3-1/#breaking-changes *
More expect_equal_unlist() Remove local_edition(3)
* use `ignore_attr = TRUE` in some cases * Use expect_null() * Use multiple expect_equal() to avoid unused argument warning
@rich-iannone sorry for the many follow-up commits. Now ready for review! |
This is great @olivroy ! Thanks so much for taking the time for all these fixes :) @yjunechoe I also invited you to take a look at the changes if you're up for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and thanks so much!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful! The deprecation warnings were a headache to track down, so I'm very happy to see them resolved
All tests continue to pass when I test on my end, so LGTM as well!
@olivroy it seems like the empty commit trick didn't fix the hanging check of the branch status. Maybe we wait another day for this to (hopefully) update? |
This comment was marked as resolved.
This comment was marked as resolved.
I don't have a way through the GitHub UI. And I tried through GH Mobile but it's the same hanging state. I might work itself out in time. Worst case scenario is you make another PR with the same changes. |
That worked! |
Alright! After CI runs, I'll merge! |
I just did gert::git_reset_soft("HEAD~3")
gert::git_commit_all("commit to unstale branch")
gert::git_push(force = TRUE) and it worked! guess GitHub didn't like removing a commit, but doesn't mind combining into many. The commits are less informative now, but it is fine since you both reviewed it already!) |
Thanks again @olivroy for modernizing the codebase. I will add a CodeCov token soon! |
Summary
Turns out this was much more work than I imagined
expect_equivalent()
(various solutions were needed here, such as creatingexpect_equal_unlist()
that is equivalent toexpect_equal(unlist(object), expected)
, useignore_attr
expect_s3_class()
andexpect_type()
instead ofexpect_is()
tidyselect::all_of()
instead of supersededone_of()
expect_no_*()
instead ofexpect_*(regexp = NA)
testthat::test_path()
to facilitate running tests interactively.Also a reminder that to unlock the coverage action, you will have to add a Codecov token
Other minor cleanup
Checklist
testthat
unit tests totests/testthat
for any new functionality.