Added UTs for Saved queries new UI #8798

riysaxen-amzn · 2024-11-03T05:26:55Z

Description

Created UTs for Saved queries new UI(#8469)

Issues Resolved

Screenshot

Testing the changes

yarn test:jest src/plugins/data/public/query/saved_query/saved_query_service.test.ts
yarn run v1.22.22
$ scripts/use_node scripts/jest src/plugins/data/public/query/saved_query/saved_query_service.test.ts
jest-haste-map: duplicate manual mock found: index
  The following files share their name; please delete one of them:
    * <rootDir>/src/plugins/vis_type_vega/public/expressions/__mocks__/index.ts
    * <rootDir>/src/core/server/saved_objects/import/__mocks__/index.ts

 PASS  src/plugins/data/public/query/saved_query/saved_query_service.test.ts (5.786 s)
  saved query service
    saveQuery
      ✓ should create a saved object for the given attributes (3 ms)
      ✓ should allow overwriting an existing saved query (1 ms)
      ✓ should optionally accept filters and timefilters in object format (1 ms)
      ✓ should throw an error when saved objects client returns error
      ✓ should throw an error if the saved query does not have a title (1 ms)
      ✓ should include dataset in the query when query enhancement is enabled and dataset exists (1 ms)
      ✓ should include isTemplate in the query object when new saved queries UI is enabled and isTemplate is true
    findSavedQueries
      ✓ should find and return saved queries without search text or pagination parameters (1 ms)
      ✓ should return the total count along with the requested queries
      ✓ should find and return saved queries with search text matching the title field (1 ms)
      ✓ should find and return parsed filters and timefilters items
      ✓ should return an array of saved queries
      ✓ should accept perPage and page properties (1 ms)
      ✓ should correctly parse a json query string
      ✓ should correctly parse a json object
      ✓ should handle null string with single quote
      ✓ should handle null string with double quote
      ✓ should handle null quoted string
      ✓ should not lose double quotes
    getSavedQuery
      ✓ should retrieve a saved query by id (1 ms)
      ✓ should only return saved queries
    deleteSavedQuery
      ✓ should delete the saved query for the given ID
    getAllSavedQueries
      ✓ should return all the saved queries (1 ms)
    getSavedQueryCount
      ✓ should return the total number of saved queries

yarn test:jest src/plugins/data/public/plugin.*                                     
yarn run v1.22.22
$ scripts/use_node scripts/jest src/plugins/data/public/plugin.test.ts src/plugins/data/public/plugin.ts
jest-haste-map: duplicate manual mock found: index
  The following files share their name; please delete one of them:
    * <rootDir>/src/plugins/vis_type_vega/public/expressions/__mocks__/index.ts
    * <rootDir>/src/core/server/saved_objects/import/__mocks__/index.ts

 PASS  src/plugins/data/public/plugin.test.ts (9.515 s)
  #DataPublicPlugin setup
    ✓ should setup the plugin and set useNewSavedQueriesUI (17 ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        10.377 s, estimated 14 s
Ran all test suites matching /src\/plugins\/data\/public\/plugin.test.ts|src\/plugins\/data\/public\/plugin.ts/i.
✨  Done in 16.33s.

Changelog

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

github-actions · 2024-11-03T05:27:34Z

ℹ️ Manual Changeset Creation Reminder

Please ensure manual commit for changeset file 8798.yml under folder changelogs/fragments to complete this PR.

If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link.

For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool.

github-actions · 2024-11-03T05:28:30Z

❌ Changeset File Not Added Yet

Please ensure manual commit for changeset file 8798.yml under folder changelogs/fragments to complete this PR. File still missing.

codecov · 2024-11-03T05:46:35Z

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.83%. Comparing base (78204a0) to head (557e0ce).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/data/public/autocomplete/autocomplete_service.ts	0.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8798      +/-   ##
==========================================
+ Coverage   60.78%   60.83%   +0.05%     
==========================================
  Files        3798     3798              
  Lines       90701    90701              
  Branches    14284    14284              
==========================================
+ Hits        55135    55181      +46     
+ Misses      32067    32021      -46     
  Partials     3499     3499

Flag	Coverage Δ
Linux_1	`29.05% <0.00%> (ø)`
Linux_2	`56.39% <ø> (ø)`
Linux_3	`37.74% <33.33%> (+0.08%)`	⬆️
Linux_4	`29.82% <0.00%> (ø)`
Windows_1	`29.06% <0.00%> (ø)`
Windows_2	`56.34% <ø> (ø)`
Windows_3	`37.74% <33.33%> (+0.08%)`	⬆️
Windows_4	`29.82% <0.00%> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

goyamegh · 2024-11-04T18:00:02Z

Can you add the changeset file and check codecov, failing.

virajsanghvi · 2024-11-04T23:12:35Z

Callouts:

it looks like saved query unit test is failing
Haven't looked yet, but while I assume this is just UTs, patch cov check is failing at 33%

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

riysaxen-amzn · 2024-11-04T23:23:59Z

Callouts:

it looks like saved query unit test is failing

Haven't looked yet, but while I assume this is just UTs, patch cov check is failing at 33%

yes, i observed UTs failing for saved queries, revised the Pr to fix them

virajsanghvi · 2024-11-04T23:16:59Z

src/plugins/data/public/autocomplete/autocomplete_service.ts

@@ -51,7 +51,7 @@ export class AutocompleteService {
  private getValueSuggestions?: ValueSuggestionsGetFn;

  private addQuerySuggestionProvider = (language: string, provider: QuerySuggestionGetFn): void => {
-    if (language && provider && this.autocompleteConfig.querySuggestions.enabled) {
+    if (language && provider && this.autocompleteConfig?.querySuggestions.enabled) {


what led us to using optional chaining if we're not exercising this through testing?

seems like we don't have any coverage on autocomplete_service.ts,

While writing unit tests for src/plugins/data/public/plugin, we initialize all the services, including

this.searchService = new SearchService(initializerContext); this.uiService = new UiService(initializerContext); this.queryService = new QueryService(); this.fieldFormatsService = new FieldFormatsService(); this.autocomplete = new AutocompleteService(initializerContext);

The optional chaining ensures that if autocompleteConfig or querySuggestions is not defined (i.e., null or undefined), it won’t cause a runtime error when trying to access enabled.
In short, while the autocomplete_service.ts might not have direct test coverage, the use of optional chaining here is a safety feature that prevents errors when properties or objects are not available during execution. If the service were better covered in the tests, we’d be able to verify if this behavior is actually necessary or if there is a more predictable initialization pattern that could be tested directly.

Did you add it unprompted as a safety measure, or because you saw it failing in some cases? I am curious about the other cases as well, but using this as an example.

but why would querySuggestions be undefined? it's enforced by type and config-schema which provides a default value

yes, i added it because the UTs were failing with Cannot read property of undefined for the following lines

I would prefer for the UTs to be fixed to call the stack correctly. Do you know which ones were failing? Not that this change is unsafe to have... just that those tests that are not creating the proper context could be missing stuff.

virajsanghvi · 2024-11-04T23:19:54Z

src/plugins/data/public/plugin.test.ts

+          if (setting === UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED) {
+            return true;
+          }
+          return false;


nit: return setting === UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED;

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

joshuali925 · 2024-11-05T00:03:47Z

src/plugins/data/public/plugin.test.ts

+describe('#DataPublicPlugin setup', () => {
+  let coreSetup: any;
+  let coreStart: any;
+  let plugin;


nit. avoid any if possible

joshuali925 · 2024-11-05T00:04:21Z

src/plugins/data/public/plugin.ts

@@ -183,7 +183,7 @@ export class DataPublicPlugin

    const useNewSavedQueriesUI =
      core.uiSettings.get(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED) &&
-      this.config.savedQueriesNewUI.enabled;
+      this.config.savedQueriesNewUI?.enabled;


why this change?

these files aren't covered by any UTs, so when writing tests for data/public/plugin since all these services are initialized, i was getting the error Cannot read property of undefined for all these lies [ref: https://github.com//pull/8798#discussion_r1828538194]

Ideally, we're just mocking things to not have to change the source code. It's mainly problematic if we fail in an unexpected way (there are times where failing fast and throwing is preferable to going down a code path that we didn't intend, and patch coverage % points to that being a concern here), but its usages in this CR don't seem that problematic, so I'm fine approving. Please do review however, and if the optional chaining presents a risk, let's fix in tests vs source.

joshuali925 · 2024-11-05T00:07:13Z

src/plugins/data/public/autocomplete/autocomplete_service.ts

@@ -51,7 +51,7 @@ export class AutocompleteService {
  private getValueSuggestions?: ValueSuggestionsGetFn;

  private addQuerySuggestionProvider = (language: string, provider: QuerySuggestionGetFn): void => {
-    if (language && provider && this.autocompleteConfig.querySuggestions.enabled) {
+    if (language && provider && this.autocompleteConfig?.querySuggestions.enabled) {


but why would querySuggestions be undefined? it's enforced by type and config-schema which provides a default value

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

virajsanghvi · 2024-11-06T18:45:53Z

src/plugins/data/public/plugin.ts

@@ -183,7 +183,7 @@ export class DataPublicPlugin

    const useNewSavedQueriesUI =
      core.uiSettings.get(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED) &&
-      this.config.savedQueriesNewUI.enabled;
+      this.config.savedQueriesNewUI?.enabled;


Ideally, we're just mocking things to not have to change the source code. It's mainly problematic if we fail in an unexpected way (there are times where failing fast and throwing is preferable to going down a code path that we didn't intend, and patch coverage % points to that being a concern here), but its usages in this CR don't seem that problematic, so I'm fine approving. Please do review however, and if the optional chaining presents a risk, let's fix in tests vs source.

riysaxen-amzn added 3 commits November 2, 2024 17:23

new savedQuerieTest UTS

e7cd5d4

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

UTs for newSavedQueries UI

ed9fcae

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

UTs for newSavedQueries UI

dbeeb88

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

github-actions bot added the first-time-contributor label Nov 3, 2024

github-actions bot added the failed changeset label Nov 3, 2024

Merge branch 'main' into savedQueriesUTs

52e3375

riysaxen-amzn and others added 2 commits November 4, 2024 09:14

Merge branch 'main' into savedQueriesUTs

2e2acd0

added changelogs file

272e241

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

fixed the UTs

d2fd437

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

Merge branch 'main' into savedQueriesUTs

4a6b488

virajsanghvi reviewed Nov 4, 2024

View reviewed changes

addressed the comments on saved queries UTs

2a098fc

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

riysaxen-amzn requested a review from virajsanghvi November 4, 2024 23:41

joshuali925 reviewed Nov 5, 2024

View reviewed changes

fix eslint

557e0ce

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

riysaxen-amzn requested a review from joshuali925 November 5, 2024 02:49

virajsanghvi approved these changes Nov 6, 2024

View reviewed changes

jowg-amazon mentioned this pull request Nov 12, 2024

Fix template queries loading and update getSampleQuery interface #8848

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added UTs for Saved queries new UI #8798

Added UTs for Saved queries new UI #8798

riysaxen-amzn commented Nov 3, 2024 •

edited

Loading

github-actions bot commented Nov 3, 2024

github-actions bot commented Nov 3, 2024

codecov bot commented Nov 3, 2024 •

edited

Loading

goyamegh commented Nov 4, 2024

virajsanghvi commented Nov 4, 2024

riysaxen-amzn commented Nov 4, 2024

virajsanghvi Nov 4, 2024

riysaxen-amzn Nov 4, 2024

virajsanghvi Nov 4, 2024

joshuali925 Nov 5, 2024

riysaxen-amzn Nov 5, 2024

AMoo-Miki Nov 5, 2024 •

edited

Loading

virajsanghvi Nov 4, 2024

riysaxen-amzn Nov 4, 2024

joshuali925 Nov 5, 2024

riysaxen-amzn Nov 5, 2024

joshuali925 Nov 5, 2024

riysaxen-amzn Nov 5, 2024

virajsanghvi Nov 6, 2024

joshuali925 Nov 5, 2024

virajsanghvi Nov 6, 2024

Added UTs for Saved queries new UI #8798

Are you sure you want to change the base?

Added UTs for Saved queries new UI #8798

Conversation

riysaxen-amzn commented Nov 3, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Nov 3, 2024

ℹ️ Manual Changeset Creation Reminder

github-actions bot commented Nov 3, 2024

❌ Changeset File Not Added Yet

codecov bot commented Nov 3, 2024 • edited Loading

Codecov Report

goyamegh commented Nov 4, 2024

virajsanghvi commented Nov 4, 2024

riysaxen-amzn commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AMoo-Miki Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riysaxen-amzn commented Nov 3, 2024 •

edited

Loading

codecov bot commented Nov 3, 2024 •

edited

Loading

AMoo-Miki Nov 5, 2024 •

edited

Loading