Add snapshot tests

[Olivia-Box-Power]

No description provided.

[alice-hannah]

Hi Olivia,

This looks good and seems to work ok generally.

I noted the comment at the start of the test file to say that the R session should be restarted before running any tests. I read this and still got myself in a muddle when reviewing the code! The tests depend on the prior state, but they also change the resulting state, both of which should be avoided.

I think if it was me, I'd rather use_afcharts was untested until this could be managed. I'm not sure of the best way to do this, but these might be worth thinking about:

• Are there any withr functions that could be used to contain the changes made in the tests?

• You mentioned in Add functionality to turn off / disable afcharts #14 that there's some new ggplot functionality coming that might help with managing geom defaults. I can see dev documentation for a function to get_geom_defaults and also some code to reset_geom_defaults, both of which I think would be useful here.

My only other suggestion for this PR would be to make some improvements to the organisation of the tests:

• The tidyverse style guide says that test files should be organised to match the files in R/. So here that would mean splitting out the tests in test-chart-output.R into files test-theme_af.R, test-scale_fill_discrete_af.R, etc.

• The code defining helper functions for tests could then be moved into a helper file so they are available to all tests.

• The code to load the test data could be moved to a setup file or alternatively could just be called as e.g. ggplot2::mpg where required.

I hope that's helpful. I'll approve the PR as everything works and it's your call whether you want to make any of these changes at this time.

Thanks!
Alice

[Olivia-Box-Power]

Thanks @alice-hannah.

I agree with you about it being bad practice to change the state, so I've commented out the use_afcharts test for the time being. I couldn't see any withr function that would work for this purpose, but good thinking! I think we may as well wait for ggplot2 to be updated rather than trying to reset defaults ourselves.

I hadn't seen a test helper file before, but it looks really useful. I've moved the helper functions into the file.

I forgot I could use package:: to load datasets! I've updated throughout.

I've kept all the snapshot tests in a single file for the time being, as there's not that many of them. Agree though that if the tests become longer should move into an individual file per function.