Fix dynamically display columns #103
Conversation
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
KishoreKicha14
requested review from
ansjcy,
jainankitk,
deshsidd and
dzane17
as code owners
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
|
Why are the latency, cpu and memory columns displayed twice in each screenshot? @KishoreKicha14 |
|
If it’s not too big a change, we can debate about changing the column names to average latency, average cpu…… and so on for only groups? What do others think about this? |
There was a problem hiding this comment.
Other than the issues @deshsidd mentioned above, the rest of the logic looks good to me!
@KishoreKicha14 Thanks for the change, could you also add unit tests and cypress tests in this PR as well?
| @@ -259,6 +275,15 @@ const QueryInsights = ({ | |||
| truncateText: true, | |||
| }, | |||
| ]; | |||
| const columnsToShow = selectedFilter === 'SIMILARITY' ? groupColumns : queryColumns; | |||
| const filteredQueries = queries.filter((query) => { | |||
There was a problem hiding this comment.
How about
const filteredQueries = queries.filter(query =>
!selectedFilter || query.group_by === selectedFilter
);
?
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
cypress/e2e/1_top_queries.cy.js
Outdated
| @@ -22,7 +22,7 @@ const clearAll = () => { | |||
| }; | |||
|
|
|||
| describe('Query Insights Dashboard', () => { | |||
| // Setup before each test | |||
| // // Setup before each test | |||
cypress/e2e/1_top_queries.cy.js
Outdated
| cy.contains('.euiTableHeaderCell', header).should('exist'); | ||
| }); | ||
| }); | ||
| beforeEach(() => { |
There was a problem hiding this comment.
Why do we need beforeEach here? this looks identical to
cypress/e2e/1_top_queries.cy.js
Outdated
| cy.get('.euiFilterSelectItem').contains('group').click(); | ||
|
|
||
| // Wait for table update | ||
| cy.get('.euiTableHeaderCell').should('have.length.lessThan', 20); |
There was a problem hiding this comment.
Why dont we assert a fixed length here? The number of headers in each scenarios should be deterministic.
cypress/e2e/1_top_queries.cy.js
Outdated
| ]; | ||
|
|
||
| // Wait for the table to load by checking if headers are visible | ||
| cy.get('.euiTableHeaderCell').should('have.length.greaterThan', 6); |
There was a problem hiding this comment.
why greater then instead of equals?
|
|
||
| await waitFor(() => expect(screen.getByRole('table')).toBeInTheDocument()); | ||
| const headers = await waitFor(() => screen.getAllByRole('columnheader', { hidden: false })); | ||
| console.log( |
| const expectedHeaders = ['ID', 'Type', 'Query Count', 'Latency', 'CPU Time', 'Memory Usage']; | ||
| expectedHeaders.forEach((header) => { | ||
| expect( | ||
| headers.some((h) => h.textContent?.trim().toLowerCase() === header.toLowerCase()) |
There was a problem hiding this comment.
why headers.some?
we should also compare the actual value without ignoring the case.
| renderQueryInsights(); | ||
| await waitFor(() => expect(screen.getByRole('table')).toBeInTheDocument()); | ||
| const typeElements = screen.getAllByText('Type'); | ||
| const typeFilterButton = typeElements.find((el) => el.closest('button')); // Ensure it's a button |
There was a problem hiding this comment.
It is a button. why use closest?
|
|
||
| await waitFor(() => { | ||
| expectedHeaders.forEach(async (header) => { | ||
| expect(await screen.findByText(header)).toBeInTheDocument(); |
There was a problem hiding this comment.
instead of finding on the whole screen and assert to be in document, could you narrow down the test to that specific table?
| ]; | ||
|
|
||
| await waitFor(() => { | ||
| expectedHeaders.forEach(async (header) => { |
There was a problem hiding this comment.
Also, the sequence matters here. let's also test the header sequence.
cypress/e2e/1_top_queries.cy.js
Outdated
|
|
||
| // Verify each expected header exists | ||
| expectedHeaders.forEach((header) => { | ||
| cy.contains('.euiTableHeaderCell', header).should('exist'); |
There was a problem hiding this comment.
The sequence matters here. let's also test the header sequence.
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Description
Problem:
The Top N queries table can display:
Only queries
Only groups
Combination of queries/groups
Some columns are applicable to only queries (eg: indices) and others to only groups (eg: query count).
Table should dynamically display the applicable columns based on the data in the table and only display the applicable columns.
This PR ensures that even when no selection is made and all query types are groups, the applicable columns are dynamically displayed.
Issues Resolved
Closes: [#45]
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.