Capture output using snapshots

[DavisVaughan]

This PR makes your package compatible with the next version of dplyr:

You were explicitly testing the message that left_join() returns when it tells you the common variables it joins by. We highly discourage this, because you don't own that message, and in fact we have changed it in the next version of dplyr. If you must do this, use a snapshot test so that it won't break on CRAN if we do change it. That is what I've done here (captured with CRAN dplyr), but I'd encourage you to just remove these tests altogether if you really don't care about them.

We plan to submit dplyr 1.1.0 on January 27th.

This should be compatible with both dev and CRAN dplyr. It would help us out if you could go ahead and send a patch version of your package to CRAN ahead of time! Thanks!

[DavisVaughan]

Also, I noticed you source a file in your tests

pknca/tests/testthat/test-dplyr.R

Line 1 in 46a7091

source("generate.data.R")

If you just name the file with a helper- prefix, then it will automatically be loaded when you run tests or do load_all() and you won't need to source it

[billdenney]

Thanks for the PR! I do want to keep those tests because I want to confirm that the join occurs based on the expected columns. While I know that others don't prefer the practice, I do test for message, warning, and error text because I want to ensure that the actual cause of the condition is the one I'm expecting and not an unexpected condition. I'll take a look at the new dplyr behavior and see how I can test better without using the text itself.

For the pointer to use the helper-* prefix, I thought that functionality was not recommended (based on r-lib/testthat#1206). I need the functionality like that (and I don't want it to go away), so I use source. And, the functions in those files are not needed during load_all(). Is helper the right way?

[DavisVaughan]

I'll take a look at the new dplyr behavior and see how I can test better without using the text itself.

Just use the snapshot tests I gave you then

[DavisVaughan]

We reverted the idea of removing helper files since we find them pretty helpful still r-lib/testthat#1626

[billdenney]

@DavisVaughan, I'd prefer to do a stronger test than the snapshot test. The reason for this is because some of my message tests have caught bugs introduced in other packages. I'll make the testing more dynamic to the version of dplyr by capturing the expected message from a well-controlled join.

Do you think a PR to dplyr would be accepted to add a class and the by information to the inform() call here? https://github.com/tidyverse/dplyr/blob/2f4b21501a683d88f029f0e664d2de95b52614cf/R/join-by.R#L368

Classing the message and adding the information seems like it would be the most robust.

[DavisVaughan]

We do not have any plans to add more information to that message because we do not believe developers should be relying on its output like that, no.

I strongly suggest that you just use a snapshot test. It is by far the most robust and lowest pain way (for both you and us) to check that the output is what you expect it to be while also never failing on CRAN.

[billdenney]

@DavisVaughan, I fixed it a different way in #199 (by capturing the expected message from a known-good join and using that). I tested my update with both the CRAN and development versions of dplyr.

I will release to CRAN likely later tonight.

[billdenney]

Actually, I just remembered that I wanted to have #197 fixed soon, so it won't be tonight, but I will have it before the dplyr submission to CRAN.