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

Use of is_testing() is a problem for folks who program against pins functions #640

Closed
juliasilge opened this issue Aug 20, 2022 · 4 comments · Fixed by #724
Closed

Use of is_testing() is a problem for folks who program against pins functions #640

juliasilge opened this issue Aug 20, 2022 · 4 comments · Fixed by #724
Labels
bug an unexpected problem or unintended behavior

Comments

@juliasilge
Copy link
Member

Interesting - as I was writing more of my unit tests, I noticed that the pin_write function behaves differently when it is run within or outside unit tests (of the testthat variety). That's because the time stamp part of the version is fixed in a unit test:

version_name <- function(metadata) {

I didn't expect that, and it took me a good while to figure out why my unit tests were failing. (I was writing the same file twice into a versioned board. My project includes tracking the metadata in a SQL database - and that complained about duplicated version keys.)

There is nothing intrinsically wrong with the current approach, but it reminds me a bit of the Volkswagen scandal 😄 and the behavior definitely was surprising. (I solved it by temporarily unsetting the TESTTHAT environmental variable in my test.)

Originally posted by @tomsing1 in #637 (comment)

@juliasilge
Copy link
Member Author

The current use of is_testing(), for example, in version_name():

pins-r/R/pin_versions.R

Lines 147 to 148 in 9e8c081

if (is_testing()) {
paste0("20120304T050607Z-", substr(metadata$pin_hash, 1, 5))

makes it hard for folks to develop against pins functions and write their own tests.

@juliasilge juliasilge added the bug an unexpected problem or unintended behavior label Aug 20, 2022
@juliasilge
Copy link
Member Author

The new tests for write_board_manifest() (specifically the ones in test-board_folder.R that look at the structure of the manifest file) involve versions and could be improved in terms of how easy they are to grasp. Currently, we use is_testing() to handle versions in tests but we don't want to continue doing that.

One option for some cases might be to use snapshot testing with a function to redact versions.

@juliasilge
Copy link
Member Author

juliasilge commented Mar 1, 2023

We could switch to use some mocking, like this:

library(pins)
b <- board_temp()

## pin_write() calls version_name() internally
b %>% pin_write(1:10, name = "nice-numbers", type = "json")
#> Creating new version '20230301T003852Z-c3943'
#> Writing to pin 'nice-numbers'

library(mockery)

# call to version_name() is 2 layers down from pin_store()
stub(
  pin_store, 
  "version_name", 
  "20120304T050607Z-xxxxx",
  depth = 2
  )

## now with stub:
b %>% pin_write(1:10, name = "nice-numbers", type = "json")
#> Replacing version '20230301T003852Z-c3943' with '20120304T050607Z-xxxxx'
#> Writing to pin 'nice-numbers'

Created on 2023-02-28 with reprex v2.0.2

Notice that the new version name has been stubbed out.

@github-actions
Copy link

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant