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

[Feature Anywhere] Fix legacy tests / bug fixes #4327

Merged

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Jun 19, 2023

Description

This PR gets all legacy functional tests to pass by changing the following:

  • adding NPE check in the vega parser for undefined visibleVisLayers. Was a miss the first time
  • Removing unnecessary savedObjectId params in the vega expression function pipeline (and downstream vega visualization) which was leftover from the old way of propagating the data down. This was causing an issue where when creating a regular vega visualization, under certain circumstances, would cause the rendering to fail due to missed parameters when generating the vega expression function
  • Removing unnecessary calls to fetch VisLayers when rendering the embeddable outside of the context of a dashboard. This is a general improvement, but also fixes a bug where it can put the embeddable in a weird state such that the inspect button is greyed out when first loading a vis in edit view (resolves after re-loading). But this fix prevents that from happening at all

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Merging #4327 (1d54ec8) into feature/feature-anywhere (12b6d44) will decrease coverage by 0.03%.
The diff coverage is 25.00%.

@@                     Coverage Diff                      @@
##           feature/feature-anywhere    #4327      +/-   ##
============================================================
- Coverage                     66.36%   66.34%   -0.03%     
============================================================
  Files                          3271     3271              
  Lines                         62997    62998       +1     
  Branches                       9758     9759       +1     
============================================================
- Hits                          41811    41799      -12     
- Misses                        18840    18846       +6     
- Partials                       2346     2353       +7     
Flag Coverage Δ
Linux ?
Windows 66.34% <25.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/vis_type_vega/public/vega_view/vega_base_view.js 54.70% <ø> (+4.41%) ⬆️
...plugins/vis_type_vega/public/vega_visualization.js 76.66% <ø> (ø)
...ugins/vis_type_vislib/public/line_to_expression.ts 6.25% <ø> (ø)
...izations/public/embeddable/visualize_embeddable.ts 19.31% <0.00%> (-0.23%) ⬇️
...ins/vis_type_vega/public/data_model/vega_parser.ts 74.53% <100.00%> (+2.24%) ⬆️

... and 8 files with indirect coverage changes

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Copy link
Member Author

Tests are all consistently passing locally:

37 passing (12.0m)
8 pending

Errors seen in CI:

Missing elements/visualizations:

     │1)    dashboard app
     │       using current data
     │         create and add embeddables
     │           add new visualization link
     │             adds new visualiztion via the top nav link:
     │
     │      retry.try timeout: Error: 3 elements completed rendering, still waiting on a total of 4
     │                 specifically:
     │ visualization from top nav add new panel

     │2)    dashboard app
     │       using current data
     │         create and add embeddables
     │           add new visualization link
     │             adds a new visualization:
     │
     │      retry.try timeout: Error: 3 elements completed rendering, still waiting on a total of 5
     │                 specifically:
     │ visualization from top nav add new panel
     │ visualization from add new link

Other errors seen in run:

                 │ERROR browser[SEVERE] http://localhost:6610/9007199254740991/bundles/plugin/visualizations/visualizations.plugin.js 6276:14 Error: Could not locate that index-pattern (id: 1b1789d0-9e93-11ea-853e-adc0effaf76d), [click here to re-create it](management/opensearch-dashboards/indexPatterns)

This one looks related to augment-vis changes, still researching:

               │ERROR browser[SEVERE] http://localhost:6610/internal/search/opensearch - Failed to load resource: the server responded with a status of 400 (Bad Request)
               │ERROR browser[SEVERE] http://localhost:6610/api/saved_objects/_find?default_search_operator=AND&page=1&per_page=100&search_fields=title%5E3&search_fields=description&type=augment-vis - Failed to load resource: the server responded with a status of 503 (Service Unavailable)
               │ERROR browser[SEVERE] http://localhost:6610/9007199254740991/bundles/core/core.entry.js 20506:18 TypeError: Failed to fetch
                 │ERROR browser[SEVERE] http://localhost:6610/internal/search/opensearch - Failed to load resource: the server responded with a status of 500 (Internal Server Error)
                 └- ✓ pass  (5.9s) "dashboard app using current data create and add embeddables add new visualization link saves the saved visualization url to the app link"
               └-> "after all" hook for "saves the saved visualization url to the app link"
                 │ proc [opensearch-dashboards]   log   [23:30:59.211] [error][data][opensearch] [search_phase_execution_exception]: 
                 │ proc [opensearch-dashboards]  error  [23:30:59.131]  Error: Internal Server Error
                 │ proc [opensearch-dashboards]     at HapiResponseAdapter.toError (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/src/core/server/http/router/response_adapter.ts:152:19)

I will try a new local stack with vanilla OSD/ES without any external plugins, such as AD, and see if there is a difference

@ohltyler
Copy link
Member Author

Confirmed this still passes locally with vanilla OSD

@ohltyler
Copy link
Member Author

Actually - current failures exist in main as well currently. For example, latest commit right now is #4268 - see same test run failure

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler ohltyler changed the title [Feature Anywhere] Fix legacy tests [Feature Anywhere] Fix legacy tests / bug fixes Jun 21, 2023
@ohltyler
Copy link
Member Author

Remaining failures after first fix:
#4310 (comment)

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Copy link
Member Author

ohltyler commented Jun 21, 2023

Group 7 passing locally

48 passing (9.0m)
5 pending

@ohltyler ohltyler marked this pull request as ready for review June 21, 2023 18:49
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Copy link
Member Author

Last completed test run: https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/5336998643

The failures seen there are equivalent to existing failures as seen on main. More details in related issue #4310

Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are just what was pulled into the lockfile after re-running bootstrap. Prefer to include the change so it is consistent

Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

LGTM

@ohltyler ohltyler merged commit 45ecc1f into opensearch-project:feature/feature-anywhere Jun 22, 2023
27 of 42 checks passed
@ohltyler ohltyler deleted the fix-legacy-tests branch June 22, 2023 17:28
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Nice - Thanks @ohltyler for the PR summary. It sounds like we were able to catch a few important issues.

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

Successfully merging this pull request may close these issues.

None yet

4 participants