Improve the test utils for Explore/Enhancements #11138
Conversation
📝 WalkthroughWalkthroughRefactors many Cypress tests to provision workspaces and datasets via API helpers. Adds endpoint-driven helpers (createWorkspaceWithDatasource, createDatasetWithEndpoint, createWorkspaceAndDatasetUsingEndpoint), randomized dataset/workspace IDs, new osd commands, constants for fixtures, and page-state/reset utilities; updates setup/cleanup and navigation across tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Cypress Test
participant OSDA as cy.osd Commands
participant Helper as utils/helpers
participant API as Backend API
participant Store as Data Store
Test->>OSDA: cy.osd.setupEnvAndGetDataSource(DATASOURCE_NAME)
OSDA->>API: POST /api/datasources (create or ensure)
API-->>OSDA: datasourceId
OSDA-->>Test: alias datasourceId
Test->>Helper: createWorkspaceAndDatasetUsingEndpoint(datasource, workspace, datasetId, indexPattern,...)
Helper->>API: POST /api/workspaces (with datasourceId, features)
API-->>Helper: workspaceId
Helper->>API: POST /api/datasets (workspaceId, datasetId, indexPattern, fields...)
API-->>Helper: datasetCreated
Helper-->>Test: aliases (workspaceId, datasetId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
LDrago27
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
ashwin-pc,
bandinib-amzn,
curq,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ruanyl,
sejli,
virajsanghvi,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
LDrago27
requested review from
TackAdam,
d-buckner,
paulstn,
ps48,
raintygao,
ruchidh and
sumukhswamy
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/dataset_selector.spec.js (1)
53-58: Remove unused indices parameter from cleanup call.The
cleanupWorkspaceAndDataSourceAndIndicesfunction only accepts aworkspaceNameparameter and does not use the indices argument. The indices array being passed here is unnecessary and should be removed to match the pattern used elsewhere (e.g., autocomplete_ui.spec.js line 57, sidebar.spec.js line 240).Function implementation (cypress/utils/commands.osd.js)
cy.osd.add('cleanupWorkspaceAndDataSourceAndIndices', (workspaceName) => { cy.osd.deleteWorkspaceByNameUsingEndpoint(workspaceName); });Update to:
cy.osd.cleanupWorkspaceAndDataSourceAndIndices(workspaceName);cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/05/autocomplete_query.spec.js (1)
43-50: Stale comment: "mock AI mode disablement" without corresponding mock.Line 44 has a comment indicating AI mode disablement should be mocked, but there's no
cy.interceptor similar mock implementation. Compare withai_editor.spec.jslines 48-61 which properly mocks AI mode. Either implement the mock or remove the misleading comment.🔧 Options to fix
Option 1: Remove the stale comment if no mock is needed:
beforeEach(() => { - // mock AI mode disablement cy.osd.navigateToWorkSpaceSpecificPage({Option 2: Add the actual mock if needed:
beforeEach(() => { - // mock AI mode disablement + // mock AI mode disablement + cy.intercept('GET', '**/enhancements/assist/languages*', { + statusCode: 200, + body: { + configuredLanguages: [], + }, + }); cy.osd.navigateToWorkSpaceSpecificPage({cypress/utils/apps/query_enhancements/commands.js (1)
218-229: Parameter removal breaks existing callers; must handle the datasetEnabled parameter.The removal of the
datasetEnabledparameter breaks at least one existing caller incypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.js:65, which explicitly passestrueas the fourth argument. Either preserve the parameter with a default value or update all callers to remove the fourth argument.The addition of error handling for unknown types is good defensive programming.
🤖 Fix all issues with AI agents
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.js:
- Line 65: Replace the direct command call cy.setDataset with the
explore-namespaced helper cy.explore.setDataset to match other tests; locate the
invocation of cy.setDataset(config.dataset, DATASOURCE_NAME, config.datasetType,
true) and change it to cy.explore.setDataset(config.dataset, DATASOURCE_NAME,
config.datasetType, true) so the test uses the same explore command namespace as
field_display_filtering.spec.js, table.spec.js, and inspect.spec.js.
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js:
- Line 52: Replace the ad-hoc `${INDEX_WITH_TIME_1}*` usage with the predefined
INDEX_PATTERN_WITH_TIME_1 constant and make the wildcard behavior consistent:
either use the corresponding INDEX_PATTERN_WITH_NO_TIME_1 (or its wildcard
variant) for the non-time dataset as well, or add a clear inline comment next to
INDEX_PATTERN_WITH_TIME_1 and INDEX_PATTERN_WITH_NO_TIME_1 explaining why one
intentionally uses a wildcard and the other does not; update references to the
unique symbols INDEX_WITH_TIME_1, INDEX_PATTERN_WITH_TIME_1, and
INDEX_PATTERN_WITH_NO_TIME_1 accordingly.
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_breakdown.spec.js:
- Around line 26-28: Fix the minor typo in the comment inside the
runHistogramBreakdownTests block: change "THe" to "The" in the comment above
describe('histogram breakdown'...) so the comment reads "The tests have some
issues in Neo, They are currently skipped Need to revisit them again." This only
requires updating the comment string in the runHistogramBreakdownTests function.
- Around line 83-84: Remove the orphaned standalone comment marker "//" found
immediately above the test declaration it(`check breakdown persistence with
interval changes for ${config.testName}`, ...) in histogram_breakdown.spec.js;
either delete that empty comment line or replace it with a meaningful comment
relevant to the test, ensuring no leftover empty comment remains above the
it(...) block.
In @cypress/utils/apps/query_enhancements/shared.js:
- Around line 242-247: The cy.log message in the resetPageState function
contains a typo ("clikcing"); update the log string in resetPageState to
"Resetting the Page by clicking on New Button" (or equivalent correct spelling)
so the message reads correctly when the function is called.
In @cypress/utils/commands.osd.js:
- Around line 877-940: There are duplicate Cypress custom command definitions
for createDatasetByEndpoint, createWorkspaceWithDataSourceId, and
setExplorePage; remove the redundant definitions so each command is defined only
once (keep the intended canonical implementation and delete the duplicate
blocks), ensuring you preserve any unique behaviors such as aliasing
(cy.wrap(...).as(aliasName)) and the request/expect logic in the remaining
createDatasetByEndpoint and that createWorkspaceWithDataSourceId and
setExplorePage retain their original parameters and side effects.
- Around line 855-868: The DELETE request is using the wrong saved_object type
path; replace the hardcoded `/api/saved_objects/search/${savedQuery.id}` call
with the query-type path so it matches the earlier search that used `type:
'query'`. Update the request in the block that references `savedQuery` (and
mirrors the behavior of the `deleteSavedObject` helper) to call
`/api/saved_objects/query/${savedQuery.id}` and keep the same headers and
failOnStatusCode handling.
In @cypress/utils/constants.js:
- Line 26: Fix the typo in the file comment that reads "// Avialble fixture and
their mappings" by changing "Avialble" to "Available" so the comment reads "//
Available fixture and their mappings"; update the exact comment text in
cypress/utils/constants.js (the line containing the existing comment) without
altering any surrounding code.
🧹 Nitpick comments (18)
cypress/utils/apps/query_enhancements/autocomplete.js (1)
372-376: Consider extracting the PPL editor cleanup logic to reduce duplication.The same PPL pre-clean logic appears in both
createOtherQueryUsingAutocomplete(lines 170-173) andcreateQuery(lines 373-376). Extracting this to a shared helper function would improve maintainability.♻️ Proposed refactor to eliminate duplication
Add a helper function at the top of the file with other utilities:
+/** + * Clears pre-populated template for PPL language + * @param {string} language - Query language name + */ +const clearPPLTemplate = (language) => { + if (language === QueryLanguages.PPL.name) { + cy.get('.inputarea').clear({ force: true }); + cy.wait(200); // Allow editor to process the clear + } +}; + /** * Gets the dataset name based on the dataset typeThen replace both occurrences:
In
createOtherQueryUsingAutocomplete(around line 169):.within(() => { // Handle initial language-specific setup - // For PPL, clear any pre-populated template first - if (config.language === QueryLanguages.PPL.name) { - cy.get('.inputarea').clear({ force: true }); - cy.wait(200); // Allow editor to process the clear - } + clearPPLTemplate(config.language);In
createQuery(around line 372):.within(() => { - // For PPL, clear any pre-populated template first - if (config.language === QueryLanguages.PPL.name) { - cy.get('.inputarea').clear({ force: true }); - cy.wait(200); // Allow editor to process the clear - } + clearPPLTemplate(config.language);cypress/utils/apps/explore/shared.js (1)
244-249: Consider extracting the hardcoded wait duration to a constant.The 2000ms wait is a magic number. Consider using an existing constant like
WAIT_MSor defining a named constant for clarity and easier maintenance.♻️ Suggested refactor
+import { WAIT_MS } from '../../constants'; + export const resetPageState = () => { // Resets the Page state by clicking the New button cy.log('Resetting the Page by clicking on New Button'); cy.getElementByTestId('discoverNewButton').click(); - cy.wait(2000); + cy.wait(WAIT_MS); };cypress/utils/constants.js (1)
69-82: Consider storing field definitions as separate JSON fixture files or structured objects.These JSON strings are very long and difficult to read, review, or maintain. Consider either:
- Storing them in separate
.jsonfixture files and importing them- Defining them as JavaScript objects and using
JSON.stringify()when neededThis would also make it easier to spot differences between the time-based and non-time-based field sets.
cypress/utils/commands.explore.js (1)
365-366: Multiple hardcoded waits may indicate timing issues.The PR adds several 3000ms waits (lines 365, 411, 433, 507, 743). While these may reduce flakiness in the short term, they:
- Slow down test execution
- May still be insufficient on slower environments
- Could mask underlying race conditions
Consider using Cypress's built-in retry mechanisms or waiting for specific conditions (API responses, element states) where possible.
cypress/utils/commands.osd.js (1)
1119-1137: Consider consistent naming convention.The command
deleteWorkSpaceuses a different naming pattern than the existingdeleteWorkspaceByNameUsingEndpoint. Consider using consistent naming likedeleteWorkspaceByAliasto clarify that this version uses an alias rather than an endpoint lookup.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/shared_links.spec.js (1)
95-96: Consider replacing hardcoded wait with a more deterministic assertion.The
cy.wait(2000)is a fixed delay that may be unnecessarily slow in fast environments or insufficient in slow ones. Consider waiting for a specific element or network request to complete instead.However, given this PR's objective is to reduce flakiness (particularly by removing
cy.reload()), and the AI summary indicates this wait accommodates asynchronous URL/state propagation, this may be an acceptable trade-off for now.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/saved_search_in_dashboards.spec.js (1)
56-59: Consider reducing or replacing the 10-second hardcoded wait.The
cy.wait(10000)on line 58 is a significant delay that could slow down test execution. If possible, consider:
- Waiting for a specific element or state change
- Using a shorter wait if 10 seconds is not always necessary
- Adding a comment explaining why such a long wait is required
If this wait is necessary for query editor readiness after clearing, documenting the reason would help future maintainers.
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/time_range_selection.spec.js (1)
50-55: Cleanup retains explicit index array - inconsistent with other files.This file explicitly passes
[INDEX_WITH_TIME_1, INDEX_WITH_TIME_2]to cleanup, whilerecent_queries.spec.jsomits the indices array. Consider standardizing the cleanup approach across all test files for maintainability.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.js (2)
10-11: Different import source for shared utilities.This file imports
getRandomizedDatasetIdfromquery_enhancements/sharedwhile other files (e.g.,recent_queries.spec.js,inspect.spec.js) import fromexplore/shared. Both modules export identical functions per the relevant code snippets. Consider consolidating to a single source for consistency.
43-46: Cleanup retains explicit index array - inconsistent with simplified pattern in other files.This file passes
[INDEX_WITH_TIME_1]to cleanup while several other files in this PR use simplified cleanup without explicit indices. Consider standardizing the approach.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_explore.spec.js (1)
27-50: Consider reducing or replacing long hardcoded waits.The
cy.wait(10000)on line 46 is a significant delay that could slow down test execution. While this may be necessary for data propagation, consider replacing it with a more deterministic wait strategy if possible (e.g., waiting for a specific element or API response).The endpoint-based setup via
createWorkspaceAndDatasetUsingEndpointis a good improvement over UI-based setup.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/queries.spec.js (1)
69-84: Clarify the purpose of the single-space empty query.The
emptyQuery = ' 'approach with a single space character is used to trigger the query editor, but theverifyDiscoverPageStateexpectsqueryString: ''(empty string). This works because the space gets trimmed or treated as empty, but it's not immediately obvious.Consider adding a brief comment explaining why a space is used rather than an empty string.
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.js (1)
117-154: Potential workspace name collision between describe blocks.Both
describeblocks (simple dataset selector selecting an index patternandfiltering index pattern in simple dataset selector) use the sameworkspaceNameconstant. While this works because the first block'safter()hook cleans up before the second block'sbefore()hook runs, this pattern is fragile:
- If test execution order changes
- If tests run in parallel
- If the first cleanup fails
Consider using distinct workspace names for each describe block to ensure complete isolation:
♻️ Suggested approach
const workspaceName = getRandomizedWorkspaceName(); +const filteringWorkspaceName = getRandomizedWorkspaceName(); const timebasedDatasetId = getRandomizedDatasetId(); const nontimebasedDatasetId = getRandomizedDatasetId();Then use
filteringWorkspaceNamein the second describe block.cypress/utils/apps/query_enhancements/commands.js (3)
27-35: Simplify the clearing logic to use a single method.The comment states "Use simpler clearing methods" but the code uses three different clearing methods sequentially (
.clear(),{selectAll}{del}, and{ctrl+a}{backspace}). This redundancy adds unnecessary complexity and 300ms of wait time.Consider testing which single method is most reliable and use only that one, or document why all three are necessary.
♻️ Simplified approach (if one method is sufficient)
- // Use simpler clearing methods to avoid trigger timeouts + // Clear editor content return cy .get('.inputarea') - .clear({ force: true }) - .wait(100) .type('{selectAll}{del}', { force: true }) - .wait(100) - .type('{ctrl+a}{backspace}', { force: true }) - .wait(100); + .wait(100);
100-134: Consider simplifying the verification and clearing logic.The added verification and conditional clearing logic (lines 105-119) significantly increases complexity, with nested checks and multiple hardcoded waits (300ms, 100ms, 200ms). While the defensive approach may help with flakiness, this complexity could make the function harder to maintain.
Consider whether:
- The root cause of residual content could be addressed more directly (e.g., ensuring
clearQueryEditoris fully effective)- The multiple
wait()calls could be replaced with proper assertions that wait for conditionsThe logging additions (lines 131-133) are helpful for debugging.
178-183: Replace hardcoded wait with a condition-based wait.The
cy.wait(200)on line 178 uses an arbitrary delay that could cause flakiness if the UI needs more time, or slow down tests unnecessarily if it completes faster. Consider using a proper assertion or wait condition instead.♻️ Suggested improvement
- cy.wait(200); cy.getElementByTestId('dataset-index-selector') + .should('be.visible') .find('[data-test-subj="comboBoxInput"]')cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js (2)
82-86: Consider adding synchronization after second dataset creation.While the first dataset creation includes an internal
cy.wait(2000)(visible in the helper function), there's no explicit wait after the second dataset creation at line 63. Consider adding a brief wait to ensure the dataset is fully ready before tests begin, reducing potential race conditions.⏱️ Add synchronization after second dataset
createDatasetWithEndpoint(DATASOURCE_NAME, workspaceName, datasetWithoutTimeId, { title: INDEX_WITHOUT_TIME_1, signalType: 'logs', fields: nonTimeBasedFieldsForDatasetCreation, }); + cy.wait(2000); });
111-111: Fix typo in comment."exapnded" should be "expanded".
📝 Fix typo
- // checking the number of exapnded documents visible on screen + // checking the number of expanded documents visible on screen
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
changelogs/fragments/11138.ymlcypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/a_check.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/ai_editor.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/queries.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_explore.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/dataset_selector.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/language_specific_display.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/saved_queries.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/saved_search_in_dashboards.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/time_range_selection.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/caching.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/download_csv.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/field_display_filtering.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/field_stats.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/inspect.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/shared_links.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_breakdown.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_interaction.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/results_summary.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/05/autocomplete_query.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/05/autocomplete_ui.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/05/recent_queries.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/05/sidebar.spec.jscypress/utils/apps/explore/autocomplete.jscypress/utils/apps/explore/saved_queries.jscypress/utils/apps/explore/shared.jscypress/utils/apps/query_enhancements/autocomplete.jscypress/utils/apps/query_enhancements/commands.jscypress/utils/apps/query_enhancements/shared.jscypress/utils/apps/workspace/commands.jscypress/utils/commands.explore.jscypress/utils/commands.osd.jscypress/utils/constants.jscypress/utils/helpers.js
💤 Files with no reviewable changes (1)
- cypress/utils/apps/explore/autocomplete.js
🧰 Additional context used
🧬 Code graph analysis (27)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js (3)
cypress/utils/apps/explore/shared.js (2)
getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (4)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)createDatasetWithEndpoint(78-98)createDatasetWithEndpoint(78-98)cypress/utils/constants.js (2)
nonTimeBasedFieldsForDatasetCreation(73-74)nonTimeBasedFieldsForDatasetCreation(73-74)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_breakdown.spec.js (3)
cypress/utils/apps/explore/shared.js (6)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)INDEX_PATTERN_WITH_TIME(76-80)INDEX_PATTERN_WITH_TIME(113-116)cypress/utils/apps/explore/constants.js (4)
DATASOURCE_NAME(6-6)DATASOURCE_NAME(6-6)INDEX_PATTERN_WITH_TIME(32-32)INDEX_PATTERN_WITH_TIME(32-32)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/utils/apps/query_enhancements/shared.js (1)
cypress/utils/apps/explore/shared.js (5)
getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)getRandomString(20-20)resetPageState(244-249)resetPageState(244-249)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/results_summary.spec.js (2)
cypress/utils/apps/explore/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/utils/apps/explore/saved_queries.js (2)
cypress/utils/apps/explore/shared.js (2)
resetPageState(244-249)resetPageState(244-249)cypress/utils/apps/query_enhancements/shared.js (2)
resetPageState(242-247)resetPageState(242-247)
cypress/utils/helpers.js (1)
cypress/utils/commands.osd.js (3)
workspaceId(273-273)workspaceId(293-293)workspaceId(819-819)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/dataset_selector.spec.js (2)
cypress/utils/apps/explore/shared.js (2)
getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_interaction.spec.js (2)
cypress/utils/apps/explore/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/05/recent_queries.spec.js (2)
cypress/utils/apps/explore/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/inspect.spec.js (3)
cypress/utils/apps/explore/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/apps/query_enhancements/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(33-33)getRandomizedDatasetId(33-33)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/utils/commands.explore.js (2)
cypress/utils/apps/explore/constants.js (2)
DATASOURCE_NAME(6-6)DATASOURCE_NAME(6-6)cypress/utils/apps/explore/shared.js (2)
setDatePickerDatesAndSearchIfRelevant(133-139)setDatePickerDatesAndSearchIfRelevant(133-139)
cypress/utils/apps/workspace/commands.js (1)
cypress/utils/commands.osd.js (1)
endpoint(1123-1123)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/caching.spec.js (2)
cypress/utils/apps/explore/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/utils/apps/query_enhancements/commands.js (2)
src/plugins/workspace/public/components/workspace_form/constants.ts (1)
typeOptions(177-190)cypress/utils/commands.osd.js (2)
dataset(802-802)dataset(1144-1144)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/field_stats.spec.js (2)
cypress/utils/apps/explore/shared.js (2)
getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/05/autocomplete_ui.spec.js (2)
cypress/utils/apps/explore/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/saved_queries.spec.js (2)
cypress/utils/apps/explore/shared.js (6)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)resetPageState(244-249)resetPageState(244-249)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/ai_editor.spec.js (2)
cypress/utils/apps/explore/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/utils/apps/query_enhancements/autocomplete.js (1)
cypress/utils/apps/query_enhancements/constants.js (2)
QueryLanguages(79-137)QueryLanguages(79-137)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/download_csv.spec.js (3)
cypress/utils/apps/explore/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (4)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)createDatasetWithEndpoint(78-98)createDatasetWithEndpoint(78-98)cypress/utils/constants.js (2)
nonTimeBasedFieldsForDatasetCreation(73-74)nonTimeBasedFieldsForDatasetCreation(73-74)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/05/autocomplete_query.spec.js (2)
cypress/utils/apps/explore/shared.js (2)
getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/shared_links.spec.js (2)
cypress/utils/apps/explore/shared.js (2)
getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.js (2)
cypress/utils/apps/query_enhancements/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(33-33)getRandomizedDatasetId(33-33)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.js (3)
cypress/utils/apps/explore/shared.js (2)
getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)cypress/utils/constants.js (2)
nonTimeBasedFieldsForDatasetCreation(73-74)nonTimeBasedFieldsForDatasetCreation(73-74)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/05/sidebar.spec.js (2)
cypress/utils/apps/explore/shared.js (2)
getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/queries.spec.js (3)
cypress/utils/apps/explore/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(35-35)getRandomizedDatasetId(35-35)cypress/utils/apps/query_enhancements/constants.js (4)
DATASOURCE_NAME(6-6)DATASOURCE_NAME(6-6)INDEX_WITH_TIME_1(28-28)INDEX_WITH_TIME_1(28-28)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
cypress/utils/commands.osd.js (2)
cypress/utils/constants.js (6)
PATHS(12-21)PATHS(12-21)defaultFieldsForDatasetCreation(69-70)defaultFieldsForDatasetCreation(69-70)AVAILABLE_FIXTURE_INDICES(27-66)AVAILABLE_FIXTURE_INDICES(27-66)cypress/utils/apps/query_enhancements/constants.js (2)
DATASOURCE_NAME(6-6)DATASOURCE_NAME(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (67)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Lint and validate
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: bundle-analyzer
- GitHub Check: lighthouse
| }); | ||
|
|
||
| cy.setDataset(config.dataset, DATASOURCE_NAME, config.datasetType); | ||
| cy.setDataset(config.dataset, DATASOURCE_NAME, config.datasetType, true); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for setDataset command definitions
echo "=== Searching for setDataset command definitions ==="
rg "setDataset" --type js -B2 -A5 | head -150
echo -e "\n=== Searching for Cypress.Commands.add patterns ==="
rg "Cypress\.Commands\.add.*setDataset" --type js -B1 -A8
echo -e "\n=== Searching for explore namespace commands ==="
rg "Cypress\.Commands\.add.*explore" --type js -B1 -A10Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 17196
🏁 Script executed:
#!/bin/bash
# Check the specific file mentioned in the review
echo "=== Checking saved_search.spec.js ==="
cat -n cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.js | sed -n '60,70p'
echo -e "\n=== Checking imports in saved_search.spec.js ==="
head -30 cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.js
echo -e "\n=== Check if file uses cy.explore or cy.setDataset ==="
grep -n "cy\.setDataset\|cy\.explore\.setDataset" cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.jsRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 1760
Update to use cy.explore.setDataset for consistency with other explore test files.
Line 65 uses cy.setDataset (from query_enhancements), but other explore test files (field_display_filtering.spec.js, table.spec.js, inspect.spec.js, etc.) consistently use cy.explore.setDataset. This file should be updated to use the explore-namespaced command for consistency.
🤖 Prompt for AI Agents
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.js
at line 65, Replace the direct command call cy.setDataset with the
explore-namespaced helper cy.explore.setDataset to match other tests; locate the
invocation of cy.setDataset(config.dataset, DATASOURCE_NAME, config.datasetType,
true) and change it to cy.explore.setDataset(config.dataset, DATASOURCE_NAME,
config.datasetType, true) so the test uses the same explore command namespace as
field_display_filtering.spec.js, table.spec.js, and inspect.spec.js.
| DATASOURCE_NAME, | ||
| workspaceName, | ||
| datasetWithTimeId, | ||
| `${INDEX_WITH_TIME_1}*`, // Uses index pattern with time |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the index constant definitions and their expected patterns
echo "=== Checking constant definitions ==="
rg -n -A 2 -B 2 'INDEX_WITH_TIME_1|INDEX_WITHOUT_TIME_1|INDEX_PATTERN_WITH_TIME_1' cypress/utils/constants.js
echo ""
echo "=== Checking usage patterns in tests ==="
rg -n -C 3 'INDEX_WITH_TIME_1\*|INDEX_WITHOUT_TIME_1' cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.jsRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 872
🏁 Script executed:
#!/bin/bash
# Find the constants file
echo "=== Finding constants file ==="
find . -name 'constants.js' | grep -E '(cypress|utils)'
echo ""
echo "=== View test file around lines 40-70 ==="
cat -n cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js | sed -n '40,70p'
echo ""
echo "=== Check if INDEX_WITH_TIME_1 is defined in test file ==="
rg -n 'INDEX_WITH_TIME_1|INDEX_PATTERN_WITH_TIME_1' cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js | head -20Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1950
🏁 Script executed:
#!/bin/bash
# View the constants file
echo "=== Checking cypress/utils/constants.js ==="
cat -n cypress/utils/constants.js | grep -A 2 -B 2 'INDEX_WITH_TIME_1\|INDEX_WITHOUT_TIME_1\|INDEX_PATTERN_WITH_TIME_1'Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 127
🏁 Script executed:
#!/bin/bash
# View the constants file to see all content
echo "=== Full constants file ==="
cat -n cypress/utils/constants.js | head -100
echo ""
echo "=== Search for INDEX definitions anywhere ==="
rg -n 'INDEX_WITH_TIME_1\s*=' cypress/Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 44528
🏁 Script executed:
#!/bin/bash
# Find INDEX_WITHOUT_TIME_1 definition
echo "=== Finding INDEX_WITHOUT_TIME_1 definition ==="
rg -n 'INDEX_WITHOUT_TIME_1\s*=' cypress/
echo ""
echo "=== Check the explore constants file ==="
cat -n cypress/utils/apps/explore/constants.js | head -50Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 2712
🏁 Script executed:
#!/bin/bash
# Check if INDEX_PATTERN_WITH_TIME_1 is used elsewhere and see the full context of how these are used
echo "=== Check usage of INDEX_PATTERN_WITH_TIME_1 in the test file ==="
rg -n 'INDEX_PATTERN_WITH_TIME' cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js
echo ""
echo "=== Check the function signature to understand what parameter is expected ==="
rg -B 5 -A 15 'createWorkspaceAndDatasetUsingEndpoint' cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js | head -30Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1245
🏁 Script executed:
#!/bin/bash
# Find the helper function definition
echo "=== Find createWorkspaceAndDatasetUsingEndpoint definition ==="
rg -B 5 -A 20 'createWorkspaceAndDatasetUsingEndpoint\s*=' cypress/utils/helpers.js
echo ""
echo "=== Or search in entire helpers directory ==="
find cypress/utils -name '*.js' -type f | xargs rg -l 'createWorkspaceAndDatasetUsingEndpoint'Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1123
🏁 Script executed:
#!/bin/bash
# Let me check the full implementation of createWorkspaceAndDatasetUsingEndpoint to see how it uses the indexPattern parameter
echo "=== Full implementation of createWorkspaceAndDatasetUsingEndpoint ==="
rg -A 50 'export const createWorkspaceAndDatasetUsingEndpoint' cypress/utils/helpers.js | head -70Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1061
🏁 Script executed:
#!/bin/bash
# Check other test files to see if they use similar patterns
echo "=== Check other explore test files for similar pattern usage ==="
find cypress/integration/core_opensearch_dashboards -name '*.spec.js' -type f | xargs rg -l 'INDEX_WITH_TIME_1|createWorkspaceAndDatasetUsingEndpoint' | head -5
echo ""
echo "=== Check how similar datasets are created in other tests ==="
rg -B 3 -A 3 'createDatasetWithEndpoint.*INDEX' cypress/integration/core_opensearch_dashboards --type js | head -30Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 809
🏁 Script executed:
#!/bin/bash
# Check the createDatasetWithEndpoint helper to understand the pattern
echo "=== Find createDatasetWithEndpoint definition ==="
rg -A 30 'export const createDatasetWithEndpoint' cypress/utils/helpers.js
echo ""
echo "=== Check how line 60 and the cleanup at line 83 use INDEX_WITHOUT_TIME_1 ==="
cat -n cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js | sed -n '58,65p'
echo ""
echo "=== Compare with INDEX_PATTERN constants available ==="
cat -n cypress/utils/apps/explore/constants.js | sed -n '29,36p'Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 2339
🏁 Script executed:
#!/bin/bash
# Let's check if there's any setup or teardown that creates these indices
echo "=== Check for index creation in fixture setup ==="
rg -n 'data_logs_small_time_1|data_logs_small_no_time_1' cypress/ --type js | grep -E '(before|setup|fixture|create)' | head -20
echo ""
echo "=== Final confirmation - show the two lines side by side with their context ==="
echo "LINE 52 (time-based dataset):"
sed -n '48,56p' cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js
echo ""
echo "LINE 60 (non-time-based dataset):"
sed -n '58,63p' cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.jsRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 1387
Use the predefined index pattern constant on line 52 and clarify the wildcard usage difference.
Line 52 manually constructs ${INDEX_WITH_TIME_1}* instead of using the existing INDEX_PATTERN_WITH_TIME_1 constant. More importantly, line 52 applies a wildcard pattern for the time-based dataset while line 60 does not apply a wildcard for the non-time-based dataset, despite INDEX_PATTERN_WITH_NO_TIME_1 being defined. Either both should use wildcard patterns or the deliberate difference should be documented with an explanatory comment—currently it creates an inconsistent setup that may cause confusion.
🤖 Prompt for AI Agents
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js
at line 52, Replace the ad-hoc `${INDEX_WITH_TIME_1}*` usage with the predefined
INDEX_PATTERN_WITH_TIME_1 constant and make the wildcard behavior consistent:
either use the corresponding INDEX_PATTERN_WITH_NO_TIME_1 (or its wildcard
variant) for the non-time dataset as well, or add a clear inline comment next to
INDEX_PATTERN_WITH_TIME_1 and INDEX_PATTERN_WITH_NO_TIME_1 explaining why one
intentionally uses a wildcard and the other does not; update references to the
unique symbols INDEX_WITH_TIME_1, INDEX_PATTERN_WITH_TIME_1, and
INDEX_PATTERN_WITH_NO_TIME_1 accordingly.
| const runHistogramBreakdownTests = () => { | ||
| // THe tests have some issues in Neo, They are currently skipped Need to revisit them again. | ||
| describe('histogram breakdown', () => { |
There was a problem hiding this comment.
Minor typo in comment.
"THe" should be "The" on line 27.
📝 Proposed fix
const runHistogramBreakdownTests = () => {
- // THe tests have some issues in Neo, They are currently skipped Need to revisit them again.
+ // The tests have some issues in Neo. They are currently skipped. Need to revisit them again.
describe('histogram breakdown', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const runHistogramBreakdownTests = () => { | |
| // THe tests have some issues in Neo, They are currently skipped Need to revisit them again. | |
| describe('histogram breakdown', () => { | |
| const runHistogramBreakdownTests = () => { | |
| // The tests have some issues in Neo. They are currently skipped. Need to revisit them again. | |
| describe('histogram breakdown', () => { |
🤖 Prompt for AI Agents
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_breakdown.spec.js
around lines 26 - 28, Fix the minor typo in the comment inside the
runHistogramBreakdownTests block: change "THe" to "The" in the comment above
describe('histogram breakdown'...) so the comment reads "The tests have some
issues in Neo, They are currently skipped Need to revisit them again." This only
requires updating the comment string in the runHistogramBreakdownTests function.
| // | ||
| it(`check breakdown persistence with interval changes for ${config.testName}`, () => { |
There was a problem hiding this comment.
Orphan comment appears incomplete.
Line 83 has just // with no content. This looks like an incomplete or leftover comment that should either be removed or completed.
📝 Proposed fix
- //
it(`check breakdown persistence with interval changes for ${config.testName}`, () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // | |
| it(`check breakdown persistence with interval changes for ${config.testName}`, () => { | |
| it(`check breakdown persistence with interval changes for ${config.testName}`, () => { |
🤖 Prompt for AI Agents
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_breakdown.spec.js
around lines 83 - 84, Remove the orphaned standalone comment marker "//" found
immediately above the test declaration it(`check breakdown persistence with
interval changes for ${config.testName}`, ...) in histogram_breakdown.spec.js;
either delete that empty comment line or replace it with a meaningful comment
relevant to the test, ensuring no leftover empty comment remains above the
it(...) block.
| export const resetPageState = () => { | ||
| // Resets the Page state by clicking the New button | ||
| cy.log('Resetting the Page by clikcing on New Button'); | ||
| cy.getElementByTestId('discoverNewButton').click(); | ||
| cy.wait(2000); | ||
| }; |
There was a problem hiding this comment.
Typo in log message.
There's a typo on line 244: "clikcing" should be "clicking".
📝 Proposed fix
export const resetPageState = () => {
// Resets the Page state by clicking the New button
- cy.log('Resetting the Page by clikcing on New Button');
+ cy.log('Resetting the Page by clicking on New Button');
cy.getElementByTestId('discoverNewButton').click();
cy.wait(2000);
};🤖 Prompt for AI Agents
In @cypress/utils/apps/query_enhancements/shared.js around lines 242 - 247, The
cy.log message in the resetPageState function contains a typo ("clikcing");
update the log string in resetPageState to "Resetting the Page by clicking on
New Button" (or equivalent correct spelling) so the message reads correctly when
the function is called.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11138 +/- ##
=======================================
Coverage 60.30% 60.31%
=======================================
Files 4624 4624
Lines 127750 127750
Branches 21508 21508
=======================================
+ Hits 77044 77056 +12
+ Misses 45214 45204 -10
+ Partials 5492 5490 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Suchit Sahoo <[email protected]>
Signed-off-by: Suchit Sahoo <[email protected]>
Signed-off-by: Suchit Sahoo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/02/saved_search_in_dashboards.spec.js (3)
13-16: Missing import:getRandomizedDatasetIdis not imported.The
createWorkspaceAndDatasetUsingEndpointcall on line 38 usesdatasetId, butgetRandomizedDatasetIdis not imported from the shared utilities. Compare with other files in this PR that correctly import it.🐛 Proposed fix
import { getRandomizedWorkspaceName, setDatePickerDatesAndSearchIfRelevant, + getRandomizedDatasetId, } from '../../../../../../utils/apps/query_enhancements/shared';
26-27: Missing declaration:datasetIdconstant is undefined.The
datasetIdvariable used on line 38 is never declared. This will cause aReferenceErrorat runtime.🐛 Proposed fix
const workspaceName = getRandomizedWorkspaceName(); +const datasetId = getRandomizedDatasetId();
24-24: Missing import:createWorkspaceAndDatasetUsingEndpointis not imported.The function is used on line 35 but not included in the import from
helpers.js.🐛 Proposed fix
-import { prepareTestSuite } from '../../../../../../utils/helpers'; +import { prepareTestSuite, createWorkspaceAndDatasetUsingEndpoint } from '../../../../../../utils/helpers';cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/03/field_display_filtering.spec.js (1)
102-105: Logic error: condition is always true, causing early return.The condition
filterButton !== 'for' || filterButton !== 'out'is alwaystruebecause:
- If
filterButton === 'for':'for' !== 'out'istrue- If
filterButton === 'out':'out' !== 'for'istrue- For any other value: at least one comparison is
trueThis causes the function to always log and return early, never executing the actual filter verification logic. The calls at lines 188-200 will silently fail to verify anything.
🐛 Proposed fix: use AND instead of OR
- if (filterButton !== 'for' || filterButton !== 'out') { + if (filterButton !== 'for' && filterButton !== 'out') { cy.log('Filter button must be for or or.'); return; }
🤖 Fix all issues with AI agents
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/saved_queries.spec.js:
- Around line 120-121: The after hook calls
cy.osd.cleanupWorkspaceAndDataSourceAndIndices without the indices array; update
the call to pass the same indices array used in caching.spec.js (i.e., provide
the indices argument alongside workspaceName) so cleanup removes the test
indices as well; locate the call to
cy.osd.cleanupWorkspaceAndDataSourceAndIndices and add the indices parameter
(the same variable or literal used in caching.spec.js) to the argument list.
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_breakdown.spec.js:
- Line 27: Fix the typo in the comment string "// THe tests have some issues in
Neo, They are currently skipped Need to revisit them again." by changing "THe"
to "The" (so it reads "// The tests have some issues in Neo, They are currently
skipped Need to revisit them again."); update the exact comment in
histogram_breakdown.spec.js to correct capitalization.
- Line 83: Remove the orphaned empty comment (`//`) in the test file
(histogram_breakdown.spec.js) — either delete that line or replace it with a
meaningful comment describing the surrounding test intent; ensure no leftover
blank comment lines remain in the file so the test code is clean.
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/traces.spec.js:
- Around line 23-24: The setup was disabled with before.skip(), leaving tests
and the after() cleanup (cleanupWorkspaceAndDataSourceAndTraces) to run against
non-existent state causing failures (e.g., traceUrl undefined in
traceTestSuite). Replace the current partial skip with a full-suite skip by
converting the outer test suite to describe.skip('traceTestSuite', ...) (or wrap
the entire suite body in describe.skip()) and remove the before.skip() so the
after() hook will not execute cleanup for resources that were never created.
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/06/build_vis.spec.js:
- Line 21: Typo in the test suite title: change the describe string in the test
file (the 'describe' call with title "build visualization manully tests") to use
the correct spelling "manually" so it reads "build visualization manually tests"
(or better "build visualization manually" if you prefer shorter). Update the
string in the describe(...) invocation to replace "manully" with "manually".
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/01/queries.spec.js:
- Around line 52-54: The after-hook omits the indices argument when calling
cleanupWorkspaceAndDataSourceAndIndices, so update the call to pass the indices
array used in setup (e.g., include `${INDEX_WITH_TIME_1}*`) so cleanup removes
the created indices; locate the after() block and modify
cleanupWorkspaceAndDataSourceAndIndices(workspace) to
cleanupWorkspaceAndDataSourceAndIndices(workspace, [ `${INDEX_WITH_TIME_1}*` ]).
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/02/saved_queries.spec.js:
- Around line 131-137: The calls to apiDeleteSavedQueryIfExists in the
beforeEach pass the resolved workspaceId but the helper expects the
workspaceName (so it can call Cypress.env(...) itself); update both calls to
pass the workspaceName identifier (the variable named workspaceName or a
template like `${workspaceName}`) instead of workspaceId and remove the manual
Cypress.env lookup (you can keep the const workspaceId if used elsewhere, but
call apiDeleteSavedQueryIfExists(`${workspaceName}-${config.saveName}`,
workspaceName) and
apiDeleteSavedQueryIfExists(`${workspaceName}-${config.testName}`,
workspaceName)).
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/02/saved_search_in_dashboards.spec.js:
- Around line 31-46: The beforeEach uses a module-scoped workspaceName which is
reused across tests causing potential collisions; change to generate a unique
workspace name per test by computing workspaceName inside beforeEach (or use a
per-test helper) so createWorkspaceAndDatasetUsingEndpoint,
cy.osd.grabDataSourceId and the cleanup in afterEach operate on a unique
identifier each run; update any references to workspaceName in afterEach and
test steps to use the newly scoped name so teardown targets the correct
workspace and prevents races.
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/05/recent_queries.spec.js:
- Around line 35-45: The test setup is missing a call to
cy.osd.setupEnvAndGetDataSource before creating the workspace; add a call to
cy.osd.setupEnvAndGetDataSource(DATASOURCE_NAME) in the before hook immediately
prior to invoking createWorkspaceAndDatasetUsingEndpoint so the data source is
provisioned/returned before workspace/dataset creation, keeping the existing
createWorkspaceAndDatasetUsingEndpoint, DATASOURCE_NAME, workspace, and
datasetId references intact.
In @cypress/utils/apps/query_enhancements/inspect.js:
- Around line 102-131: The JSDoc example in inspect.js has lost its comment
formatting and indentation (the example JSON with fields like "FlightNum" and
the "personal" nested block), so re-add standard JSDoc line prefixes and
indentation for that example: prepend each example line inside the JSDoc block
with " * " and align nested arrays/objects consistently (e.g., ensure the
"personal" nested structure and its inner "address", "coordinates", "email"
entries are indented and prefixed with "*" like the other JSDoc lines) so the
entire example block is restored to proper JSDoc formatting.
In @cypress/utils/commands.osd.js:
- Around line 797-809: The helper command setExplorePage is defined twice
(duplicate definitions at two locations), causing the second definition to
silently override the first; remove the redundant declaration by deleting the
second setExplorePage implementation (the one around lines 1139-1151) so only a
single setExplorePage cy.osd.add(...) remains, ensuring the unique symbol
setExplorePage and its parameter signature (datasourceId, workspaceId,
timeRange, datasetId, datasetOptions, query, endpoint) is preserved exactly
once.
- Around line 707-756: The file contains a duplicate definition of the
cy.osd.add('createDatasetByEndpoint', ...) function which causes the second
definition to override the first; remove the redundant duplicate (the second
occurrence) so only a single createDatasetByEndpoint registration remains,
keeping the existing implementation that includes building attributes, the
conditional timeFieldName, the POST to
/api/saved_objects/index-pattern/:datasourceId::datasetId, response assertions,
cy.log and aliasing the id.
- Around line 765-795: The file contains a duplicated command registration for
createWorkspaceWithDataSourceId (defined at two places); remove the second
duplicate registration so only one cy.osd.add('createWorkspaceWithDataSourceId',
...) implementation remains, ensuring any unique alias/default param usage from
either copy is preserved by keeping the first (or the most complete) definition
and deleting the redundant one.
- Line 802: The dataset template string assigned to the variable dataset is
missing a closing single quote after ${datasetOptions.title}, producing a
malformed string; update the construction of dataset (where datasourceId,
datasetId, datasetOptions.timeField and datasetOptions.title are interpolated)
to add the missing closing single quote after the title value so the
timeFieldName and title fields are properly quoted and the resulting
URL/parameter string is well-formed.
In @cypress/utils/constants.js:
- Line 26: Fix the typo in the comment above the fixture mappings in
cypress/utils/constants.js by changing "Avialble" to "Available" so the comment
reads "Available fixture and their mappings"; update only the comment text to
correct spelling without altering surrounding code or behavior.
🧹 Nitpick comments (35)
cypress/utils/apps/data_explorer/commands.js (1)
187-189: Avoid hard-coded waits; prefer conditional waits and test IDs.Two concerns with this addition:
cy.wait(2000)is brittle — Fixed waits are a common source of test flakiness. Consider waiting for a specific condition instead, such as the toast confirmation appearing/disappearing or the flyout content updating after deletion.CSS class selector is fragile — Using
.euiFlyoutFootercouples the test to implementation details. Prefer a test ID selector if available (e.g.,cy.getElementByTestId('...')).♻️ Suggested refactor
cy.getElementByTestId('confirmModalConfirmButton').click(); - cy.wait(2000); - cy.get('.euiFlyoutFooter').find('button[type="button"]').contains('Cancel').first().click(); + // Wait for deletion toast or modal to close before dismissing the flyout + cy.getElementByTestId('confirmModalConfirmButton').should('not.exist'); + cy.getElementByTestId('euiFlyoutCloseButton').click();If a test ID for the flyout footer cancel button isn't available, consider adding one to the source, or at minimum replace the hard wait with a condition:
- cy.wait(2000); + cy.getElementByTestId('confirmModalConfirmButton').should('not.exist');cypress/utils/apps/query_enhancements/inspect.js (1)
225-228: Consider removing the hardcodedcy.wait(500)delay.Hardcoded waits are generally an anti-pattern in Cypress as they can be flaky (too short on slow CI, unnecessary delay on fast systems). Since you're already using
.should('be.visible')which has built-in retry-ability, the explicit wait may be redundant.If you're addressing a specific timing issue, consider whether increasing the default command timeout or adding a more specific assertion would be more reliable.
♻️ Suggested refactor
- cy.wait(500); - cy.getElementByTestId('inspectorViewChooserRequests') - .should('be.visible') - .click({ waitForAnimations: false }); + cy.getElementByTestId('inspectorViewChooserRequests') + .should('be.visible') + .click({ waitForAnimations: false });If the wait is necessary due to a dropdown animation, consider using
{ timeout: 10000 }on the.should('be.visible')assertion instead, or investigate if there's a more reliable element state to wait for.cypress/utils/commands.js (1)
174-195: LGTM with a minor suggestion for edge case handling.The implementation correctly retrieves advanced settings with proper fallback logic. Consider logging or throwing a warning when the setting key is not found (line 191 returns
undefinedsilently ifsettings[settingKey]doesn't exist), which could help debug test failures.💡 Optional enhancement for debugging
const settings = response.body.settings || response.body; const settingValue = settings[settingKey]?.userValue ?? settings[settingKey]?.value; + if (settings[settingKey] === undefined) { + cy.log(`Warning: Setting key "${settingKey}" not found`); + } cy.log(`Advanced setting ${settingKey}: ${settingValue}`); return cy.wrap(settingValue);cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/06/build_vis.spec.js (1)
69-72: Consider reducing or removing the 10-second wait.The comment says "Prevent backend error," but with endpoint-based setup now in place, this aggressive wait may no longer be necessary. If backend timing is still an issue, consider using a conditional wait or retry mechanism instead of a fixed 10-second delay.
cypress/utils/apps/workspace/commands.js (2)
142-160: Consider defaultingendpointparameter for consistency.The
endpointparameter is required but other similar commands in the codebase (e.g., incommands.osd.js) default it toCypress.config('baseUrl'). Consider adding a default to make the API more forgiving:-Cypress.Commands.add('createWorkspaceWithEndpoint', (endpoint, { settings, ...workspace } = {}) => { +Cypress.Commands.add('createWorkspaceWithEndpoint', (endpoint = Cypress.config('baseUrl'), { settings, ...workspace } = {}) => {
170-184: Handle potential undefinedworkspace.namein error messages.If
workspaceis passed as an empty object or without anameproperty, the error message will display "Create workspace undefined failed", which is unhelpful for debugging.- throw new Error( - `Create workspace ${workspace.name} failed after cleanup: ${retryResp?.body?.error}` - ); + throw new Error( + `Create workspace ${workspace.name || 'unknown'} failed after cleanup: ${retryResp?.body?.error}` + );Similarly for line 183:
- throw new Error(`Create workspace ${workspace.name} failed: ${resp?.body?.error}`); + throw new Error(`Create workspace ${workspace.name || 'unknown'} failed: ${resp?.body?.error}`);cypress/utils/constants.js (1)
68-82: Consider exporting field schemas as objects or separate JSON files.The field schema strings are very long and difficult to read/maintain. Consider either:
- Moving them to separate JSON fixture files and importing them
- Exporting as JavaScript objects directly (and using
JSON.stringify()when needed)This would improve maintainability and allow IDE support for schema validation.
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/07/rule_matching_vis_metric.spec.js (1)
26-34: Consider reusingdatasetNameconstant in endpoint call.Line 30 duplicates the pattern
${INDEX_WITH_TIME_1}*which is already defined asdatasetNameon line 18.♻️ Suggested refactor
createWorkspaceAndDatasetUsingEndpoint( DATASOURCE_NAME, workspaceName, datasetId, - `${INDEX_WITH_TIME_1}*`, + datasetName, 'timestamp', // timestampField 'logs', // signalType ['use-case-observability'] // features );cypress/utils/helpers.js (1)
112-147: Consider using an options object for better readability.The function has 9 parameters, which can be error-prone when calling. Consider refactoring to use an options object pattern for optional parameters:
♻️ Example refactor
export const createWorkspaceAndDatasetUsingEndpoint = ( datasourceName, workspaceName, datasetId, indexPattern, options = {} ) => { const { timestampField = 'timestamp', signalType = 'logs', features = ['use-case-observability'], workspaceAliasName = `${workspaceName}:WORKSPACE_ID`, datasetAliasName = `${datasetId}:DATASET_ID` } = options; // ... rest of implementation };This would make call sites more readable:
createWorkspaceAndDatasetUsingEndpoint( DATASOURCE_NAME, workspaceName, datasetId, indexPattern, { timestampField: 'timestamp', signalType: 'logs' } );cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/06/rule_matching_vis_line.spec.js (1)
16-18: Consider reusing thedatasetNameconstant.
datasetNameis defined on line 18 as${INDEX_WITH_TIME_1}*, but the same pattern is repeated on line 30. Consider using thedatasetNameconstant for consistency.Suggested change
createWorkspaceAndDatasetUsingEndpoint( DATASOURCE_NAME, workspaceName, datasetId, - `${INDEX_WITH_TIME_1}*`, + datasetName, 'timestamp', // timestampField 'logs', // signalType ['use-case-observability'] // features );Also applies to: 30-30
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/07/rule_matching_vis_bar_gauge.spec.js (1)
14-16: Same suggestion as vis_line.spec.js: reusedatasetNameconstant.The
datasetNameconstant defined on line 16 could be reused on line 28 instead of duplicating the pattern.Suggested change
createWorkspaceAndDatasetUsingEndpoint( DATASOURCE_NAME, workspaceName, datasetId, - `${INDEX_WITH_TIME_1}*`, + datasetName, 'timestamp', // timestampField 'logs', // signalType ['use-case-observability'] // features );Also applies to: 28-28
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/04/histogram_interaction.spec.js (1)
55-57: Consider passing the index pattern to cleanup for consistency.Other test files in this PR explicitly pass the index array to cleanup (e.g.,
cleanupWorkspaceAndDataSourceAndIndices(workspaceName, [INDEX_WITH_TIME_1])). This file omits it, which may leave test indices behind if the cleanup function doesn't automatically handle them.Suggested fix
after(() => { - cy.osd.cleanupWorkspaceAndDataSourceAndIndices(workspace); + cy.osd.cleanupWorkspaceAndDataSourceAndIndices(workspace, [INDEX_PATTERN_WITH_TIME]); });cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_interaction.spec.js (1)
51-53: Consider passing the index pattern to cleanup for consistency.Other explore test files (e.g.,
add_vis_to_dashboard.spec.js,rule_matching_vis.spec.js) pass an index array to cleanup. This file omits it, which may leave test indices behind.Suggested fix
after(() => { - cy.osd.cleanupWorkspaceAndDataSourceAndIndices(workspace); + cy.osd.cleanupWorkspaceAndDataSourceAndIndices(workspace, [INDEX_PATTERN_WITH_TIME]); });cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/03/caching.spec.js (1)
59-76: Consider extracting the alternative dataset creation to a helper or ensuring cleanup.The inline dataset creation for the refresh validation test works correctly, but the
alternativeDatasetIddataset is created within a test and may not be explicitly cleaned up. Since the after hook callscleanupWorkspaceAndDataSourceAndIndices, which should clean up workspace-associated datasets, this is likely fine.However, the
cy.wait(2000)is a hardcoded delay that could cause flakiness. If this is necessary for eventual consistency, consider adding a comment explaining why, or use a more resilient polling approach.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/07/rule_matching_vis_table.spec.js (1)
17-18: Minor: Consider reusingdatasetNameconstant.Line 30 uses
${INDEX_WITH_TIME_1}*which is the same as thedatasetNameconstant defined on line 18. Consider reusingdatasetNamefor consistency and to avoid duplication.♻️ Suggested change
createWorkspaceAndDatasetUsingEndpoint( DATASOURCE_NAME, workspaceName, datasetId, - `${INDEX_WITH_TIME_1}*`, + datasetName, 'timestamp', // timestampField 'logs', // signalType ['use-case-observability'] // features );Also applies to: 30-30
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/07/rule_matching_vis_histogram.spec.js (1)
84-99: Consider using deterministic waits instead of hardcoded delays.The
cy.wait(1000)calls after typing bucket count/size are used to wait for canvas updates. While these may be necessary for debounced updates, hardcoded delays can cause flakiness. Consider using a more resilient approach such as waiting for the canvas data URL to change:♻️ Alternative approach using polling
// Instead of: cy.contains('label', 'Bucket Count').click().type('10'); cy.wait(1000); cy.get('canvas.marks').then((canvas) => { ... }); // Consider polling until canvas changes: cy.contains('label', 'Bucket Count').click().type('10'); cy.get('canvas.marks').should((canvas) => { const afterCanvasDataUrl = canvas[0].toDataURL(); expect(afterCanvasDataUrl).not.to.eq(beforeCanvasDataUrl); });Using Cypress's built-in retry mechanism with
.should()is more resilient than fixed delays.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/01/saved_search.spec.js (1)
31-32: Consider adding a TODO comment or tracking issue for the skipped tests.The
describe.skipindicates these tests need utility updates. Consider linking to a tracking issue or adding more context about what specific utilities need updating to help future contributors.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/01/simple_dataset_selector.spec.js (2)
133-172: Potential issue: Second describe block creates a workspace with the same name after first describe's cleanup.The test structure has:
- A top-level
before()(line 37) that creates the workspace- First describe's
after()(line 76) that cleans up the workspace- Second describe's
before()(line 134) that creates a workspace with the sameworkspaceNameThis works due to Cypress execution order, but using the same
workspaceNamevariable for both describe blocks is fragile. If the cleanup in line 77-80 fails or is skipped, the second describe's setup could conflict.Consider using a distinct workspace name for the filtering tests to improve isolation:
+const filteringWorkspaceName = getRandomizedWorkspaceName(); + describe('filtering index pattern in simple dataset selector', () => { before(() => { cy.osd.getDataSourceId(DATASOURCE_NAME); cy.get('@DATASOURCE_ID').then((datasourceId) => { cy.osd.createWorkspaceWithDataSourceId( datasourceId, - workspaceName, + filteringWorkspaceName, ['use-case-search'], - `${workspaceName}:WORKSPACE_ID` + `${filteringWorkspaceName}:WORKSPACE_ID` ); });
146-165: Consider refactoring to avoid eslint-disable comments.The loop creates datasets dynamically but requires multiple
eslint-disablecomments forno-loop-func. While the current implementation works (sincecurrentDatasetIdis scoped per iteration), consider extracting the dataset creation into a helper function to eliminate the lint suppressions:const createDatasetInWorkspace = (datasetIdVal, pattern) => { cy.get('@DATASOURCE_ID').then((datasourceId) => { cy.get(`@${workspaceName}:WORKSPACE_ID`).then((workspaceId) => { cy.osd.createDatasetByEndpoint(/* ... */); }); }); }; for (let i = 1; i <= noIndexPatterns; i++) { createDatasetInWorkspace(getRandomizedDatasetId(), `${INDEX_PATTERN_WITH_TIME.slice(0, i)}*`); }cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/05/sidebar.spec.js (1)
157-159: Consider replacing fixed waits with assertions.The
cy.wait(200)aftercy.clearQueryEditor()introduces a fixed delay that can cause flakiness. If there's an observable state change (e.g., editor becoming empty), prefer waiting for that condition instead.Also applies to: 169-170
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/04/advanced_settings.spec.js (1)
117-143: Remove commented-out debug code.Lines 119 and 139 contain commented-out
cy.wait()calls that appear to be debug artifacts. These should be removed to keep the codebase clean.🧹 Proposed cleanup
// Wait for initial data loading and verify elements are present cy.osd.waitForLoader(true); - //cy.wait(60000); // DIAGNOSTIC: Verify settings are still correct before proceeding// Wait for page reload and data loading cy.osd.waitForLoader(true); - //cy.wait(90000); // DIAGNOSTIC: Verify reduced sampleSize setting is correct after reloadcypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/03/shared_links.spec.js (2)
181-187: Consider reducing or explaining the 6-second wait.A 6-second hardcoded wait (
cy.wait(6000)) is quite long and may indicate an underlying timing issue. Consider:
- Using a conditional wait (e.g., waiting for a specific element state)
- Adding a comment explaining why this duration is necessary
200-210: Consider replacing hardcoded wait with attribute polling.Instead of a fixed 2-second wait for short URL generation, consider polling for the attribute change:
♻️ Suggested improvement
// Test short url cy.getElementByTestId('useShortUrl').click(); - // Wait for short URL to be generated (watch for attribute change) - cy.wait(2000); // Give time for short URL generation - cy.getElementByTestId('copyShareUrlButton') + .should('have.attr', 'data-share-url') + .and('not.include', 'app/') // Short URLs don't contain 'app/' path .then(($el) => {cypress/utils/commands.explore.js (3)
370-372: Hardcoded wait before Cancel click may be fragile.The 2-second wait followed by a Cancel button click seems like a workaround. Consider waiting for a specific element state instead.
♻️ Consider explicit wait condition
cy.getElementByTestId('confirmModalConfirmButton').click(); - cy.wait(2000); - cy.get('.euiFlyoutFooter').find('button[type="button"]').contains('Cancel').first().click(); + // Wait for confirmation to complete and flyout to be ready + cy.get('.euiFlyoutFooter') + .should('be.visible') + .find('button[type="button"]') + .contains('Cancel') + .first() + .click();
395-396: Pattern concern: Multiple 3-second hardcoded waits.There are several
cy.wait(3000)calls after dataset operations (lines 395, 450, 472, 527, 744). While these may be necessary for stability, consider:
- Adding comments explaining why 3 seconds is needed
- Extracting to a named constant (e.g.,
DATASET_OPERATION_DELAY = 3000)- Investigating if there's a deterministic signal to wait for instead
Also applies to: 450-451, 472-473, 527-528, 744-745
750-778: Address TODO or track in an issue.The
navigateToTracesViaLogsUrlcommand has a TODO comment indicating it's a temporary solution until an APM datasource is available. Consider:
- Creating an issue to track this technical debt
- Adding more context about when the proper solution should be implemented
The URL manipulation approach works but is fragile if URL structures change.
Would you like me to help create an issue to track the replacement of this workaround with proper APM datasource support?
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/02/dataset_selector.spec.js (2)
34-34: Consider more descriptive constant name.
noIndexPatternscould be misread as "no index patterns" rather than "number of index patterns". A clearer name would improve readability.♻️ Suggested naming
-const noIndexPatterns = 5; // Determines the no of index patterns that should be in the dropdown for filtering test case +const NUM_INDEX_PATTERNS_FOR_FILTER_TEST = 5; // Number of index patterns to create for dropdown filtering test
146-165: Consider extracting dataset creation loop to a helper.The eslint-disable comments for
no-loop-funcare necessary here due to Cypress's async nature. However, this logic could be extracted to a helper function for better readability and reusability.♻️ Optional extraction
// In helpers.js or shared.js const createMultipleDatasets = (count, basePattern, workspaceName, signalType, timestamp) => { for (let i = 1; i <= count; i++) { const currentDatasetId = getRandomizedDatasetId(); cy.get('@DATASOURCE_ID').then((datasourceId) => { cy.get(`@${workspaceName}:WORKSPACE_ID`).then((workspaceId) => { cy.osd.createDatasetByEndpoint( currentDatasetId, workspaceId, datasourceId, { title: `${basePattern.slice(0, i)}*`, signalType, timestamp, }, `${currentDatasetId}:DATASET_ID` ); }); }); } };cypress/utils/commands.osd.js (1)
1132-1134: Unusual expected status codes for DELETE operation.HTTP status 201 (Created) is atypical for a DELETE operation. Consider using 200 or 204 (No Content) which are standard success responses for deletions.
♻️ Proposed fix
- expect(response.status).to.be.oneOf([200, 201]); + expect(response.status).to.be.oneOf([200, 204]);cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/saved_search_in_dashboards.spec.js (2)
42-48: Intercept should be registered before the actions that trigger the request.The
cy.interceptfor AI mode disablement is registered aftercreateWorkspaceAndDatasetUsingEndpointcompletes. If the intercepted endpoint is called during workspace/dataset creation or navigation, it won't be mocked. Consider moving this intercept beforecy.osd.setupEnvAndGetDataSource.♻️ Proposed fix
before(() => { + // mock AI mode disablement - register early to intercept all requests + cy.intercept('GET', '**/enhancements/assist/languages*', { + statusCode: 200, + body: { + configuredLanguages: [], + }, + }); + cy.osd.setupEnvAndGetDataSource(DATASOURCE_NAME); // Create workspace and dataset using our new helper function createWorkspaceAndDatasetUsingEndpoint( DATASOURCE_NAME, workspaceName, datasetId, `${INDEX_WITH_TIME_1}*`, // Uses index pattern 'timestamp', // timestampField 'logs', // signalType ['use-case-observability'] // features ); - // mock AI mode disablement - cy.intercept('GET', '**/enhancements/assist/languages*', { - statusCode: 200, - body: { - configuredLanguages: [], - }, - });
57-58: Consider replacing hard-coded wait with a more reliable approach.The
cy.wait(10000)is a long arbitrary wait that may be insufficient in slow environments or wasteful in fast ones. Consider waiting for a specific condition or element instead.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/queries.spec.js (1)
70-85: Consider the empty query handling approach.Setting a single-space empty query (
' ') and then verifying for an empty string ('') relies on the query editor's whitespace normalization. This is fragile if the editor behavior changes.The extended 60000ms timeout for
discoverQueryElapsedMsis appropriate for empty queries that may return large result sets.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/saved_search.spec.js (1)
6-11: Import path inconsistency with other spec files.This file imports from
utils/apps/query_enhancements/sharedwhilequeries.spec.jsimports the same utilities fromutils/apps/explore/shared. Both export identical functions likegetRandomizedDatasetIdandgetRandomizedWorkspaceName, but this inconsistency may cause confusion during maintenance.Consider standardizing on one import path across all explore test files for consistency.
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_breakdown.spec.js (1)
50-53: Consider moving experimental setting tobefore()hook.Setting
explore:experimentalinbeforeEachmeans it runs before every test. If the setting persists across tests within the same describe block, moving it tobefore()would be more efficient.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.js (1)
47-48: Remove stale eslint-disable comment.The
no-loop-funceslint disable comment on line 48 appears to be leftover from previous code. There is no loop at this location - the code is simply using chained Cypress aliases.🔧 Suggested fix
// Creating the Non Timebased Dataset - // eslint-disable-next-line no-loop-func cy.get('@DATASOURCE_ID').then((datasourceId) => {
...ation/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/saved_queries.spec.js
Show resolved
Hide resolved
| const datasetId = getRandomizedDatasetId(); | ||
|
|
||
| const runHistogramBreakdownTests = () => { | ||
| // THe tests have some issues in Neo, They are currently skipped Need to revisit them again. |
There was a problem hiding this comment.
Typo in comment.
"THe" should be "The".
✏️ Suggested fix
- // THe tests have some issues in Neo, They are currently skipped Need to revisit them again.
+ // The tests have some issues in Neo. They are currently skipped. Need to revisit them again.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // THe tests have some issues in Neo, They are currently skipped Need to revisit them again. | |
| // The tests have some issues in Neo. They are currently skipped. Need to revisit them again. |
🤖 Prompt for AI Agents
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_breakdown.spec.js
at line 27, Fix the typo in the comment string "// THe tests have some issues in
Neo, They are currently skipped Need to revisit them again." by changing "THe"
to "The" (so it reads "// The tests have some issues in Neo, They are currently
skipped Need to revisit them again."); update the exact comment in
histogram_breakdown.spec.js to correct capitalization.
| cy.getElementByTestId('discoverChart').should('be.visible'); | ||
| }); | ||
|
|
||
| // |
There was a problem hiding this comment.
Empty comment should be removed or completed.
Line 83 contains an orphaned // comment with no content.
✏️ Suggested fix
- //
it(`check breakdown persistence with interval changes for ${config.testName}`, () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // | |
| it(`check breakdown persistence with interval changes for ${config.testName}`, () => { |
🤖 Prompt for AI Agents
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/histogram_breakdown.spec.js
at line 83, Remove the orphaned empty comment (`//`) in the test file
(histogram_breakdown.spec.js) — either delete that line or replace it with a
meaningful comment describing the surrounding test intent; ensure no leftover
blank comment lines remain in the file so the test code is clean.
.../integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/traces.spec.js
Outdated
Show resolved
Hide resolved
| const datasetId = getRandomizedDatasetId(); | ||
|
|
||
| export const runBuildVisTests = () => { | ||
| describe('build visualization manully tests', () => { |
There was a problem hiding this comment.
Typo: "manully" should be "manually".
- describe('build visualization manully tests', () => {
+ describe('build visualization manually tests', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('build visualization manully tests', () => { | |
| describe('build visualization manually tests', () => { |
🤖 Prompt for AI Agents
In
@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/06/build_vis.spec.js
at line 21, Typo in the test suite title: change the describe string in the test
file (the 'describe' call with title "build visualization manully tests") to use
the correct spelling "manually" so it reads "build visualization manually tests"
(or better "build visualization manually" if you prefer shorter). Update the
string in the describe(...) invocation to replace "manully" with "manually".
| export const WAIT_MS = Cypress.env('WAIT_MS'); | ||
| export const WAIT_MS_LONG = Cypress.env('WAIT_MS_LONG'); | ||
|
|
||
| // Avialble fixture and their mappings |
There was a problem hiding this comment.
Fix typo in comment.
"Avialble" should be "Available".
-// Avialble fixture and their mappings
+// Available fixtures and their mappings📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Avialble fixture and their mappings | |
| // Available fixtures and their mappings |
🤖 Prompt for AI Agents
In @cypress/utils/constants.js at line 26, Fix the typo in the comment above the
fixture mappings in cypress/utils/constants.js by changing "Avialble" to
"Available" so the comment reads "Available fixture and their mappings"; update
only the comment text to correct spelling without altering surrounding code or
behavior.
zhongnansu
left a comment
zhongnansu
left a comment
There was a problem hiding this comment.
please address comments from @coderabbitai, they seem valid to me
Signed-off-by: Suchit Sahoo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @cypress/utils/commands.osd.js:
- Around line 802-806: The dataset string assigned to the variable dataset is
missing a closing single quote after ${datasetOptions.title}, causing a
malformed URL; update the construction of dataset in commands.osd.js (the const
dataset line) to add the missing closing quote around title so it reads
title:'${datasetOptions.title}' and ensure the rest of the tuple
(type:INDEX_PATTERN) remains unchanged so the encoded URL is valid.
- Around line 1028-1030: The test incorrectly expects a 201 for a DELETE
response; update the assertion that checks response.status (the line using
expect(response.status).to.be.oneOf([...])) to expect 200 or 204 instead of 201,
and keep the existing cy.log('Workspace deleted successfully.') but ensure any
downstream code can handle a 204 No Content response if present.
- Around line 272-274: The code uses Cypress.env to set workspaceId and
immediately calls cy.visit(`/w/${workspaceId}/app/${page}`) without validating
workspaceId; add a guard that checks the workspaceId value returned by
Cypress.env (variable workspaceId) before calling cy.visit and if it's
undefined/empty fail fast with a clear error (e.g., throw or Cypress.fail)
including the env key name, or log and abort the test so you never call cy.visit
with "undefined"; ensure the validation is performed right after assigning
workspaceId and before any call to cy.visit or further navigation logic.
- Around line 289-317: The helper deleteWorkspaceByNameUsingEndpoint is missing
validation for workspaceId retrieved via
Cypress.env(`${workspaceName}:WORKSPACE_ID`), so ensure you check that
workspaceId is defined and non-empty before calling cy.request: if workspaceId
is undefined/null/empty, log a clear message (using cy.log or process logger)
and short-circuit by returning a resolved cy.wrap or throw a descriptive Error
so no request to /api/workspaces/undefined is made; update the beginning of
deleteWorkspaceByNameUsingEndpoint to validate workspaceId and handle the
invalid case accordingly instead of proceeding to cy.request.
- Around line 62-65: The alias stored as '@WORKSPACE_ID' is inconsistent with
other helpers (deleteWorkspaceByNameUsingEndpoint,
navigateToWorkSpaceSpecificPage) which expect Cypress.env() entries keyed as
`${workspaceName}:WORKSPACE_ID`; update the interception handler in the command
that uses cy.wait('@createWorkspaceInterception') to also set
Cypress.env(`${workspaceName}:WORKSPACE_ID`, id) after extracting the name and
id from the interception (e.g., interception.response.body.result.name and .id),
and optionally keep the existing cy.wrap(...).as('WORKSPACE_ID') for backward
compatibility.
🧹 Nitpick comments (5)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/traces.spec.js (1)
23-23: Consider moving this comment closer to thedescribe.skipstatement.This comment explaining why the tests are skipped is placed inside the
traceTestSuitefunction, approximately 690 lines away from the actualdescribe.skipon line 711. Placing it adjacent to the skip statement would improve discoverability for developers reviewing which tests are disabled and why.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/02/saved_search_in_dashboards.spec.js (1)
50-50: Remove commented-out code.This commented line appears to be leftover from a previous approach. Either remove it or add a comment explaining why it's preserved.
🧹 Suggested cleanup
['use-case-search'] // features ); - - // cy.osd.grabDataSourceId(workspaceName, DATASOURCE_NAME); });cypress/utils/commands.osd.js (3)
788-792: Consider storing workspace ID with workspace name prefix in Cypress.env.The ID is stored using
aliasName(default:'WORKSPACE_ID'), but other commands likedeleteWorkspaceByNameUsingEndpointandnavigateToWorkSpaceSpecificPageexpect the pattern${workspaceName}:WORKSPACE_ID. Consider also storing with the workspace name prefix for consistency:Proposed enhancement
// Save the workspace ID as an alias (with custom name to avoid conflicts) cy.wrap(response.id).as(aliasName); // Also store in Cypress env for persistence across runs Cypress.env(aliasName, response.id); + // Store with workspace name prefix for commands that use this pattern + Cypress.env(`${workspaceName}:WORKSPACE_ID`, response.id);
922-927: Missing datasource ID alias/env storage after creation.Unlike
addDataSource(line 165), this command doesn't save the created datasource ID as an alias or inCypress.env(). Callers of this command may need to subsequently callgetDataSourceIdto retrieve the ID, which adds an extra API call.Proposed enhancement
cy.request(createRequest).then((response) => { cy.log('Data source creation response:', JSON.stringify(response, null, 2)); expect(response.status).to.be.oneOf([200, 201]); + if (response.body && response.body.id) { + cy.wrap(response.body.id).as('DATASOURCE_ID'); + Cypress.env('DATASOURCE_ID', response.body.id); + } cy.log('Datasource created successfully.'); });
1018-1033: Consider unifying workspace ID retrieval pattern.This command uses
cy.get('@${workspaceName}:WORKSPACE_ID')(Cypress alias), whiledeleteWorkspaceByNameUsingEndpointusesCypress.env(). Having two deletion commands with different ID retrieval mechanisms can cause confusion. Consider either:
- Consolidating to one delete command, or
- Making both check both storage mechanisms
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/traces.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/02/saved_search_in_dashboards.spec.jscypress/utils/commands.osd.js
🧰 Additional context used
🧬 Code graph analysis (1)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/02/saved_search_in_dashboards.spec.js (2)
cypress/utils/apps/query_enhancements/shared.js (4)
getRandomizedWorkspaceName(26-27)getRandomizedWorkspaceName(26-27)getRandomizedDatasetId(33-33)getRandomizedDatasetId(33-33)cypress/utils/helpers.js (2)
createWorkspaceAndDatasetUsingEndpoint(112-147)createWorkspaceAndDatasetUsingEndpoint(112-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (67)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Lint and validate
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: bundle-analyzer
- GitHub Check: lighthouse
🔇 Additional comments (13)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/04/traces.spec.js (1)
711-713: Appropriate use ofdescribe.skipto temporarily disable tests during refactoring.The test suite is properly preserved for future re-enablement. Consider removing the "- SKIPPED" suffix from the suite name since
describe.skipalready indicates the skipped status in test runners' output.Optional: Simplify suite name
-describe.skip('Traces Test Suite - SKIPPED', () => { +describe.skip('Traces Test Suite', () => { prepareTestSuite('Traces', traceTestSuite); });cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/02/saved_search_in_dashboards.spec.js (5)
25-28: LGTM!The new imports for
prepareTestSuiteandcreateWorkspaceAndDatasetUsingEndpointalign with the PR's goal of switching to API-based setup utilities.
30-31: LGTM!The
datasetIdfollows the same randomization pattern asworkspaceName, ensuring unique identifiers for test isolation.
53-55: LGTM!Simplified cleanup using workspace-scoped operation aligns with the PR's goal of reducing test flakiness.
34-36: Verify the skipped tests will be addressed.The entire test suite is skipped with a TODO indicating the top nav has changed. Since this PR modifies the test setup, confirm there's a follow-up plan to update and re-enable these tests once the navigation flow is corrected.
33-33: Remove the dual registration concern; it is unfounded.The function
runSavedSearchTestsis not called from any other file in the codebase. The export and module-level invocation viaprepareTestSuiteat line 139 is an isolated pattern in this file only. No dual registration risk exists.Likely an incorrect or invalid review comment.
cypress/utils/commands.osd.js (7)
9-16: LGTM!Import additions are consistent with the new commands that utilize
defaultFieldsForDatasetCreationandAVAILABLE_FIXTURE_INDICES.
319-333: LGTM!Explicit timeout propagation improves test reliability in slow environments.
456-483: LGTM!The addition of the
useCaseparameter provides flexibility for different workspace types. The setup flow is logical.
517-524: LGTM!Replacing
cy.reload()withcy.visit(currentUrl)aligns with the PR objective to reduce flakiness in authenticated environments. This approach better preserves authentication state.
817-875: LGTM!The API-based approach for deleting saved queries is cleaner than UI-based deletion. The filtering logic correctly matches both workspace ID and exact title, preventing accidental deletions.
933-995: LGTM!The environment setup logic correctly handles both OSD and non-OSD environments. The datasource existence check before creation prevents duplicate datasources, and the fixture upload runs consistently regardless of datasource state.
735-741: No issue found — reference type is correct.The
type: 'OpenSearch'value is the established pattern for data source references in datasets and data views throughout the codebase. This is verified insrc/data/common/data_views/data_views/data_view.ts, data structure type definitions, and 50+ usages across test files and source code in the workspace, explore, data, and data source management plugins. The reference type is appropriate for the dataset API context.
| // Get workspace ID from Cypress env | ||
| const workspaceId = Cypress.env(`${workspaceName}:WORKSPACE_ID`); | ||
| cy.visit(`/w/${workspaceId}/app/${page}`); |
There was a problem hiding this comment.
No validation for undefined workspace ID before navigation.
If the workspace ID is not found in environment variables, this will navigate to /w/undefined/app/${page}, leading to confusing test failures.
Proposed fix
// Get workspace ID from Cypress env
const workspaceId = Cypress.env(`${workspaceName}:WORKSPACE_ID`);
+ if (!workspaceId) {
+ throw new Error(`Workspace ID not found for "${workspaceName}". Ensure workspace was created and ID was stored in Cypress.env.`);
+ }
cy.visit(`/w/${workspaceId}/app/${page}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get workspace ID from Cypress env | |
| const workspaceId = Cypress.env(`${workspaceName}:WORKSPACE_ID`); | |
| cy.visit(`/w/${workspaceId}/app/${page}`); | |
| // Get workspace ID from Cypress env | |
| const workspaceId = Cypress.env(`${workspaceName}:WORKSPACE_ID`); | |
| if (!workspaceId) { | |
| throw new Error(`Workspace ID not found for "${workspaceName}". Ensure workspace was created and ID was stored in Cypress.env.`); | |
| } | |
| cy.visit(`/w/${workspaceId}/app/${page}`); |
🤖 Prompt for AI Agents
In @cypress/utils/commands.osd.js around lines 272 - 274, The code uses
Cypress.env to set workspaceId and immediately calls
cy.visit(`/w/${workspaceId}/app/${page}`) without validating workspaceId; add a
guard that checks the workspaceId value returned by Cypress.env (variable
workspaceId) before calling cy.visit and if it's undefined/empty fail fast with
a clear error (e.g., throw or Cypress.fail) including the env key name, or log
and abort the test so you never call cy.visit with "undefined"; ensure the
validation is performed right after assigning workspaceId and before any call to
cy.visit or further navigation logic.
| cy.osd.add('deleteWorkspaceByNameUsingEndpoint', (workspaceName, endpoint) => { | ||
| const baseUrl = endpoint || Cypress.config('baseUrl') || ''; | ||
|
|
||
| // Get the workspace ID from environment variable or alias using the pattern ${workspaceName}:WORKSPACE_ID | ||
| const workspaceId = Cypress.env(`${workspaceName}:WORKSPACE_ID`); | ||
|
|
||
| return cy | ||
| .request({ | ||
| method: 'DELETE', | ||
| url: `${baseUrl}/api/workspaces/${workspaceId}`, | ||
| headers: { | ||
| 'osd-xsrf': true, | ||
| }, | ||
| failOnStatusCode: false, // Don't fail the test if workspace doesn't exist | ||
| }) | ||
| .then((resp) => { | ||
| if (resp.status === 200 || resp.status === 204) { | ||
| cy.log(`Successfully deleted workspace: ${workspaceName} (ID: ${workspaceId})`); | ||
| } else if (resp.status === 404) { | ||
| cy.log( | ||
| `Workspace ${workspaceName} (ID: ${workspaceId}) not found - may already be deleted` | ||
| ); | ||
| } else { | ||
| throw new Error( | ||
| `Delete workspace ${workspaceName} failed: ${resp?.body?.error || resp.statusText}` | ||
| ); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing validation for undefined workspace ID.
If Cypress.env(\${workspaceName}:WORKSPACE_ID`)returnsundefined(e.g., workspace was never created or env wasn't set), the request will be made to/api/workspaces/undefined`, which could produce confusing errors or unintended behavior.
Proposed fix
cy.osd.add('deleteWorkspaceByNameUsingEndpoint', (workspaceName, endpoint) => {
const baseUrl = endpoint || Cypress.config('baseUrl') || '';
// Get the workspace ID from environment variable or alias using the pattern ${workspaceName}:WORKSPACE_ID
const workspaceId = Cypress.env(`${workspaceName}:WORKSPACE_ID`);
+ if (!workspaceId) {
+ cy.log(`No workspace ID found for "${workspaceName}" - skipping deletion`);
+ return cy.wrap(null);
+ }
+
return cy
.request({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cy.osd.add('deleteWorkspaceByNameUsingEndpoint', (workspaceName, endpoint) => { | |
| const baseUrl = endpoint || Cypress.config('baseUrl') || ''; | |
| // Get the workspace ID from environment variable or alias using the pattern ${workspaceName}:WORKSPACE_ID | |
| const workspaceId = Cypress.env(`${workspaceName}:WORKSPACE_ID`); | |
| return cy | |
| .request({ | |
| method: 'DELETE', | |
| url: `${baseUrl}/api/workspaces/${workspaceId}`, | |
| headers: { | |
| 'osd-xsrf': true, | |
| }, | |
| failOnStatusCode: false, // Don't fail the test if workspace doesn't exist | |
| }) | |
| .then((resp) => { | |
| if (resp.status === 200 || resp.status === 204) { | |
| cy.log(`Successfully deleted workspace: ${workspaceName} (ID: ${workspaceId})`); | |
| } else if (resp.status === 404) { | |
| cy.log( | |
| `Workspace ${workspaceName} (ID: ${workspaceId}) not found - may already be deleted` | |
| ); | |
| } else { | |
| throw new Error( | |
| `Delete workspace ${workspaceName} failed: ${resp?.body?.error || resp.statusText}` | |
| ); | |
| } | |
| }); | |
| }); | |
| cy.osd.add('deleteWorkspaceByNameUsingEndpoint', (workspaceName, endpoint) => { | |
| const baseUrl = endpoint || Cypress.config('baseUrl') || ''; | |
| // Get the workspace ID from environment variable or alias using the pattern ${workspaceName}:WORKSPACE_ID | |
| const workspaceId = Cypress.env(`${workspaceName}:WORKSPACE_ID`); | |
| if (!workspaceId) { | |
| cy.log(`No workspace ID found for "${workspaceName}" - skipping deletion`); | |
| return cy.wrap(null); | |
| } | |
| return cy | |
| .request({ | |
| method: 'DELETE', | |
| url: `${baseUrl}/api/workspaces/${workspaceId}`, | |
| headers: { | |
| 'osd-xsrf': true, | |
| }, | |
| failOnStatusCode: false, // Don't fail the test if workspace doesn't exist | |
| }) | |
| .then((resp) => { | |
| if (resp.status === 200 || resp.status === 204) { | |
| cy.log(`Successfully deleted workspace: ${workspaceName} (ID: ${workspaceId})`); | |
| } else if (resp.status === 404) { | |
| cy.log( | |
| `Workspace ${workspaceName} (ID: ${workspaceId}) not found - may already be deleted` | |
| ); | |
| } else { | |
| throw new Error( | |
| `Delete workspace ${workspaceName} failed: ${resp?.body?.error || resp.statusText}` | |
| ); | |
| } | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In @cypress/utils/commands.osd.js around lines 289 - 317, The helper
deleteWorkspaceByNameUsingEndpoint is missing validation for workspaceId
retrieved via Cypress.env(`${workspaceName}:WORKSPACE_ID`), so ensure you check
that workspaceId is defined and non-empty before calling cy.request: if
workspaceId is undefined/null/empty, log a clear message (using cy.log or
process logger) and short-circuit by returning a resolved cy.wrap or throw a
descriptive Error so no request to /api/workspaces/undefined is made; update the
beginning of deleteWorkspaceByNameUsingEndpoint to validate workspaceId and
handle the invalid case accordingly instead of proceeding to cy.request.
| }).then((response) => { | ||
| expect(response.status).to.be.oneOf([200, 201]); | ||
| cy.log('Workspace deleted successfully.'); |
There was a problem hiding this comment.
Incorrect expected status code for DELETE operation.
Status code 201 (Created) is semantically incorrect for a DELETE operation. Typical successful DELETE responses are 200 (OK) or 204 (No Content).
Proposed fix
}).then((response) => {
- expect(response.status).to.be.oneOf([200, 201]);
+ expect(response.status).to.be.oneOf([200, 204]);
cy.log('Workspace deleted successfully.');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }).then((response) => { | |
| expect(response.status).to.be.oneOf([200, 201]); | |
| cy.log('Workspace deleted successfully.'); | |
| }).then((response) => { | |
| expect(response.status).to.be.oneOf([200, 204]); | |
| cy.log('Workspace deleted successfully.'); |
🤖 Prompt for AI Agents
In @cypress/utils/commands.osd.js around lines 1028 - 1030, The test incorrectly
expects a 201 for a DELETE response; update the assertion that checks
response.status (the line using expect(response.status).to.be.oneOf([...])) to
expect 200 or 204 instead of 201, and keep the existing cy.log('Workspace
deleted successfully.') but ensure any downstream code can handle a 204 No
Content response if present.
|
@zhongnansu Sure. I have addressed most of the major and critical comments. I will raise a subsequent PR to clean up the minor ones. |
LDrago27
changed the title
...integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/queries.spec.js
Show resolved
Hide resolved
|
Thank you sir! |
4 participants
Description
Improve the test utils used by Explore & Enhancements
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
New Features
Infrastructure
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.