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

test: some suggestions for making failing test file more readable #10

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maelle
Copy link
Member

@maelle maelle commented Sep 30, 2024

👋 @quishqa!

I don't know yet why the Windows build is failing (I am on Ubuntu and can't run the tests locally yet see #9) however I think this test file can be made more informative when it fails by using more specific expectations. Some of them might be new to you, I think going over https://testthat.r-lib.org/reference/index.html once in a while is useful as new expectations appear over time (it's useful for me at least!).

Anyway as a summary:

  • no need to explicitly load the internal data.
  • I had trouble understanding the failure param_means["ws"] > 1 (actual) not equal to param_means["ws"] < 2 (expected)., using expect_lt() and expect_gt() instead will give a clearer failure.
  • I'd recommend moving the expectations around the class of pinheiros and its columns before the calculations.
  • These expectations around classes should use https://testthat.r-lib.org/reference/inheritance-expectations.html although for the columns I have added a comment, I think it'd make sense to create the vector of all class names then use expect_setequal() https://testthat.r-lib.org/reference/expect_setequal.html (it does not check the order of columns).

@maelle
Copy link
Member Author

maelle commented Sep 30, 2024

Now it's failing everywhere, clearly I did something wrong. Happy to run the tests locally once I have more instructions 🙈

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.

1 participant