Suggestions from a user

[crew102]

Greetings -

I was recently working with @daattali on integrating visual tests into ggExtra using the vdiffr framework. I wanted to get your thoughts on a few small changes that I thought may benefit vidffr. If you like one of them, I'd be happy to work on a PR to implement it.

1. Export write_svg. I initially struggled to get my visual tests to pass, mostly because the way I was saving my baseline figures (i.e., the figures in tests/figs) was different from how vdiffr was saving the figures during the tests (via write_svg). Although the behavior of write_svg is indirectly documented in the README, it took me awhile to realize that I had to mimic that behavior when creating the baseline plots. I think the easiest way to address this would be to just export write_svg, and suggest that the user use this function when saving their baseline figures.

2. Make a suggestion as to how the user can re-use the code that creates their baseline figures when writing their unit tests. The solution that we ended up using was to save the plot-rendering functions in a list, which is created in a "helper" file in tests/testthat. That way we could source the helper file when creating the baseline figures, and rely on this code when running the unit tests as well. I couldn't figure out exactly when you did in Use vdiffr for visual unit tests tidyverse/ggplot2#1874, but if you have another suggestion for how users can reuse their plot-generating functions, I think it would be a nice addition to the documentation.

3. This may be too formal, but I was thinking it would be useful for the package to suggest where users should save their script that renders their baseline figures. We saved ours within in the test/testthat directory.

I that that creating a minimal example of a package that uses the vdiffr framework inside vdiffr would address numbers 2 and 3, and number 1 is straightforward.

Chris

[daattali]

I haven't personally used vdiffr but based on what I saw, I aggree that #2 and #3 would be useful. And if Chris is offering to help, you should consider taking him up on the offer, he did a great job improving my package

…

On May 23, 2017 9:02 PM, "Chris Baker" ***@***.***> wrote: Greetings - I was recently working with @daattali <https://github.com/daattali> on integrating visual tests into ggExtra using the vdiffr framework. I wanted to get your thoughts on a few small changes that I thought may benefit vidffr. If you like one of them, I'd be happy to work on a PR to implement it. 1. Export write_svg. I initially struggled to get my visual tests to pass, mostly because the way I was saving my baseline figures (i.e., the figures in tests/figs) was different from how vdiffr was saving the figures during the tests (via write_svg). Although the behavior of write_svg is indirectly documented in the README, it took me awhile to realize that I had to mimic that behavior when creating the baseline plots. I think the easiest way to address this would be to just export write_svg, and suggest that the user use this function when saving their baseline figures. 2. Make a suggestion as to how the user can re-use the code that creates their baseline figures when writing their unit tests. The solution that we ended up using was to save the plot-rendering functions in a list, which is created in a "helper" file in tests/testthat <https://github.com/daattali/ggExtra/blob/master/tests/testthat/helper-funs.R>. That way we could source the helper file when creating the baseline figures, and rely on this code when running the unit tests as well. I couldn't figure out exactly when you did in tidyverse/ggplot2#1874 <tidyverse/ggplot2#1874>, but if you have another suggestion for how users can reuse their plot-generating functions, I think it would be a nice addition to the documentation. 3. This may be too formal, but I was thinking it would be useful for the package to suggest where users should save their script that renders their baseline figures. We saved ours within in the test/testthat directory. I that that creating a minimal example of a package that uses the vdiffr framework inside vdiffr would address numbers 2 and 3, and number 1 is straightforward. Chris — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/lionel-/vdiffr/issues/16>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA6IFP0X3LnZlxNwlsUG6_zKohel2TbEks5r84GVgaJpZM4NkeSe> .

[lionel-]

it would be useful for the package to suggest where users should save their script that renders their baseline figures

Yes a testthat helper file seems like the right place. It's automatically sourced when devtools::load_all() is run.

The solution that we ended up using was to save the plot-rendering functions in a list

Why in a list instead of normally defined functions? I think for ggplot2 I was just defining a baseline plot (rather than a function generating a baseline plot) in each file then modify it from there, but I don't remember. If the baseline plots are used all over the test files it definitely makes sense to define them in a helper file.

I think an introductory vignette would be a good place to help users getting up to speed. We'd love a PR!

[crew102]

Putting the plot-rendering functions is a list just allowed us to call them in a loop without having to list out their names first (e.g., using names(funList) here https://github.com/daattali/ggExtra/blob/master/tests/testthat/render-figs.R#L46-L48 instead of c("fun 1", "fun 2", etc) . It probably makes more sense to not use a list when documenting vidffr, just to keep the example minimal.

Yes a testthat helper file seems like the right place. It's automatically sourced when devtools::load_all() is run.

Just to clarify, I assume you are referring to where to store the plot-render functions, not the script that calls those functions. In number 3 I was actually suggesting that vdiffr indicate where to store the script that renders the baseline plots.

I'll get working on a vignette! Can I assume that write_svg will be exported in the next version?

[lionel-]

Putting the plot-rendering functions is a list just allowed us to call them in a loop

Ah yes, makes sense.

I was actually suggesting that vdiffr indicate where to store the script that renders the baseline plots

I would just organise the code within helper- and test- files to keep things simple.

Can I assume that write_svg will be exported in the next version?

I guess it doesn't hurt to export it. But I'm not sure I understand your workflow. Why not develop the test within an expect_doppelganger() statement and check how things are going with the Shiny app?

[crew102]

Annnnnd I just realized that I totally misunderstood the intended vdiffr workflow. Here's what we did:

1. Write functions that return figures, and save those functions in a helper file.
2. Write a script that calls the functions defined in helper file. This was the script that "renders the baseline figures." It was run once, to create our expected plots in the figs dir. We need to use write_svg in this file, so that our baseline figures were rendered in exactly the same way that vidffr renders them during the unit tests.
3. Write our actual unit tests, in which we feed the functions defined in the first step into expect_doppelganger() calls.

The reason why I thought we needed step 2 was that vdiffr skips those tests for which it cannot find a baseline figure. Instead, you get something like this:

Skipped ---------------------------------------------------
1. some cool test (@test-all.R#4) - Figure not generated yet: awesomefile.svg

Which I interpreted as meaning that we needed to create the figure. But I can see now that the expectation was that the user would use the app the first time around to create these figures...Correct? That makes a lot more sense (and would resolve all of my issues).

It may make sense to explicetly suggest in the testthat warning that the user use the app to create a figure if doesn't already exist (e.g., change to "Figure not generated yet: awesomefile.svg. Use the web app to create it."), and/or adding a note about this in the README.