-
Notifications
You must be signed in to change notification settings - Fork 107
Add some tests for wflow_start_rstudio #205
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
Conversation
It seems like most of the test won't work unless setting |
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.
@trannhatanh89 This is great! Once you switch to using on.exit()
, these tests should pass.
There's only one more test I'd like to see included: throwing an error if the Git user.name and user.email aren't configured.
https://github.com/jdblischak/workflowr/blob/master/tests/testthat/test-wflow_start.R#L204
You can copy-paste it, and then change the expected error message:
https://github.com/jdblischak/workflowr/blob/master/tests/testthat/test-wflow_start.R#L216
https://github.com/jdblischak/workflowr/blob/master/R/wflow_start_rstudio.R#L29
Please let me know if you have any questions. Thanks!
test_that("wflow_start_studio accepts custom name", { | ||
|
||
project_name <- "A new project" | ||
site_dir <- tempfile() |
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.
The trick to get around the changing working directory is to use on.exit()
to reset after the test function exits. The 3 lines below configure the test to revert the working directory and then delete the temporary test directory created by wflow_start()
. If you include these lines at the beginning of each test (right here, after defining site_dir
), this should fix your issue.
cwd <- getwd()
on.exit(setwd(cwd))
on.exit(unlink(site_dir, recursive = TRUE, force = TRUE), add = TRUE)
site_yaml_contents <- readLines(file.path(site_dir, "analysis", "_site.yml")) | ||
expect_identical(site_yaml_contents[1], sprintf('name: "%s"', project_name)) | ||
|
||
unlink(site_dir, recursive = TRUE, force = TRUE) |
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.
When you switch to using the on.exit()
strategy, you can delete the unlink()
calls at the end of the function.
site_dir <- tempfile() cwd <- getwd() on.exit(setwd(cwd)) on.exit(unlink(site_dir, recursive = TRUE, force = TRUE), add = TRUE)
test_that("wflow_start_rstudio throws an error if user.name and user.email are not set",())
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
+ Coverage 85.69% 86.26% +0.56%
==========================================
Files 32 32
Lines 3517 3517
==========================================
+ Hits 3014 3034 +20
+ Misses 503 483 -20
Continue to review full report at Codecov.
|
@trannhatanh89 Thanks for contributing to workflowr! |
@jdblischak, thank you for approving the pull request. I wanted to wait for checks to pass last night before asking you to re-review, but i guess i fell asleep. =_= |
@trannhatanh89 Heads up. You may need to fix your
The issue is that Sorry for the inconvenience. I fixed the issue in 3398e78 |
Summary
Hello @jdblischak, thank you for the opportunity to contribute to workflowr. As we have discussed, I added a few unit tests for
wflow_start_rstudio
. So far they are based onwflow_start
tests with updated syntax forwflow_start_rstudio
. I went though those test and picked out ones that seemed appropriate for the rstudio version. I create this pull request so that the code could be checked. If everything went through, please review and allow this branch to be merged.Sincerely,
Anh Tran.
Checklist
Details