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.

Sign up for GitHub

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

Jump to bottom

Added UTs for Saved queries new UI #8798

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Added UTs for Saved queries new UI #8798

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e7cd5d4
new savedQuerieTest UTS
riysaxen-amzn Nov 3, 2024
ed9fcae
UTs for newSavedQueries UI
riysaxen-amzn Nov 3, 2024
dbeeb88
UTs for newSavedQueries UI
riysaxen-amzn Nov 3, 2024
52e3375
Merge branch 'main' into savedQueriesUTs
riysaxen-amzn Nov 3, 2024
2e2acd0
Merge branch 'main' into savedQueriesUTs
riysaxen-amzn Nov 4, 2024
272e241
added changelogs file
riysaxen-amzn Nov 4, 2024
d2fd437
fixed the UTs
riysaxen-amzn Nov 4, 2024
4a6b488
Merge branch 'main' into savedQueriesUTs
riysaxen-amzn Nov 4, 2024
2a098fc
addressed the comments on saved queries UTs
riysaxen-amzn Nov 4, 2024
557e0ce
fix eslint
riysaxen-amzn Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/8798.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Added UTs for Saved queries new UI ([#8798](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8798))
4 changes: 2 additions & 2 deletions src/plugins/data/public/autocomplete/autocomplete_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@AMoo-Miki AMoo-Miki Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

this.querySuggestionProviders.set(language, provider);
}
};
Expand All @@ -69,7 +69,7 @@ export class AutocompleteService {

/** @public **/
public setup(core: CoreSetup) {
this.getValueSuggestions = this.autocompleteConfig.valueSuggestions.enabled
this.getValueSuggestions = this.autocompleteConfig?.valueSuggestions.enabled
? setupValueSuggestionProvider(core)
: getEmptyValueSuggestions;

Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/public/field_formats/field_formats_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
private readonly fieldFormatsRegistry: FieldFormatsRegistry = new FieldFormatsRegistry();

public setup(core: CoreSetup) {
core.uiSettings.getUpdate$().subscribe(({ key, newValue }) => {
core.uiSettings?.getUpdate$().subscribe(({ key, newValue }) => {

Check warning on line 41 in src/plugins/data/public/field_formats/field_formats_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/field_formats/field_formats_service.ts#L41 

Added line #L41 was not covered by tests
if (key === UI_SETTINGS.FORMAT_DEFAULT_TYPE_MAP) {
this.fieldFormatsRegistry.parseDefaultTypeMap(newValue);
}
Expand Down
86 changes: 86 additions & 0 deletions src/plugins/data/public/plugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/
import { DataPublicPlugin, DataPublicPluginSetup } from '../public/plugin';
import { DataPublicPluginStart } from './types';
import { coreMock } from '../../../core/public/mocks';
import { UI_SETTINGS } from '../common';
import { of } from 'rxjs';
import { setUseNewSavedQueriesUI } from './services';

jest.mock('./services');

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

Choose a reason for hiding this comment

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

nit. avoid any if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

let mockDataPublicPluginStart: MockedKeys<DataPublicPluginStart>;

const expressionsMock = {
registerFunction: jest.fn(),
registerType: jest.fn(),
};

const uiActionsMock = {
registerAction: jest.fn(),
addTriggerAction: jest.fn(),
};

beforeEach(() => {
const initializerContext = {
config: {
get: jest.fn().mockReturnValue({
savedQueriesNewUI: { enabled: true },
search: {
aggs: {
shardDelay: {
enabled: true,
},
},
},
autocompleteConfig: {
valueSuggestions: {
enabled: true,
},
},
}),
},
};

plugin = new DataPublicPlugin(initializerContext);

coreSetup = {
...coreMock.createSetup({ pluginStartContract: mockDataPublicPluginStart }),
uiSettings: {
get: jest.fn((setting) => {
if (setting === UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED) {
return true;
}
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: return setting === UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

}),
get$: jest.fn((setting) => {
if (setting === UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED) {
return of(true);
}
return of(false);
}),
getUpdate$: jest.fn(() => of({ key: UI_SETTINGS.FORMAT_DEFAULT_TYPE_MAP, newValue: {} })),
},
expressions: expressionsMock,
uiActions: uiActionsMock,
};

coreStart = coreMock.createStart();
});

it('should setup the plugin and set useNewSavedQueriesUI', () => {
const setupContract = plugin.setup(coreSetup, {
expressions: expressionsMock,
uiActions: uiActionsMock,
usageCollection: {},
});

expect(setUseNewSavedQueriesUI).toHaveBeenCalledWith(true);
});
});
2 changes: 1 addition & 1 deletion src/plugins/data/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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]

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

setUseNewSavedQueriesUI(useNewSavedQueriesUI);

return {
Expand Down
Loading
Loading