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

[tests] improve testfile_path() to download files #785

Merged
merged 2 commits into from Oct 1, 2023

Conversation

JanMarvin
Copy link
Owner

Previously all files were downloaded prior to testing. Now the download is triggered by testfile_path(). This allows restoring parallel tests.

@JanMarvin
Copy link
Owner Author

Could you have a look @olivroy ? I remembered an earlier complaint from your side.

  • Tests with external files should run only if the file was downloaded and downloads should only be attempted if online
  • On the CI the tests with external files should only run on release and for coverage
  • I didn't want to make things overly complicated, no md5 file hashes etc. If some test file is broken (e.g. partially downloaded, we can skip it)

tests/testthat/helper.R Outdated Show resolved Hide resolved
@olivroy
Copy link
Collaborator

olivroy commented Sep 2, 2023

Well, it's just that at first, the downloads were problematic..

As you said, it's probably fine as is..
I know that styler has a skip_if_parallel() that may be handy.

I will do more research on how other packages do this. the zip package creates temp dir for that too.

https://testthat.r-lib.org/articles/parallel.html the testthat vignette

@olivroy
Copy link
Collaborator

olivroy commented Sep 25, 2023

I tripped on this again while working on #799 (solved by downloading the zip file from gh, unzip, then copy the contents manually to tests/testthat/testfiles

@JanMarvin
Copy link
Owner Author

Yes, I plan to merge it, but wanted to look into the comment above. Since testthat hides all the annoying network stuff I need and I don't want to add curl as dependency, I needed to play around with it.

@JanMarvin JanMarvin force-pushed the improve_testfile_path branch 2 times, most recently from 18d2810 to c0e839f Compare October 1, 2023 18:30
@JanMarvin JanMarvin merged commit fcb6670 into main Oct 1, 2023
9 checks passed
@JanMarvin JanMarvin deleted the improve_testfile_path branch October 1, 2023 19:04
@JanMarvin
Copy link
Owner Author

At least the code looks now a little less confusing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants