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_regression #718

Merged
merged 43 commits into from
Apr 19, 2024
Merged

712 - {shinytest2} for tm_a_regression #718

merged 43 commits into from
Apr 19, 2024

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Apr 11, 2024

Part of #712

@m7pr m7pr added the core label Apr 11, 2024
@m7pr m7pr marked this pull request as ready for review April 11, 2024 13:14
@m7pr m7pr requested a review from vedhav April 11, 2024 13:15
@m7pr m7pr mentioned this pull request Apr 11, 2024
19 tasks
Copy link
Contributor

github-actions bot commented Apr 11, 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              674     412  38.87%   308-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            1063    1063  0.00%    92-1311
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                          8771    8471  3.42%

Diff against main

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

Results for commit: 76558fd

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Apr 11, 2024

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
shinytest2-tm_a_regression 👶 $+41.93$ $+31$ $0$ $0$ $0$
shinytest2-tm_file_viewer 💚 $17.71$ $-1.15$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
shinytest2-tm_a_regression 👶 $+4.63$ e2e_tm_a_regression_data_extract_spec_elements_are_initialized_with_the_default_values_specified_by_response_and_regressor_arg
shinytest2-tm_a_regression 👶 $+5.45$ e2e_tm_a_regression_data_parameter_and_module_label_is_passed_properly
shinytest2-tm_a_regression 👶 $+4.66$ e2e_tm_a_regression_outlier_definition_and_label_are_visible_by_default
shinytest2-tm_a_regression 👶 $+4.89$ e2e_tm_a_regression_outlier_definition_and_label_have_default_values_and_label_text
shinytest2-tm_a_regression 👶 $+11.24$ e2e_tm_a_regression_plot_type_has_7_specific_choices_changing_choices_does_not_throw_errors
shinytest2-tm_a_regression 👶 $+5.64$ e2e_tm_a_regression_plot_type_is_set_properly
shinytest2-tm_a_regression 👶 $+5.42$ e2e_tm_a_regression_unchecking_display_outlier_hides_outlier_label_and_definition

Results for commit 56a0010

♻️ This comment has been updated with latest results.

Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

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

ggplot(), aes_string(), aes(), geom_point(), stat_smooth() and others are not prefixed :-\ and leads to the following content on the app

image

tests/testthat/test-shinytest2-tm_a_regression.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_regression.R Outdated Show resolved Hide resolved
tests/testthat/test-shinytest2-tm_a_regression.R Outdated Show resolved Hide resolved
m7pr and others added 4 commits April 12, 2024 11:06
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@m7pr
Copy link
Contributor Author

m7pr commented Apr 12, 2024

@averissimo this is weird, since this was no longer the case when I added require(ggplot2) to teal_data() but maybe I was working on library(teal.modules.general) and had ggplot2 attached since I see regular plots

image

@m7pr
Copy link
Contributor Author

m7pr commented Apr 12, 2024

Ah, I see those warnings in a clean session. ?Will ?prefix ?ggplot? !yaiks!

> devtools::load_all(".")
ℹ Loading teal.modules.general
Registered S3 method overwritten by 'teal':
  method        from      
  c.teal_slices teal.slice
Loading required package: ggmosaic
Loading required package: ggplot2
RStudio Community is a great place to get help:
https://community.rstudio.com/c/tidyverse
Loading required package: shiny
Loading required package: teal
Loading required package: teal.data
Loading required package: teal.code
Loading required package: teal.slice

You are using teal version 0.15.2.9024

Attaching package:tealThe following objects are masked frompackage:teal.slice:

    as.teal_slices, teal_slices

Loading required package: teal.transform
Registered S3 method overwritten by 'tern':
  method   from 
  tidy.glm broom
Warning messages:
1: packageggmosaicwas built under R version 4.3.1 
2: packageggplot2was built under R version 4.3.2 
3: packageshinywas built under R version 4.3.2 
4: packageteal.datawas built under R version 4.3.2 
5: packagetestthatwas built under R version 4.3.2 
>   app <- app_driver_tm_a_regression()
[INFO] 2024-04-12 11:17:30.4888 pid:2956 token:[] teal.modules.general Initializing tm_a_regression
> app$open_url()
image

@averissimo
Copy link
Contributor

Yeah, been there and got surprised when @vedhav pointed it to me that it wasn't working out of the box.

Unfortunately, {shinytest2} has this limitation, as I would hope we didn't have to change the modules that much

@averissimo averissimo changed the title 712 - {shinytest2} for tm_a_regerssion 712 - {shinytest2} for tm_a_regression Apr 12, 2024
Copy link
Contributor

github-actions bot commented Apr 12, 2024

Unit Tests Summary

  1 files   13 suites   4m 11s ⏱️
 67 tests  67 ✅ 0 💤 0 ❌
227 runs  227 ✅ 0 💤 0 ❌

Results for commit 76558fd.

♻️ This comment has been updated with latest results.

@m7pr
Copy link
Contributor Author

m7pr commented Apr 15, 2024

@gogonzo regarding

we still need to test output

During reifnement on Thursday we decided to focus on whether options can be checked, selected and changed. We did agree we want to test the output change only for tables. For plots we decided it is too complicated right now to verify changes on the plots.

@gogonzo
Copy link
Contributor

gogonzo commented Apr 15, 2024

During reifnement on Thursday we decided to focus on whether options can be checked, selected and changed. We did agree we want to test the output change only for tables. For plots we decided it is too complicated right now to verify changes on the plots.

Fair enough

@m7pr
Copy link
Contributor Author

m7pr commented Apr 16, 2024

Hey @gogonzo would you mind another look? I think I applied most of the comments and advices.
I am not sure about that we do with ggplot2:: prefixing yet, but this can be decided once you align with @averissimo

@m7pr m7pr requested a review from gogonzo April 16, 2024 11:42
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

I guess this is enough if we don't test the output yet

m7pr and others added 4 commits April 18, 2024 16:14
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@m7pr m7pr enabled auto-merge (squash) April 18, 2024 14:16
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

In this PR test for validation errors are missing. There are couple of validate in the srv_a_regression and none of them are tested. Same applies to other PRs in the issue. I will not block this PR, this can be done iteratively.

@m7pr m7pr merged commit cb6f918 into main Apr 19, 2024
21 checks passed
@m7pr m7pr deleted the 712-tm_a_regression@main branch April 19, 2024 10:55
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.

4 participants