[TESTID-58,59] Inspect functionality for Discover and Visualizations #9292
Conversation
Signed-off-by: Argus Li <[email protected]>
Signed-off-by: Argus Li <[email protected]>
Signed-off-by: Argus Li <[email protected]>
ArgusLi
requested review from
ananzh,
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi,
mengweieric,
LDrago27,
virajsanghvi,
sejli,
joshuali925 and
huyaboo
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9292 +/- ##
=======================================
Coverage 61.70% 61.71%
=======================================
Files 3816 3816
Lines 91824 91829 +5
Branches 14542 14543 +1
=======================================
+ Hits 56664 56668 +4
Misses 31506 31506
- Partials 3654 3655 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@ArgusLi could you upload a video? |
ananzh
added
v2.19.0
backport 2.x
discover_2.0-test
...rch_dashboards/opensearch_dashboards/apps/query_enhancements/field_display_filtering.spec.js
Outdated
Show resolved
Hide resolved
| cy.intercept('POST', '**/search/*').as('docTablePostRequest'); | ||
|
|
||
| cy.getElementByTestId('docTable').get('tbody tr').should('have.length.above', 3); // To ensure it waits until a full table is loaded into the DOM, instead of a bug where table only has 1 hit. | ||
| docTable.toggleDocTableRow(0); |
There was a problem hiding this comment.
Why this toggleDocTableRow is to // To ensure it waits until a full table is loaded into the DOM, instead of a bug where table only has 1 hit.?
There was a problem hiding this comment.
The comment is for cy.getElementByTestId('docTable').get('tbody tr').should('have.length.above', 3);. I can move the comment above the line instead if it makes it more legible?
| * Toggle expansion of row rowNumber of Doc Table. | ||
| * @param {number} rowNumber rowNumber of Doc Table starts at 0 for row 1. | ||
| */ | ||
| export const toggleDocTableRow = (rowNumber) => { |
There was a problem hiding this comment.
nit: maybe call it expandDocTableRow is more straightforward?
There was a problem hiding this comment.
I would prefer that name, but it can also shrink the tableRow, so I think toggle is a better word.
| ]; | ||
|
|
||
| /** | ||
| * Gets Date and returns string of date in format Jan 24, 2025 @ 16:20:08.000 |
There was a problem hiding this comment.
should add example for input and output
There was a problem hiding this comment.
should not Jan 24, 2025 @ 16:20:08.000 in overall description. we can use it as example as line 24. check my comment for formatValue and add more details.
| } | ||
|
|
||
| /** | ||
| * Format value to the corresponding string expected in the UI. |
There was a problem hiding this comment.
| * Format value to the corresponding string expected in the UI. | |
| /** | |
| * Formats values for UI display by: | |
| * - Adding thousand separators to numbers using locale settings | |
| * - Converting SQL/PPL datetime strings (YYYY-MM-DD HH:MM:SS.SSS) to formatted dates | |
| * - Passing through other values unchanged | |
| * @param {string|number} value - The value to format | |
| * @returns {string} The formatted value | |
| * @example | |
| * formatValue(1000) // "1,000" | |
| * formatValue("2025-01-24 16:20:08.000") // "Jan 24, 2025 @ 16:20:08.000" | |
| */ |
There was a problem hiding this comment.
nit: It would be great if you could have separate functions for number, string and date, like formatNumber, formatString and formatDate. Then in the formatValue you call them. I think in sharing or some other tests, we also need to format date. So it would be great to have separate format helpers.
There was a problem hiding this comment.
I have separated it and moved all format functions into shared.js.
| }; | ||
|
|
||
| /** | ||
| * Flatten the object in the format e.g. { |
There was a problem hiding this comment.
| * Flatten the object in the format e.g. { | |
| /** | |
| * Recursively flattens a nested object structure into dot notation and formats all values | |
| * @param {object} obj - Object to be flattened | |
| * @param {string} [parentKey] - Key prefix for nested properties | |
| * @param {object} [properties={}] - Accumulator for flattened properties | |
| * @returns {object} Flattened object with formatted values | |
| * @example | |
| * const input = { | |
| * FlightNum: "SYS76NQ", | |
| * DestLocation: { lat: "38.94449997", lon: "-77.45580292" } | |
| * }; | |
| * flattenObjectAndFormatValues(input) | |
| * // Returns: { | |
| * // FlightNum: "SYS76NQ", | |
| * // "DestLocation.lat": "38.94449997", | |
| * // "DestLocation.lon": "-77.45580292" | |
| * // } | |
| */ |
better to have both input and output and don't put the example in the overall discription
| }; | ||
|
|
||
| /** | ||
| * Flatten the array in the format e.g. [ |
There was a problem hiding this comment.
description is not clear. suggest to change to Flattens an array of field definitions and extracts values for a specific row
| } | ||
| }; | ||
|
|
||
| /** |
There was a problem hiding this comment.
| /** | |
| /** | |
| * Verifies that specified visualizations in the Flights dashboard do not have inspect options | |
| * Tests each visualization in visualizationTitlesWithNoInspectOptions array | |
| */ |
| }; | ||
|
|
||
| /** | ||
| * In the Flights dashboard, verify that the visualizations identified as having an inspect option have the option. |
Signed-off-by: Argus Li <[email protected]>
Signed-off-by: Argus Li <[email protected]>
Signed-off-by: Argus Li <[email protected]>
Signed-off-by: Argus Li <[email protected]>
Signed-off-by: Argus Li <[email protected]>
|
Can you update ciGroup10 or 12 in include this test? |
Signed-off-by: Argus Li <[email protected]>
Signed-off-by: Argus Li <[email protected]>
Signed-off-by: Argus Li <[email protected]>
Signed-off-by: Argus Li <[email protected]>
| cy.deleteWorkspaceByName(workspaceName); | ||
| cy.visit('/app/home'); | ||
| cy.osd.createInitialWorkspaceWithDataSource(datasourceName, workspaceName); | ||
| cy.wait(2000); |
There was a problem hiding this comment.
i don't think we need this wait here
| config.language | ||
| ); | ||
|
|
||
| cy.log(flattenedFieldsWithValues); |
There was a problem hiding this comment.
Nope, should have cleaned it up.
| (config.language === QueryLanguages.SQL.name || | ||
| config.language === QueryLanguages.PPL.name) | ||
| ) { | ||
| cy.log(`Skipped for ${key}`); |
There was a problem hiding this comment.
I want to keep this here. These are skipped because of a bug (#9305), and I want to make it clear they are skipped. I'll add the link to the bug to the comment as well.
Removed a log and a wait. Signed-off-by: Argus Li <[email protected]>
Description
Adds a Cypress test spec for the inspect functionality in the Discover and Dashboards pages.
Issues Resolved
Closes #8955, #8956.
Screenshot
inspect.mp4
Testing the changes
With OSD running, run yarn run cypress open. In E2E specs, you will see 1 new test spec inspect.spec.js. Run the test spec.
Changelog