search

sinanpl / OaxacaBlinder

R implementation of Oaxaca-Blinder gap decomposition
MIT License
1 stars 1 forks source link

tests #7

Open sinanpl opened 6 months ago

sinanpl commented 6 months ago

Opening this issue as a reminder to create tests. Perhaps comparing output with the oaxaca package on CRAN

sinanpl commented 5 months ago

I finally managed to get this started, @davidskalinder. I think it's safest to start fixes & improvments with some automated tests. If you feel like contributing for this part, very much appreciated! Along the way I also discovered the mentions in #10

https://github.com/sinanpl/OaxacaBlinder/issues/10#issuecomment-2002588444

davidskalinder commented 5 months ago

Excellent, sounds good -- I'll try to develop to tests as much as possible (though I'm somewhat of a n00b at coding to tests so it might be rough around the edges for a while).

Am I right that the branch with the testing framework hasn't been merged in yet?

davidskalinder commented 5 months ago

Oh, and @sinanpl here's a question for you on this: what do you think of using tidyverse packages in tests? I think the dependencies for OaxacaBlinder are admirably minimal so far, but it seems like the tidyverse tools will be especially handy for tests, where's there's bound to be plenty of data wrangling to compare results?

It looks like if we want to use tidyverse or other packages just for testing, the way to go is to list them (individually, for the tidyverse) under Suggests (probably by running usethis::use_package() with type = "Suggests") so that they won't be installed for users...

So maybe the way to go on this is to use packages fairly liberally in tests and to simply include them in Suggests in PRs that use a new package? But let me know if you'd rather be more conservative about which packages get used in testing...

sinanpl commented 5 months ago

I intentionlly avoided tidyverse during development. I really like the ecosystem and heavily use it in day-to-day tasks in R, but for package development I would push for keeping it to base R.

I don't foresee much dataframe operations for testing. my idea was to compare some simple calculations / coefficient vectors. if there is a usecase we could introduce it as you suggest, not doing that has my preference though

davidskalinder commented 5 months ago

Okay gotcha, I'll see what I can do sticking to base R even in tests. If it ends up slowing things down too much, then I'll post back to gripe and moan about it lol.

davidskalinder commented 5 months ago

Hmm, I just came across this as I was reviewing Hadley's tips for testing:

We strongly recommend that the organisation of test files match the organisation of R/ files

@sinanpl, it looks like some of the files in the tests branch depart from this guideline -- do you want to stick to Hadley's advice? Or is there another style you want to use to keep tests organized (that I should follow when writing new ones)?

davidskalinder commented 4 months ago

Hmm checking back on this, it looks like the tests branch has been deleted? @sinanpl, are you confident that what you wanted to test there has been taken care of in the tests that have already been merged in? (I did not check whether my tests overlapped what was in that branch.)

davidskalinder commented 4 months ago

Hmm checking back on this, it looks like the tests branch has been deleted?

Ah, looking back at https://github.com/sinanpl/OaxacaBlinder/pull/18#discussion_r1539906609 I see that tests was actually merged into that PR. So when we accept that one we can make sure that anything from tests that should be merged does indeed get merged.