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

Revamp tests #255

Merged
merged 7 commits into from
May 18, 2022
Merged

Revamp tests #255

merged 7 commits into from
May 18, 2022

Conversation

agila5
Copy link
Contributor

@agila5 agila5 commented May 3, 2022

Revamp the existing tests using the so-called test fixtures: https://testthat.r-lib.org/articles/test-fixtures.html

The main benefit of the new approach is that each test is explicitly run in a clean and independent session (so each time the PBF file is copied to the tempdir() and removed at the end of the test). That helped me a lot to detect problems in the old version of the tests. The main con is that this might be extremely difficult to modify in a few months, let's see 😅

@agila5
Copy link
Contributor Author

agila5 commented May 3, 2022

@Robinlovelace I know that it might be difficult to understand all the stuff, but if you have time to review, please do

@Robinlovelace
Copy link
Member

@Robinlovelace I know that it might be difficult to understand all the stuff, but if you have time to review, please do

Looks like some serious changes to the testing code, and if it allows tests to run in a more isolated environment that's good. However I don't feel I have the experience to review this, could we ask someone from rOpenSci to take a look?

@agila5
Copy link
Contributor Author

agila5 commented May 5, 2022

could we ask someone from rOpenSci to take a look?

If you know anybody that would like to review the PR please ask, otherwise I think we can simply merge and then worry when one of the tests breaks.

The main changes are summarised in the setup_pbf() function which is used to run each test in a separated environment. Moreover, the calls to withr::local_envvar are used to temporarily modify the oe_download_directory().

@agila5 agila5 mentioned this pull request May 6, 2022
@agila5
Copy link
Contributor Author

agila5 commented May 18, 2022

Any news here?

@Robinlovelace
Copy link
Member

None from me but happy for this to be merged.

@agila5 agila5 merged commit 05221d4 into master May 18, 2022
@agila5 agila5 deleted the improve_tests branch May 18, 2022 17:44
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.

2 participants