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

remove uses of testthat::with_mock() (fixes #223) #237

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

jameslamb
Copy link
Collaborator

Fixes #223

This removes the one use of testthat::with_mock() in the package, for the reasons mentioned in #223.

It does this by moving the {httr} calls we want to mock into separate internal functions within {uptasticsearch}. Those thin wrappers are introduced here mainly because of the advice from https://testthat.r-lib.org/reference/local_mocked_bindings.html#namespaced-calls

It's trickier to mock functions in other packages that you call with ::. For example, take this minor variation:

some_function <- function() {
  anotherpackage::another_function()
}

To mock this function, you'd need to modify another_function() inside the anotherpackage package. You can do this by supplying the .package argument to local_mocked_bindings() but we don't recommend it because it will affect all calls to anotherpackage::another_function(), not just the calls originating in your package. Instead, it's safer to either import the function into your package, or make a wrapper that you can mock.

Notes for Reviewers

These new internal functions are used in multiple files, so I wanted to centralize them in one other file. Introducing: helperfuns.R 😊

@jameslamb jameslamb added the maintenance miscellaneous maintenance label Jan 29, 2025
@jameslamb jameslamb changed the title remove uses of testthat::with_mock() (fixes #223) WIP: remove uses of testthat::with_mock() (fixes #223) Jan 29, 2025
@jameslamb jameslamb changed the title WIP: remove uses of testthat::with_mock() (fixes #223) remove uses of testthat::with_mock() (fixes #223) Jan 29, 2025
@jameslamb jameslamb marked this pull request as ready for review January 29, 2025 15:00
Copy link
Collaborator

@austin3dickey austin3dickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me!

@jameslamb
Copy link
Collaborator Author

jameslamb commented Jan 29, 2025

Thanks! This is the only change we *need to be protected on CRAN ahead of R 4.5.0.

I expect that to come out in April (as every previous minor release has), so we have a bit more time. I'm going to continue with a few other things to try to reduce the we-get-a-fix-or-be-archived-email-from-CRAN risk.

Like these:

@jameslamb jameslamb merged commit ba76841 into uptake:main Jan 29, 2025
18 checks passed
@jameslamb jameslamb deleted the remove-with-mock branch January 29, 2025 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance miscellaneous maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

with_mock() is going away
2 participants