Skip to content

Comments

Set minimal shiny version for a snapshots#342

Open
llrs-roche wants to merge 3 commits intomainfrom
snapshots
Open

Set minimal shiny version for a snapshots#342
llrs-roche wants to merge 3 commits intomainfrom
snapshots

Conversation

@llrs-roche
Copy link
Contributor

Pull Request

Fixes #341

  • Sets minimal shiny version for a snapshot test as it seems shiny version affects snapshots
  • In interactive testing I saw some warning related to modal_content, the second commit on R/verbatim_popup.R is to remove them and simplify a bit the code

@github-actions
Copy link
Contributor

badge

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  ------------------------------------------------------
R/basic_table_args.R             23       0  100.00%
R/draggable_buckets.R            87      12  86.21%   116-121, 151-156
R/get_dt_rows.R                  13       0  100.00%
R/ggplot2_args.R                 49       0  100.00%
R/nested_closeable_modal.R       20      20  0.00%    83-103
R/optionalInput.R               255      79  69.02%   190, 315-389, 402-409, 411-416, 434, 436, 501, 583-596
R/panel_group.R                  39      39  0.00%    50-136
R/plot_with_settings.R          309      16  94.82%   299-305, 327, 364, 373-374, 390, 578-579, 581, 583
R/standard_layout.R              52       4  92.31%   90-93
R/table_with_settings.R         199      24  87.94%   32-36, 82, 127-158
R/utils.R                         7       0  100.00%
R/verbatim_popup.R              104      35  66.35%   112-113, 115, 123-154
R/white_small_well.R              7       0  100.00%
TOTAL                          1164     229  80.33%

Diff against main

Filename              Stmts    Miss  Cover
------------------  -------  ------  -------
R/verbatim_popup.R       -1       0  -0.32%
TOTAL                    -1       0  -0.02%

Results for commit: 6f4e443

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Feb 19, 2026

Unit Tests Summary

  1 files   16 suites   1m 55s ⏱️
172 tests 171 ✅ 0 💤 1 ❌
399 runs  398 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit f31b4cb.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 19, 2026

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
plot_with_settings_ui 💚 $52.40$ $-1.24$ $0$ $0$ $0$ $0$

Results for commit 7e102fe

♻️ This comment has been updated with latest results.

@osenan osenan self-assigned this Feb 19, 2026
@donyunardi
Copy link
Contributor

I never like snapshot test. Should we take this opportunity to change this test to something else? Maybe just regex if the html expression exist?

Copy link
Contributor

@osenan osenan left a comment

Choose a reason for hiding this comment

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

God job! Only a minor comment

@llrs-roche
Copy link
Contributor Author

llrs-roche commented Feb 20, 2026

I never like snapshot test. Should we take this opportunity to change this test to something else? Maybe just regex if the html expression exist?

In this case we are testing the UI. We could just use regex that the labels/options are there but that wouldn't test if the html changed or that we get it.
Parsing XML/HTML with regex is a path to disaster but we could test that we get an object of a given class (not sure if this is html or shiny.tag or something else). Or maybe just testing that the label shows up on the resulting UI is enough?

Copy link
Contributor

@donyunardi donyunardi left a comment

Choose a reason for hiding this comment

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

I think we need to investigate this a bit deeper. It's not clear why it's failing in the first place as I have other PR in this repo that passes R CMD Check using the latest shiny version.

Putting Request Changes so we don't merge accidentally during investigation.

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.

[Bug]:Snapshot failures found during R CMD Check

3 participants