-
Notifications
You must be signed in to change notification settings - Fork 917
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
Add tests for saved search creation and loading for query enhancement #9112
base: main
Are you sure you want to change the base?
Add tests for saved search creation and loading for query enhancement #9112
Conversation
Signed-off-by: Justin Kim <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9112 +/- ##
==========================================
- Coverage 60.90% 60.90% -0.01%
==========================================
Files 3808 3808
Lines 91196 91196
Branches 14400 14400
==========================================
- Hits 55547 55543 -4
- Misses 32096 32099 +3
- Partials 3553 3554 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Justin Kim <[email protected]>
...ore-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_search.spec.js
Show resolved
Hide resolved
|
||
const workspaceName = uuid(); | ||
// datasource name must be 32 char or less | ||
const datasourceName = uuid().substring(0, 32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here i am randomizing to avoid conflict in parallel tests for the workspace name and datasource
… popover that appears Signed-off-by: Justin Kim <[email protected]>
...opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/dataset_selector.spec.js
Show resolved
Hide resolved
...ore-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_search.spec.js
Outdated
Show resolved
Hide resolved
dataset: INDEX_PATTERN_WITH_TIME, | ||
queryString: 'bytes_transferred > 9950', | ||
hitCount: 28, | ||
sampleTableData: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since we capsuled a lot of test items in, some item like sampleTableData is not quite clear to me. shall we add a comment at the beginning of indexPatternTestConfigurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah can do
...ore-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_search.spec.js
Show resolved
Hide resolved
...ore-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_search.spec.js
Outdated
Show resolved
Hide resolved
|
||
// Sets the dataset for the search. Since INDEXES are not saved to the dataset, | ||
// we need to click through various buttons to manually add them | ||
const setDataset = (dataset) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make mapLanguageToApiResponseString and mapDatasetToType to constants for type safe and single source of truth? For example, instead of running a func mapDatasetToType
we just do
const DatasetTypes = {
INDEX_PATTERN: {
name: 'INDEX_PATTERN',
value: INDEX_PATTERN_WITH_TIME,
supportedLanguages: [QueryLanguages.DQL, QueryLanguages.LUCENE, QueryLanguages.SQL, QueryLanguages.PPL],
},
INDEXES: {
name: 'INDEXES',
value: INDEX_WITH_TIME_1,
supportedLanguages: [QueryLanguages.SQL, QueryLanguages.PPL],
},
};
and
const QueryLanguages = {
DQL: {
name: 'DQL',
apiName: 'kuery', // The mapping is part of the definition
supports: {
filters: true,
histogram: true,
// ...
}
},
// ...
};
and if (language === QueryLanguages.DQL.name)
instead of if (language === 'DQL')
. So adding a new query language only requires updating QueryLanguages and relevant templates and
Adding a new dataset type is only add to DatasetTypes.
Then we can have
TestCaseGenerator: Handles test case creation
TestExecutor: Manages test execution flow
export const generateTestCase = (dataset, language) => {
const baseConfig = {
name: `${dataset.name}-${language.name}`,
dataset: dataset.value,
language: language.name,
queryString: createQueryString(language.name, dataset.value),
...language.supports,
};
return {
...baseConfig,
hitCount: getExpectedHitCount(dataset.name, language.name),
sampleTableData: getExpectedTableData(dataset.name, language.name),
saveName: `${language.name.toLowerCase()}-${dataset.name.toLowerCase()}-01`,
};
};
export const generateAllTestCases = () => {
return Object.values(DatasetTypes).flatMap(dataset =>
dataset.supportedLanguages.map(language =>
generateTestCase(dataset, language)
)
);
then
// Main test suite
generateAllTestCases().forEach(testCase => {
it(`should handle saved search for ${testCase.name}`, async () => {
// Set up the search
await navigateToDiscoverPage(cy, testSetup.workspaceName);
await setDataset(cy, {
...testCase,
datasourceName: testSetup.datasourceName
});
await setSearchConfigurations(cy, testCase);
// Verify the search state
verifyDiscoverPageState(cy, testCase);
// Save and verify the search
await saveSearch(cy, testCase.saveName);
verifySavedSearchInAssetsPage(cy, {
...testCase,
workspaceName: testSetup.workspaceName
});
});
});
I think we can move some functions here to util. I am thinking how to make the tests are readable and type safe and no big duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For:
const DatasetTypes = {
INDEX_PATTERN: {
name: 'INDEX_PATTERN',
value: INDEX_PATTERN_WITH_TIME,
supportedLanguages: [QueryLanguages.DQL, QueryLanguages.LUCENE, QueryLanguages.SQL, QueryLanguages.PPL],
},
INDEXES: {
name: 'INDEXES',
value: INDEX_WITH_TIME_1,
supportedLanguages: [QueryLanguages.SQL, QueryLanguages.PPL],
},
};
Im afraid of restricting the value
to specific INDEX_PATTERN_WQITH_TIME
and INDEX_WITH_TIME_1
. For example, what if another dev wanted to use INDEX_WITHOUT_TIME_1
?
But I completely agree with abstracting things out that can be abstracted, and I will do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented something very similar to what you have described. Could you take a look?
...ore-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_search.spec.js
Outdated
Show resolved
Hide resolved
...ore-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_search.spec.js
Outdated
Show resolved
Hide resolved
|
||
cy.getElementByTestId('docTableHeaderFieldSort_bytes_transferred').click(); | ||
|
||
// TODO: This reload shouldn't need to be here, but currently the sort doesn't always happen right away |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get it. For DQL, sort should happen after waitForLoad. Do you mind adding a video for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't always replicate it, but sometimes:
- The "Selected fields" are not applied. The headers for the selected fields are there, but all the data is dumped into the first column
- The Sort sometimes doesn't happen, even after loading for a while
I'll grab videos of these if i can replicate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a video of the latter case, but its too big to upload here :D . I'll share it with you in private
...ore-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_search.spec.js
Outdated
Show resolved
Hide resolved
...ore-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_search.spec.js
Outdated
Show resolved
Hide resolved
Signed-off-by: Justin Kim <[email protected]>
Description
Adds cypress tests for query enhanced env. Creates a saved search and loads it.
Issues Resolved
closes #8949
closes #8948
Changelog
Check List
yarn test:jest
yarn test:jest_integration