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

Update unit test template for usethis::use_test #2082

Closed
wants to merge 2 commits into from

Conversation

jakubsob
Copy link

There is no issue created for this PR, nor have I created one, but I wanted to come forward with a small potential improvement.

The current unit test template is very minimal and it's good because it's just a placeholder.

But such a minimal template doesn't set R users to create a communicative unit test.

There's an established way of structuring unit tests with #Arrange, #Act, #Assert comments so that they are more readable and communicate intent of the test better.

This was proposed by Bill Wake and is endorsed by the likes of Martin Fowler.

I believe by introducing this new template, R community could benefit from having guidelines how to structure their unit tests better.

@jennybc
Copy link
Member

jennybc commented Nov 19, 2024

I'm certainly supportive of this sentiment, but am skeptical about how much changing the template can do to change behaviour. This search related to the current template is interesting/funny-in-a-dark-way:

https://github.com/search?q=%22multiplication+works%22+path%3Atests%2Ftestthat%2F*&type=code

I interpret these results as evidence of how little folks really look at the template -- to the extent that they often forget to even edit the test description!

There's also the issue that if you really want to model desirable practices in the template, then I don't think you should test behaviour that is someone else's responsibility (base R, in this case). You could argue the same about "multiplication works" but I suppose the silliness and brevity of the test is meant to convey that it's disposable.

@jakubsob
Copy link
Author

jakubsob commented Nov 20, 2024

@jennybc Thank you, those are very valid points.

I agree that testing base R could also give some false ideas about testing, and my motivation was how to make an example of a test that will work for everyone? One possibility was to test something from base R. But now I see how that might be confusing.

My intention with changing this template is not to change someones behavior, but possibly nudge them in the right direction.

Question about adoption of this template has crossed my mind, and based on my experience, it was the most useful when I was just getting started with R. I assume that more advanced R users will always disregard this template, no matter what's in there, or create test files on their own, not via use_test().

But for who are beginners, this might be the first time they encounter unit tests (maybe they haven't finished reading R Packages book before starting to build). This was the case for me, and back then I definitely haven't thought that this example is so silly that it's disposable. I thought that it's how you should do it. That to say "something works" is enough.

As for the fact that some folks don't even change it, what if the template test was failing instead?

Also based on your comments, maybe just an empty template with comments would be better?

# Use Arrange, Act, Assert pattern to structure your test.
# https://xp123.com/3a-arrange-act-assert/
test_that("[something] does [something]", {
  # Arrange
  
  # Act
  
  # Assert
  fail("Update this test with your logic.")
})

I'm also skeptical how much impact this change might have. But there's a slim chance that it will reach some folks and nudge them into the right direction. And this change won't harm the rest if they disregard this template anyway.

So I see this change as having an unknown, but probably positive impact and having very low risk.

@jennybc
Copy link
Member

jennybc commented Nov 20, 2024

Thanks for these considerations and philosphically I'm certainly in agreement with you! But I need to release usethis on CRAN deadline and I'm not convinced that this is as impactful a change as other open PRs and issues.

@jennybc jennybc closed this Nov 20, 2024
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