insightsengineering / teal

Exploratory Web Apps for Analyzing Clinical Trial Data
https://insightsengineering.github.io/teal/
Other
168 stars 33 forks source link

Fixes integration tests when Chrome >=105 is not installed #1202

Closed donyunardi closed 2 months ago

donyunardi commented 3 months ago

Related to #1195

I'm focusing on test-shinytest2-filter_panel to gain more insight during the integration test.

Changes description

github-actions[bot] commented 3 months ago

badge

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  --------------------------------------------------------------------------------------------------------------------------------------------------
R/dummy_functions.R                  36      25  30.56%   21-37, 40-47
R/get_rcode_utils.R                  31       1  96.77%   50
R/include_css_js.R                   22      17  22.73%   12-38, 76-82
R/init.R                             86      31  63.95%   108-115, 161-162, 164, 176-197, 228-229, 231
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_bookmark_manager.R         158     125  20.89%   42-43, 57-59, 70-83, 93-143, 148-149, 189, 224-301
R/module_filter_manager.R            84      19  77.38%   38-42, 157, 162-175
R/module_nested_tabs.R              161      60  62.73%   39-112, 128, 180, 202, 224, 232, 236
R/module_snapshot_manager.R         241     178  26.14%   95-107, 136-139, 143-144, 159-169, 173-188, 190-198, 205-220, 224-228, 230-236, 239-252, 255-273, 282-298, 313-336, 339-350, 353-359, 373, 394-418
R/module_tabs_with_filters.R         76      33  56.58%   33-68, 100, 116
R/module_teal_with_splash.R         114      34  70.18%   60-95, 110, 131, 197-198
R/module_teal.R                     110      76  30.91%   52-119, 150-151, 157, 168, 181-212
R/module_wunder_bar.R                60      39  35.00%   23-41, 55-64, 68-77
R/modules.R                         159      26  83.65%   127-130, 147-151, 206-209, 291-292, 344, 356-364, 418-421
R/reporter_previewer_module.R        19       2  89.47%   30, 34
R/show_rcode_modal.R                 19      19  0.00%    17-36
R/tdata.R                            53       1  98.11%   154
R/teal_data_module-eval_code.R       27       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                  6       0  100.00%
R/teal_reporter.R                    62       5  91.94%   69, 118-119, 122, 139
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      59      12  79.66%   137-150
R/TealAppDriver.R                   298     298  0.00%    43-612
R/utils.R                           173       1  99.42%   255
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   110-377
R/zzz.R                              12       8  33.33%   3-15
TOTAL                              2217    1072  51.65%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  --------
R/TealAppDriver.R      +25     +25  +100.00%
TOTAL                  +25     +25  -0.59%

Results for commit: 1ab6374a8ad36ea5afb1f59f5e1006dc3cb5cb57

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

github-actions[bot] commented 3 months ago

Unit Tests Summary

  1 files   30 suites   2m 53s :stopwatch: 240 tests 240 :white_check_mark: 0 :zzz: 0 :x: 505 runs  505 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 1ab6374a.

:recycle: This comment has been updated with latest results.

donyunardi commented 3 months ago

Convert to draft so it won't be accidentally merged.

donyunardi commented 3 months ago

I downloaded the internal image, install chrome, install teal and families, and I still can't reproduce the error.

One thing to note is that I originally tried to install just chromium-browser, but then I encountered a blocker because it wanted to install it from Snap. Apparently, there's another setting that you'd need to do to get snap running and after couple tries, I gave up. Instead, I downloaded the chrome debian package and install it to the container.

wget https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb
apt install ./google-chrome-stable_current_amd64.deb

I'm curious about the contents of our CI image. Is it a full Chrome installation like what I just did, or is it the Snap version of Chromium? Does it make a difference?

@cicdguy @walkowif Please, any advise here?

cicdguy commented 3 months ago

We install Chrome like so on our CI images in GitHub.

donyunardi commented 3 months ago

Oh okay, so you install the whole Chrome as well, which I think similar to what I did. Then, I am clueless on why it's still passing on my docker container.

Next thing we can try is to run CI test based on this branch where I use print to quickly see what happened. Or, I can save the log it in current directory and IDR can help locate this file.

Any other ideas?

donyunardi commented 3 months ago

@walkowif helped setup running CI test on this branch.

Here's the result log: https://nest.pages.roche.com/-/automation/systems-integration-tests/-/jobs/45805756/artifacts/public/logs/check-teal.html

My suspicion is confirmed that there's a problem with get_active_filter_vars method during CI, specifically on displayed_datasets_index variable: https://github.com/insightsengineering/teal/blob/6042f560e66a9317f2b6e905f54cc3101851ffd5/R/TealAppDriver.R#L241-L257

The value of displayed_datasets_index is null as you can see in the log:

  {shinytest2} R  info   16:12:43.47 Value for displayed_datasets_index: 
  {shinytest2} R  info   16:12:43.47 Value for available_datasets: iris, mtcars

However, it's odd that it's able to retrieve the values for available_datasets, which is embedded in the active-filter_active_vars_contents class.

I wonder if it has something to do with the selector statement inside is_visible method because I can clearly see the span tag in the log file. Here's an example for one of the module:

<div id=\"teal-main_ui-root-module_1-module_filter_panel-active-filter_active_vars_contents\">\n                  <span id=\"teal-main_ui-root-module_1-module_filter_panel-active-iris\">

is_visible should return TRUE.

Any idea? @insightsengineering/nest-core-dev

donyunardi commented 3 months ago

@averissimo Feel free to use this branch (add new commits, etc) to investigate further.

If you need to trigger the CI on this branch, please go to: https://code.roche.com/nest/automation/systems-integration-tests/-/pipelines/new Select branch troubleshoot-teal and Run pipeline.

averissimo commented 2 months ago

Fixed the problem with this PR, the integration tests are running again without the logs before making it ready for review.

An alternative solution would be to install Chrome >=105 on the container image. @cicdguy @walkowif (Chrome 90 is 3 years old)

averissimo commented 2 months ago

Pipeline is confirmed successful: https://nest.pages.roche.com/-/automation/systems-integration-tests/-/jobs/46148438/artifacts/public/logs/check-teal.html

The job fails, but not due to teal.

m7pr commented 2 months ago

You are the legend @averissimo

github-actions[bot] commented 2 months ago

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
shinytest2-filter_panel 💚 $17.45$ $-3.23$ $-2$ $0$ $0$ $+2$
shinytest2-teal_slices 💚 $13.70$ $-4.95$ $-16$ $0$ $0$ $+2$
Additional test case details | Test Suite | $Status$ | Time on `main` | $±Time$ | Test Case | |:-----|:----:|:----:|:----:|:-----| | shinytest2-filter_panel | 💚 | $6.08$ | $-1.87$ | e2e_filtering_a_module_specific_filter_is_not_refected_in_other_unshared_modules | | shinytest2-filter_panel | 💚 | $6.06$ | $-1.86$ | e2e_filtering_a_module_specific_filter_is_refected_in_other_shared_module | | shinytest2-teal_slices | 💚 | $6.08$ | $-1.63$ | e2e_teal_slices_filters_are_initialized_when_global_filters_are_created | | shinytest2-teal_slices | 💚 | $7.61$ | $-3.31$ | e2e_teal_slices_filters_are_initialized_when_module_specific_filters_are_created |

Results for commit 581bd626aa9fc8d368480b666ea688591ca4fd89

♻️ This comment has been updated with latest results.

walkowif commented 2 months ago

@averissimo I checked that it should be possible to install Chromium 120 in the images with R 4.3. @cicdguy We can discuss if there are any potential implications of such change.

cicdguy commented 2 months ago

@averissimo I checked that it should be possible to install Chromium 120 in the images with R 4.3.

@cicdguy We can discuss if there are any potential implications of such change.

Yes let's please install the latest version of Chromium on the container images.

I believe we were installing Chrome due to Chromium only being available via Snapcraft - the latter requiring a host-level socket to be mounted on the container at build-time for it to work. Perhaps that's evolved for the better.

averissimo commented 2 months ago

Good to know :heart: !

This PR patches the problem and creates a fallback solution for the lack of support of the relevant API.

However, I think it would be best to bridge the gap between the versions of chrome among platforms (local dev, GH and integration).

walkowif commented 2 months ago

Just wanted to confirm that the images used for integration tests have been updated to include Chromium 120.

averissimo commented 2 months ago

@walkowif , I believe the pipeline that was created for this branch can be removed (nest/automation/systems-integration-tests@troubleshoot-teal)

Thanks for the help with setting that up and for updating Chromium :1st_place_medal: