-
-
Notifications
You must be signed in to change notification settings - Fork 47
Fixes TealAppDriver & implement regression tests #1559
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?
Conversation
a4d85aa
to
8ba4976
Compare
…id will be variable)
Unit Tests Summary 1 files 26 suites 1m 58s ⏱️ Results for commit 513347f. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 1583ce1 ♻️ This comment has been updated with latest results. |
Code Coverage Summary
Diff against main
Results for commit: 513347f Minimum allowed coverage is ♻️ This comment has been updated with latest results |
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.
I see many changes to make code shorter and more readable. Thanks!
I have some questions about some tests changes but tests look good.
Some tests failed locally on windows. It can be related to Windows vs Linux or some update on chrome (I had to update it just today). See comments.
I see three tests skipped:
- test-module_teal.R:1614:7: what happens when module$label is duplicated (when nested modules)
- test-module_teal.R:1621:5: srv_teal data reload sets back the same active filters in each module
- test-module_teal.R:1624:5: srv_teal data reload doesn't fail when teal_data has no datasets
Except the first that might require a PR to merge, I think the ones about srv_teal data reload could be tested already.
I haven't check if the new tests cover well any possible regression, I'll check that on Monday.
teal.data::join_key("mtcars2", "mtcars1", keys = c("am")) | ||
) | ||
|
||
app <- TealAppDriver$new(data = data, modules = example_module()) |
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.
Not sure what is happening, but I'm seeing a warning locally:
Warning (test-shinytest2-data_summary.R:99:3): e2e: data summary table does not list unsupported objects
'package:MultiAssayExperiment' may not be available when loading
skip_if_too_deep(5) | ||
app <- TealAppDriver$new( | ||
data = simple_teal_data(), | ||
modules = example_module(label = "Example Module") | ||
) | ||
app$expect_no_shiny_error() | ||
app$expect_screenshot(selector = "#teal-tabpanel_wrapper") |
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.
Locally I got a warning about missing windows snapshot (Not sure how important this is)
Warning (test-shinytest2-init.R:11:3): e2e: teal app initializes with no errors
Adding new file snapshot: 'tests/testthat/_snaps/windows-4.5/teal-001.png'
".teal-filter-panel [id$=\\'filters-main_filter_accordian\\']", | ||
"> .accordion-item > .accordion-collapse" | ||
) | ||
testthat::expect_true(app$is_visible(filterpanel_accordion_selector)) | ||
app$click(selector = paste( | ||
".teal-filter-panel [id$='filters-main_filter_accordian']", |
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.
I fixed the typo accordian
-> accordion
in teal.slice regression tests but if you use an id there is no need to specify the class of the element as ids are unique. Start the selector with #filters-main_filter_accordion
(if insightsengineering/teal.slice#653 gets merged before this PR)
@@ -56,6 +49,7 @@ testthat::test_that("bookmark_manager_button shows modal with url containing sta | |||
|
|||
testthat::expect_match( | |||
rvest::html_text(app$get_html_rvest("div[id$=bookmark_modal] pre")), | |||
"_state_id_" | |||
"_state_id_=[a-zA-Z0-9]+", |
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 seems a bit weird. Is this the pattern for ids? Shouldn't then the regex include -
and _
? If it is for ids of elements it might accept other characters like .
or space
app$navigate_teal_tab("Report previewer") | ||
|
||
accordian_selector <- sprintf("#%s-pcards .accordion-toggle", app$active_module_ns()) | ||
app$click(selector = accordian_selector) |
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.
Her locally it FAILS I'm using the main branch of the dependencies for testing this:
Error (test-shinytest2-reporter.R:60:3): e2e: adding a report card in a module adds it in the report previewer tab
Error in `app_find_node_id(self, private, input = input, output = output,
selector = selector)`: Cannot find HTML element with selector #teal-teal_modules-nav-temporary_label-module-pcards .accordion-toggle
) | ||
) | ||
) | ||
app$click(selector = "#teal-close_teal_data_module_modal button[data-dismiss='modal']") |
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.
Not tested but if you use the id why select with attributes properties too?
app$click(selector = "#teal-close_teal_data_module_modal button[data-dismiss='modal']") | |
app$click(selector = "#teal-close_teal_data_module_modal") |
testthat::expect_false(app$is_visible(".teal-filter-panel")) | ||
testthat::expect_false(app$is_visible(".teal-active-data-summary-panel")) |
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.
I think it is best to test also that previously the panel(s) were visible. The teal filter panel could disappear and we wouldn't notice.
@@ -43,6 +43,7 @@ reporter_previewer_module <- function(label = "Report previewer", server_args = | |||
# This is to prevent another module being labeled "Report previewer". | |||
class(module) <- c(class(module), "teal_module_previewer") | |||
module$label <- label | |||
module$path <- label |
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.
Not sure what this change does, the commit message does not provide any clue and I find surprising that the same information is on two parts of the same object. Should label be in just one place?
@@ -466,7 +466,7 @@ testthat::describe("srv_teal teal_modules", { | |||
`teal_modules-active_module_id` = "module_1" | |||
) | |||
out <- modules_output$module_1() | |||
testthat::expect_true(!is.null(out)) | |||
testthat::expect_type(out, "double") |
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.
What is this magic number and how it is related to "srv_teal teal_modules are not called again when data changes"? All the previous tests expect it to be NULL?
@@ -35,3 +35,26 @@ report_module <- function(label = "example teal module") { | |||
} | |||
) | |||
} | |||
|
|||
example_teal_data_module <- function(with_submit = FALSE, once = TRUE) { |
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.
I only see it being used on teal_data_module
did you want to reuse it in other tests too?
Pull Request
Fixes #1535
Changes description
testthat::skip("chromium")
from e2e tests