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

Use withr to not change global state #829

Merged
merged 8 commits into from
Nov 13, 2023
Merged

Use withr to not change global state #829

merged 8 commits into from
Nov 13, 2023

Conversation

strengejacke
Copy link
Member

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #829 (26503f5) into main (919ac84) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

❗ Current head 26503f5 differs from pull request most recent head 4914275. Consider uploading reports for the commit 4914275 to get more accurate results

@@           Coverage Diff           @@
##             main     #829   +/-   ##
=======================================
  Coverage   55.04%   55.04%           
=======================================
  Files         124      124           
  Lines       15388    15388           
=======================================
  Hits         8470     8470           
  Misses       6918     6918           

@strengejacke
Copy link
Member Author

@IndrajeetPatil Can I add multiple test_that() code chunks into one withr::*() call?
Like:

withr::with_environment(
  new.env(),
  m <- lm(...)
  test_that("1", {
    ...
  })
  test_that("2", {
    ...
  })
  test_that("3", {
    ...
  })
)

@IndrajeetPatil
Copy link
Member

Yes, but IINM you'd need to do something like this:

withr::with_environment(
  new.env(),
  code = {
    m <- lm(...)
    test_that("1", {
      ...
    })
    test_that("2", {
      ...
    })
    test_that("3", {
      ...
    })
  }
)

@strengejacke
Copy link
Member Author

Cool! This should resolve our scoping issues, and probably also the random test order thing.

@IndrajeetPatil
Copy link
Member

You could do that, or maybe a better approach is to create a new environment in each test and test the expectations within this scope.

For example, see https://github.com/r-lib/withr/blob/0c5254ebc74590e80cc0056303d74b049b943920/tests/testthat/test-namespace.R#L70-L87

@strengejacke
Copy link
Member Author

You could do that, or maybe a better approach is to create a new environment in each test and test the expectations within this scope.

This is what I would normally do, but if you look at the test structure of insight, e.g. https://github.com/easystats/insight/blob/main/tests/testthat/test-lm.R, I would need to re-fit the model every time again for each test then (in those cases where we use <<-, which we want to avoid).

@IndrajeetPatil
Copy link
Member

Maybe add back the removed "random test order" check?

@strengejacke
Copy link
Member Author

I think that check should work once we addressed the scoping issues using withr, so I'll do that after the withr-work is done.

@strengejacke strengejacke merged commit ebe7d72 into main Nov 13, 2023
14 of 24 checks passed
@strengejacke strengejacke deleted the use_withr branch November 13, 2023 10:57
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.

Check that the tests don't change the global state
2 participants