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

[chore] tidy testing environment #106

Merged
merged 3 commits into from
Aug 13, 2024
Merged

[chore] tidy testing environment #106

merged 3 commits into from
Aug 13, 2024

Conversation

zkamvar
Copy link
Member

@zkamvar zkamvar commented Aug 12, 2024

I noticed that I was getting errors during testing because of a couple of factors unrelated to the actual validity of the code:

  1. I did not have testthis installed
  2. Folders created inside of tempdir() are not cleaned up leading to errors when re-running the tests because the folders are not empty.

I fixed the first with skip_if_not_installed() and the second by using withr::local_tempdir()

@zkamvar zkamvar requested a review from annakrystalli August 12, 2024 19:46
Copy link

github-actions bot commented Aug 12, 2024

@zkamvar zkamvar added the upkeep maintenance, infrastructure, and similar label Aug 12, 2024
Copy link
Member

@annakrystalli annakrystalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @zkamvar !

Fix for 2. I'm very happy to accept. Much cleaner indeed and makes working interactively with tests much smoother.

Regarding testthis, it is listed in Suggests and I would expect anyone contributing code to install the full list of dependencies and be able to run the full suite of tests, build all docs etc before making a PR. So I would prefer to remove those skips

tests/testthat/test-check_tbl_unique_round_id.R Outdated Show resolved Hide resolved
@zkamvar
Copy link
Member Author

zkamvar commented Aug 13, 2024

Regarding testthis, it is listed in Suggests and I would expect anyone contributing code to install the full list of dependencies and be able to run the full suite of tests, build all docs etc before making a PR. So I would prefer to remove those skips

That's fair. I've removed them. Feel free to squash-merge this since it amounts to exactly one change effectively.

@zkamvar zkamvar requested a review from annakrystalli August 13, 2024 13:57
@annakrystalli
Copy link
Member

annakrystalli commented Aug 13, 2024

Feel free to squash-merge this since it amounts to exactly one change effectively.

Honestly I'm not 100% percent how to do that. Is it git reset --soft HEAD~3?

Copy link
Member

@annakrystalli annakrystalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good!

@zkamvar
Copy link
Member Author

zkamvar commented Aug 13, 2024

Feel free to squash-merge this since it amounts to exactly one change effectively.

Honestly I'm not 100% percent how to do that. Is it git reset --soft HEAD~3?

screenshot showing the dropdown menu for "merge pull request" with three options for merging including "merge", "squash and merge", and "rebase and merge"

@zkamvar zkamvar merged commit 172091c into main Aug 13, 2024
8 checks passed
@zkamvar zkamvar deleted the znk/tidy-tests branch August 13, 2024 15:22
@annakrystalli
Copy link
Member

Feel free to squash-merge this since it amounts to exactly one change effectively.

Honestly I'm not 100% percent how to do that. Is it git reset --soft HEAD~3?

screenshot showing the dropdown menu for "merge pull request" with three options for merging including "merge", "squash and merge", and "rebase and merge"

Awesome! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upkeep maintenance, infrastructure, and similar
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants