Skip to content

Fix failing integration test#88

Open
osenan wants to merge 4 commits into
mainfrom
fix-badge-test@main
Open

Fix failing integration test#88
osenan wants to merge 4 commits into
mainfrom
fix-badge-test@main

Conversation

@osenan

@osenan osenan commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

There was a failing integration test in the badge test.
To fix it, it was changed the way we click the input and check if the expected content is there or not. I reused the testing functions of tmc. Actually, we can keep them on teal.picks and reuse them in modules packages. In that case, we probably would need to place the function within R folder.
Please run the test:
testthat::test_file("tests/testthat/test-badge_dropdown.R")

To make it sure now the test are passed.

@osenan osenan requested a review from a team June 3, 2026 14:49
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

badge

Code Coverage Summary

Filename                  Stmts    Miss  Cover    Missing
----------------------  -------  ------  -------  --------------------------------------------------------------
R/as_picks.R                140      21  85.00%   64, 168-172, 183-187, 202-208, 230-232
R/assertion.R                 5       0  100.00%
R/call_utils.R              147      22  85.03%   23-28, 65, 132-138, 261, 281-282, 285-289, 294
R/helpers.R                   9       0  100.00%
R/interaction.R              42       1  97.62%   105
R/module_merge.R            257       2  99.22%   328, 609
R/module_picks.R            324      23  92.90%   76-82, 101, 138-139, 328-330, 332-336, 474, 522, 560, 572, 580
R/picks.R                   183       1  99.45%   339
R/print.R                    36       2  94.44%   50, 58
R/resolver.R                141      15  89.36%   110-118, 284-289
R/tidyselect-helpers.R       29       0  100.00%
R/tm_merge.R                 54       0  100.00%
R/ui_containers.R            55       0  100.00%
R/zzz.R                       5       5  0.00%    3-11
TOTAL                      1427      92  93.55%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  -------
R/picks.R        -8       0  -0.02%
TOTAL            -8       0  -0.04%

Results for commit: 2c70921

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Unit Tests Summary

  1 files   11 suites   19s ⏱️
276 tests 263 ✅ 13 💤 0 ❌
412 runs  396 ✅ 16 💤 0 ❌

Results for commit 2c70921.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
badge_dropdown 👶 $+0.06$ shinytest2_badge_dropdown_badge_dropdown_is_toggled_when_clicking_and_updates_when_selecting_different_values
badge_dropdown 👶 $+0.03$ shinytest2_badge_dropdown_badge_fixed_is_visible_with_a_lock_icon_and_the_dropdown_is_not_toggleable
badge_dropdown 💀 $0.15$ $-0.15$ shinytest2_badge_dropdown_is_visible_when_clicking_on_it_multiple_times
picks 💀 $0.01$ $-0.01$ variables_allow_clear_attribute_sets_allow_clear_manually_to_FALSE
picks 💀 $0.01$ $-0.01$ variables_allow_clear_attribute_sets_allow_clear_manually_to_TRUE

Results for commit 5dbbb97

♻️ This comment has been updated with latest results.

Comment thread tests/testthat/helper-shinytest2.R Outdated
}

# Badge label may prefix variables with dataset (e.g. "ADLB BNRIND").
.teal_picks_strip_ds_prefix_vec <- function(x) { # nolint: object_length_linter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is not used in this package yet

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, removed here

Comment thread tests/testthat/helper-shinytest2.R Outdated
# Wait until a Shiny input has a non-empty DOM value (e.g. after `updateSelectInput`).
# Caller must supply the full Shiny input ID (without leading "#").
# Uses one [`AppDriver$wait_for_js()`] instead of polling `wait_for_idle()`.
wait_until_nonempty_active_module_input <- function(app_driver, input_id) { # nolint: object_length_linter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is not used in this package yet

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, removed here

@m7pr m7pr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@osenan maybe rethink which functions do we really need in this package - 2 are not used at all

@osenan

osenan commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Hi, my initial intention was to port every testing function developed in tmc to be maintained in teal.picks and then reused in tmc and other module packages. That is why I copied the full script. But let's go step by step, I have removed the non used functions. Most of them are already used only for the tests in teal.picks

@osenan osenan requested a review from m7pr June 17, 2026 10:59

@averissimo averissimo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall, but maybe we should move the new functions to the R folder (either as private or exported).

This way we can re-use them in other packages without too much repeated code.

Left some other housecleaning comments as well ;)

Comment thread tests/testthat/test-badge_dropdown.R Outdated
Comment thread tests/testthat/test-badge_dropdown.R Outdated
# When `singleton_as_bare_string` is `TRUE` and `length(val) == 1L`, return a single JSON string
# token (e.g. `"foo"`). Otherwise return a JSON array (`[]`, `["a"]`, or `["a","b"]`). The
# always-array form is used where `const arr = ...` must remain an array (DOM sync script).
.teal_picks_js_json_collection_literal <- function(val, singleton_as_bare_string) { # nolint: object_length_linter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Only used in function below, can you add a default parameter for the second parameter and replace the occurrences of teal_picks_js_string_array_literal with this one?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we add this functions here we should make them available in the R/ folder instead.

This would reduce redundant code in tmc/tmg/...

I think we can export the JS API with a app_driver or shinytest2 prefix. Or other naming convention

  • app_driver_get_teal_picks_slot
  • app_driver_set_teal_picks_slot
  • app_driver_click_teal_picks_summary_badge

Is any other function being used?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe add a checkmate::assert_class(app_driver, "AppDriver") to those functions just in case

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that adding them to the R folder is the best idea. I suggest not to export them but to export them using getFromNamespace

@averissimo averissimo self-assigned this Jun 22, 2026
osenan and others added 2 commits June 22, 2026 15:21
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants