search

ngreifer / cobalt

Covariate Balance Tables and Plots - An R package for assessing covariate balance
https://ngreifer.github.io/cobalt/
70 stars 11 forks source link

Convert tests to `testthat` #65

Closed etiennebacher closed 4 months ago

etiennebacher commented 1 year ago

Hello @ngreifer, the goal of this PR is to convert the tests that are in do_not_include/tests.R to testthat, so that the testing process is more robust and more transparent about the expected outcome (related to #5). Indeed, unless I'm mistaken (which may completely be the case), you don't really test that outputs stay the same in do_not_include/tests.R.

I just converted a few tests so that you can let me know what you think. If you agree with this conversion, then I can go on and move the rest of the tests.

The "problem" is that bal.tab() shows quite a lot of information and it would be tedious to hardcode all expected output to check that the results don't change, so I used snapshot testing. If you're not familiar with this, it consists in saving the output of bal.tab() to a text file (contained in tests/testthat/_snaps) and this output will serve as a reference for the future checks. This means that if anything changes in the output of bal.tab() (including the formatting such as spaces, indents, etc.), you will be notified and you can then review the changes (and potentially accept them). Here are some slides about it if you're interested: https://indrajeetpatil.github.io/intro-to-snapshot-testing/#/title-slide

Anyway, let me know first if you want to continue this transition towards testthat

Thanks for this super useful package ;)

ngreifer commented 1 year ago

@etiennebacher Thank you so much for doing this! cobalt severely needs a testing framework. I've only just started to use testthat for some of my other packages. It will take some time for me to look into this and learn how to incorporate your additions.

etiennebacher commented 1 year ago

Hi @ngreifer, small bump on this, just to know if you need some help with the transition to testthat (I don't want to put pressure, I was just going through the list of my open PRs)

etiennebacher commented 4 months ago

I see that you started using testthat so this PR loses a bit of its interest (although I still think you could expect_snapshot() to easily test the numeric outputs).