-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upkeep 2024 #135
Upkeep 2024 #135
Conversation
@@ -90,3 +90,72 @@ test_that("plots are rendered correctly", { | |||
scale_fill_manual(values = c("grey80", "grey20")) | |||
) | |||
}) | |||
|
|||
test_that("method which return text works - snapshot", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, we were testing internal implementation details of ggplot2 by snapshotting the plot data.
We are instead interested in how the plot looks, and so a visual snapshot test is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the maintenance update. Looks all great :)
@@ -90,3 +90,72 @@ test_that("plots are rendered correctly", { | |||
scale_fill_manual(values = c("grey80", "grey20")) | |||
) | |||
}) | |||
|
|||
test_that("method which return text works - snapshot", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
Do you think the changes warrant a CRAN update or is it fine to just leave it like this as it is mostly about tests anyways? |
No, I don't think so: there hasn't been any user-visible change from any of these changes. |
No description provided.