-
-
Notifications
You must be signed in to change notification settings - Fork 103
test: add failing tests for check_model() theme argument bug (#851) #869
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
base: main
Are you sure you want to change the base?
test: add failing tests for check_model() theme argument bug (#851) #869
Conversation
…ts#851) - Tests document expected behavior of theme argument - Tests currently FAIL due to bugs in see package - Covers: standard themes, custom themes, string themes, attribute storage - Bug is in see/R/plot.check_model.R (hardcoded args issue) Relates to easystats#851
Summary of ChangesHello @ANAMASGARD, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds a comprehensive set of failing tests to pinpoint and document a critical bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new test file (test-check_model_theme.R) to address issue #851, which concerns the theme argument in the check_model() function not working as expected. The tests cover various scenarios, including standard ggplot2 themes, custom themes, and theme attribute storage. The tests are currently failing due to bugs in the see package, as noted in the pull request description. The pull request provides a comprehensive set of tests to validate the expected behavior of the theme argument once the underlying bugs in the see package are resolved.
|
|
||
| # Test with theme_dark passed as function (not string) | ||
| expect_no_error({ | ||
| p1 <- check_model(m, theme = ggplot2::theme_dark) |
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.
It's good to see the explicit namespace call here (ggplot2::theme_dark). This ensures that the correct theme is being called, especially in environments where multiple packages might have functions with the same name. However, consider adding a comment to explain why the namespace is explicitly called out, as it might not be obvious to future readers.
p1 <- check_model(m, theme = ggplot2::theme_dark) # Explicit namespace call for clarity and to avoid conflicts| theme_attr <- attr(p1, "theme") | ||
| expect_true(is.function(theme_attr) || is.character(theme_attr)) |
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.
This assertion checks that the theme attribute is either a function or a character. While this is a good start, consider adding a more specific check to ensure that the function or character value corresponds to a valid theme. For example, if it's a character, check if the theme exists in the ggplot2 namespace. If it's a function, check if it returns a ggplot2 theme object.
theme_attr <- attr(p1, "theme")
expect_true(is.function(theme_attr) || is.character(theme_attr))
# Add more specific checks to validate the theme
if (is.character(theme_attr)) {
expect_true(exists(theme_attr, where = asNamespace("ggplot2")), info = "Theme string should exist in ggplot2 namespace")
} else if (is.function(theme_attr)) {
expect_s3_class(theme_attr(), "theme", info = "Theme function should return a ggplot2 theme object")
}| # Test with string | ||
| p2 <- check_model(m, theme = "ggplot2::theme_minimal") | ||
| expect_true(!is.null(attr(p2, "theme"))) | ||
| expect_equal(attr(p2, "theme"), "ggplot2::theme_minimal") |
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.
This test uses expect_equal to check if the theme attribute is stored correctly as a string. While this works, it might be more robust to use expect_identical to ensure that the attribute is exactly the same, including its attributes. This can help catch subtle differences that expect_equal might miss.
p2 <- check_model(m, theme = "ggplot2::theme_minimal")
expect_true(!is.null(attr(p2, "theme")))
expect_identical(attr(p2, "theme"), "ggplot2::theme_minimal") # Use expect_identical for stricter comparison| # ----------------------------------------------------------------------------- | ||
| # This tests that the plot method's style parameter can override the theme | ||
| # set during check_model() call. | ||
| test_that("plot style argument overrides check_model theme", { |
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.
Consider adding a comment to clarify the purpose of this test. It's not immediately obvious why we're testing the default theme behavior separately. A brief explanation can help improve the test's readability and maintainability.
# Test 5: Default theme should work when no theme specified
# -----------------------------------------------------------------------------
# Tests that check_model works correctly when no theme argument is provided,
# ensuring it falls back to the default theme.
test_that("check_model works without theme argument (default behavior)", {…ats#851) Tests document expected behavior of theme argument: - Standard ggplot2 themes (theme_dark, theme_minimal, theme_bw) - Custom user-defined theme functions - Theme as string for backward compatibility - Theme attribute storage and retrieval Changes from review: - Remove trailing whitespace - Add namespace clarity comments - Use expect_identical for stricter comparison - Validate theme functions return theme objects - Add explanatory comment for default theme test Note: The actual bug is in the see package's plot.check_model() function which passes hardcoded arguments that don't exist in standard ggplot2 themes. This requires a fix in the see repository. Relates to easystats#851
Current StatusThis PR adds comprehensive failing tests that document the expected behavior of the Root Cause LocationThe actual bug is not in the Bug Location: Problem: The What This PR Provides✅ Comprehensive test coverage documenting three bug scenarios:
✅ Tests validate:
Next StepsI plan to:
Related Links
|
|
@ANAMASGARD Thanks a lot for this support! I'm currently busy with teaching, but I'll get back to this early November. Just to inform you and to let you know your PRs are recognized :-) |
|
@strengejacke Thank you so much sir , please review it and give your feedback and let me know if any further changes I should make . |
|
@strengejacke Also Sir can you recommend me some blogs or resources so that i can understand easystats better , so that I can contribute better . |
Relates to #851