Skip to content
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

712 - {shinytest2} for tm_a_pca #716

Merged
merged 31 commits into from
Apr 22, 2024
Merged

712 - {shinytest2} for tm_a_pca #716

merged 31 commits into from
Apr 22, 2024

Conversation

averissimo
Copy link
Contributor

@averissimo averissimo commented Apr 10, 2024

Pull Request

Part of #712

Changes description

  • Adds 2 new e2e tests using shinytest2
    • Data extract inputs
      • Main
      • Available on specific plots
    • Encoding options via "(guided) monkey typing"
  • Fixes typo on module

Considerations

  • End-2-End tests are complex and require complex set of expectations
    • Otherwise, we risk having a very long testing pipeline (AppDriver has a relevant start-up time)
  • How complete do we want to be on the encoding testing?
    • Took a brute force approach here, but small changes/removals will break the tests

@averissimo averissimo marked this pull request as ready for review April 10, 2024 15:32
Copy link
Contributor

github-actions bot commented Apr 10, 2024

badge

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  ------------------------------------
R/tm_a_pca.R                    833     833  0.00%    108-1074
R/tm_a_regression.R             779     779  0.00%    153-1037
R/tm_data_table.R               184     184  0.00%    93-330
R/tm_file_viewer.R              172     172  0.00%    44-250
R/tm_front_page.R               132     121  8.33%    70-226
R/tm_g_association.R            336     336  0.00%    135-543
R/tm_g_bivariate.R              678     416  38.64%   303-775, 816, 927, 944, 962, 973-995
R/tm_g_distribution.R          1056    1056  0.00%    122-1317
R/tm_g_response.R               352     352  0.00%    154-579
R/tm_g_scatterplot.R            728     728  0.00%    230-1059
R/tm_g_scatterplotmatrix.R      284     265  6.69%    165-478, 539, 553
R/tm_missing_data.R            1075    1075  0.00%    92-1323
R/tm_outliers.R                 991     991  0.00%    134-1269
R/tm_t_crosstable.R             257     257  0.00%    141-446
R/tm_variable_browser.R         829     824  0.60%    79-1069, 1107-1291
R/utils.R                        99      96  3.03%    82-267
R/zzz.R                           2       2  0.00%    2-3
TOTAL                          8787    8487  3.41%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: aa9d18c

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Apr 10, 2024

Unit Tests Summary

  1 files   20 suites   9m 59s ⏱️
103 tests 103 ✅ 0 💤 0 ❌
418 runs  418 ✅ 0 💤 0 ❌

Results for commit aa9d18c.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Apr 10, 2024

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
shinytest2-tm_a_pca 👶 $+96.27$ $+46$ $0$ $0$ $0$
shinytest2-tm_a_regression 💚 $42.59$ $-1.56$ $0$ $0$ $0$ $0$
shinytest2-tm_file_viewer 💚 $17.85$ $-1.17$ $0$ $0$ $0$ $0$
shinytest2-tm_g_scatterplot 💔 $72.67$ $+1.30$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
shinytest2-tm_a_pca 👶 $+8.58$ e2e_tm_a_pca_Changing_output_encodings_for_plot_type_does_not_generate_errors
shinytest2-tm_a_pca 👶 $+7.90$ e2e_tm_a_pca_Changing_output_encodings_of_NA_action_does_not_generate_errors
shinytest2-tm_a_pca 👶 $+27.82$ e2e_tm_a_pca_Changing_output_encodings_of_font_size_does_not_generate_errors
shinytest2-tm_a_pca 👶 $+7.12$ e2e_tm_a_pca_Changing_output_encodings_of_plot_type_hides_and_shows_options
shinytest2-tm_a_pca 👶 $+4.99$ e2e_tm_a_pca_Changing_output_encodings_of_standardization_does_not_generate_errors
shinytest2-tm_a_pca 👶 $+6.01$ e2e_tm_a_pca_Changing_output_encodings_of_tables_display_does_not_generate_errors
shinytest2-tm_a_pca 👶 $+6.78$ e2e_tm_a_pca_Changing_output_encodings_of_theme_does_not_generate_errors
shinytest2-tm_a_pca 👶 $+7.47$ e2e_tm_a_pca_Color_by_columns_data_extract_must_be_from_non_selected_variable_set
shinytest2-tm_a_pca 👶 $+5.49$ e2e_tm_a_pca_Eigenvector_table_should_have_data_extract_selection_Murder_Assault_on_header
shinytest2-tm_a_pca 👶 $+7.49$ e2e_tm_a_pca_Eigenvector_table_should_have_data_extract_selection_Murder_UrbanPop_on_header
shinytest2-tm_a_pca 👶 $+6.61$ e2e_tm_a_pca_module_is_initialised_with_the_specified_defaults_in_function_call
shinytest2-tm_a_regression 💚 $5.66$ $-1.15$ e2e_tm_a_regression_data_parameter_and_module_label_is_passed_properly

Results for commit 4d8b1d6

♻️ This comment has been updated with latest results.

@kartikeyakirar kartikeyakirar mentioned this pull request Apr 11, 2024
19 tasks
@m7pr
Copy link
Contributor

m7pr commented Apr 11, 2024

I see you did prefix a couple of functions with ggplot2::
Should you also prefix + operator with ggplot2::+`` : P ?

{tmg} depends on ggplot2. Should we really prefix https://github.com/insightsengineering/teal.modules.general/blob/main/DESCRIPTION#L27 ? I was trying to find out our agreement on how we handle external dependencies but couldn't find the issue :P

@m7pr
Copy link
Contributor

m7pr commented Apr 11, 2024

@averissimo do you think your tests could benefit from a app$get_screenshot + app$expect_screenshot approach?

https://github.com/insightsengineering/teal.widgets/blob/e1d81a91fd37c327cf4ce01e09133bdaa2bfe259/tests/testthat/test-plot_with_settings_ui.R#L9-L41

@m7pr m7pr changed the title Adds automated tests to tm_a_pca module 712 - {shinytest2} for tm_a_pca Apr 11, 2024
@averissimo
Copy link
Contributor Author

{tmg} depends on ggplot2. Should we really prefix https://github.com/insightsengineering/teal.modules.general/blob/main/DESCRIPTION#L27 ? I was trying to find out our agreement on how we handle external dependencies but couldn't find the issue :P

Without the prefix many of the modules will fail to be tested. I believe we have to do it inside of eval_code / within.

ggmosaic is currently being prefixed in {tmg} even as it's a Depends.

@averissimo do you think your tests could benefit from a app$get_screenshot + app$expect_screenshot approach?

I was wondering the same, they are very explicit in what they are testing

  • the presence of a row in the table
  • the visibility of an encoding

Snapshot / screenshot would work, but it requires an eagle eye (making sure the spirit of the test is kept) for the future if re-generating the snapshots again.

m7pr
m7pr previously requested changes Apr 16, 2024
Copy link
Contributor

@m7pr m7pr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename set_module_input to set_active_module_input

tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_pca.R Outdated Show resolved Hide resolved
@averissimo averissimo dismissed m7pr’s stale review April 18, 2024 13:58

Stale (rename of function was addressed)

@m7pr m7pr removed their request for review April 19, 2024 11:20
@averissimo averissimo enabled auto-merge (squash) April 22, 2024 09:25
@averissimo averissimo merged commit 0e61dc4 into main Apr 22, 2024
21 checks passed
@averissimo averissimo deleted the 712-tm_a_pca@main branch April 22, 2024 11:43
@kartikeyakirar
Copy link
Contributor

app_driver$stop() calls are misisng added here 35b0d36

@averissimo
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants