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

Recommendations for snapshotting dynamic output? #1484

Closed
MichaelChirico opened this issue Nov 8, 2021 · 4 comments
Closed

Recommendations for snapshotting dynamic output? #1484

MichaelChirico opened this issue Nov 8, 2021 · 4 comments

Comments

@MichaelChirico
Copy link
Contributor

I'm trying (Issue: MichaelChirico/potools#256 & WIP PR: MichaelChirico/potools#257) to migrate some tests to use expect_snapshot() and it's made the test script way cleaner so I'm quite keen to do so, however I've come across the following issues:

  • The test is run in withr::local_tempdir(), and the function prints a full path
  • The function reads & displays file.mtime() for some files. This is more static if the file is read from the package repo, but as part of setup/teardown, I copy the file to local_tempdir(), meaning the file update time is new every run as well

Is such a situation out of scope for expect_snapshot()? Would you recommend I refactor the code to be able to isolate these dynamic parts, or is there any way to turn this into a feature request?

PS I also noticed it seems expect_snapshot() doesn't catch all of the output from system(), meaning I have a messier terminal:

══ Testing test-translate-package.R ════════════════════════════════════════════════════════
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 12 ]5 translated messages.
[ FAIL 1 | WARN 0 | SKIP 0 | PASS 15 ]5 translated messages.
5 translated messages.
5 translated messages.
[ FAIL 2 | WARN 0 | SKIP 0 | PASS 20 ]5 translated messages.
...... done.
5 translated messages.
[ FAIL 3 | WARN 0 | SKIP 0 | PASS 20 ]3 translated messages.
..... done.
3 translated messages.
[ FAIL 4 | WARN 0 | SKIP 0 | PASS 31 ]1 translated message.
2 translated messages.
.. done.
..... done.
1 translated message.
2 translated messages.
[ FAIL 5 | WARN 0 | SKIP 0 | PASS 31 ]Generating en@quot translations
[ FAIL 5 | WARN 0 | SKIP 0 | PASS 47 ]Generating en@quot translations
[ FAIL 5 | WARN 0 | SKIP 0 | PASS 54 ]Generating en@quot translations
[ FAIL 5 | WARN 0 | SKIP 0 | PASS 59 ]

Is there anything to do there besides refactor to try & use system2 internally?

@hadley
Copy link
Member

hadley commented Nov 8, 2021

Two main choices for the dynamic stuff:

  • Add a mock to make it static during the snapshot
  • Use the transform argument and provide some regexps to standardise the varying outputs

I forget the details of the system() output, but I'm pretty sure there's nothing testthat can do because it's emitted through some mechanism that I can't capture at the R-level. I don't recall if system2() helps here, or you need to use sys or processx.

@MichaelChirico
Copy link
Contributor Author

OK, that makes sense. I think system() is the perennial problem also affecting knitr:

https://stackoverflow.com/q/36928322/3576984

yihui/knitr#1203

system2() indeed helps (maybe not in all cases?) -- with stderr = TRUE, terminal's output to stderr is redirected as the return value of system2():

system2("ls", "ijdfiajsdifjasdifja", stderr=TRUE)
# [1] "ls: cannot access 'ijdfiajsdifjasdifja': No such file or directory"
# attr(,"status")
# [1] 2
# Warning messages:
# 1: In system2("ls", "ijdfiajsdifjasdifja", stderr = TRUE) :
#   setting stdout = TRUE
# 2: In system2("ls", "ijdfiajsdifjasdifja", stderr = TRUE) :
#   running command ''ls' ijdfiajsdifjasdifja 2>&1' had status 2

In any case, both referencing the mock() workaround (which is better unit test practice anyway) and the issue with system() warrant mention in the vignette; I'll try and add something there.

@MichaelChirico
Copy link
Contributor Author

(feel free to assign me, I don't have permission to do so)

@hadley
Copy link
Member

hadley commented Jan 5, 2022

Let's add this to #1265

@hadley hadley closed this as completed Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants