fix(ui): front end sorting for numeric values now being handled correctly #16237
Conversation
ghost
requested review from
hoorayimhelping and
zoesteinkamp
and removed request for
a team
There was a problem hiding this comment.
I was trying to run the e2e test with it.only to see if it's possible to clean up the testing code a little bit, and the test crashes when run with only. Looks like we're inadvertently coupling our tests together, which is an antipattern in cyperess.
| // TODO: complete testing when FE sorting functionality is sorted | ||
| // https://github.com/influxdata/influxdb/issues/16200 | ||
| cy.getByTestID('_value-table-header').click() | ||
| cy.get('.table-graph-cell__sort-desc').should('exist') |
There was a problem hiding this comment.
i like the eagerness to test a lot of things, but i think this is bit too implementation heavy, in that it's testing implementation over behavior. it's an implementation detail that we change the class to reflect the sort order. the behavior is that we change the sort order, which you're testing below. i think testing this will lead to brittle tests, because if we ever want to change the name of the class, or the component the class targets, or any implementation detail around showing an indicator of the sort order, these tests will likely break. i don't think we should assert on these things.
There was a problem hiding this comment.
It's a bit of both. The only indication that a specific cell header isn't toggled by the front end sort is the classname associated with it, hence the check for it. I do think it's necessary in this case to perform this check here so that we can validate that an action has occurred that is reflected in the UI, in addition to its functionality
There was a problem hiding this comment.
i don't think it's necessary at all - it really feels like an implementation detail: we could change our underlying css so that we render columns sorted ascending with some kind squiggly underline and columns sorted descending with a dashed outline. making those changes would render this test invalid.
either way, i think it's enough discussion on a test - i think this is a bit implementation heavy, but if you're comfortable with this, go for it
| cy.getByTestID('_value-table-header').click() | ||
| cy.get('.table-graph-cell__sort-desc').should('exist') | ||
| cy.getByTestID('_value-table-header') | ||
| .should('exist') |
There was a problem hiding this comment.
fyi, cy.get('foo').should('exist') is almost always superfluous. the get operation implicitly asserts existence.
There was a problem hiding this comment.
Good point, but it is a way of maintaining UI stability in that we want to know whether the UI is manipulated by the click event
There was a problem hiding this comment.
but it is a way of maintaining UI stability in that we want to know whether the UI is manipulated by the click event
i'm curious how should('exist') does that where cy.getByTestID('_value-table-header') doesn't?
| @@ -623,7 +623,7 @@ describe('DataExplorer', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| it('can view table data', () => { | |||
| it('can view table data & sort values numerically', () => { | |||
There was a problem hiding this comment.
this test fails when run with it.only which means it's coupled to another part of the test. i think this needs to get fixed.
There was a problem hiding this comment.
I'll take a look and try to reproduce it locally and see whether I can make it less flakey
There was a problem hiding this comment.
until we sort this out, i think you should skip this test and link to this issue: #16253
| cy.getByTestID('_value-table-header').click() | ||
| cy.get('.table-graph-cell__sort-desc').should('exist') | ||
| cy.getByTestID('_value-table-header') | ||
| .should('exist') |
There was a problem hiding this comment.
but it is a way of maintaining UI stability in that we want to know whether the UI is manipulated by the click event
i'm curious how should('exist') does that where cy.getByTestID('_value-table-header') doesn't?
| // TODO: complete testing when FE sorting functionality is sorted | ||
| // https://github.com/influxdata/influxdb/issues/16200 | ||
| cy.getByTestID('_value-table-header').click() | ||
| cy.get('.table-graph-cell__sort-desc').should('exist') |
There was a problem hiding this comment.
i don't think it's necessary at all - it really feels like an implementation detail: we could change our underlying css so that we render columns sorted ascending with some kind squiggly underline and columns sorted descending with a dashed outline. making those changes would render this test invalid.
either way, i think it's enough discussion on a test - i think this is a bit implementation heavy, but if you're comfortable with this, go for it
| @@ -623,7 +623,7 @@ describe('DataExplorer', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| it('can view table data', () => { | |||
| it('can view table data & sort values numerically', () => { | |||
There was a problem hiding this comment.
until we sort this out, i think you should skip this test and link to this issue: #16253
…ctly (#16237) fix(ui): front end table sort now works for numbers, skipping test until flake is resolved
Closes #16200
Problem
Sorting tables via the front end sorted resulted in unordered numbers due to the fact that numbers were being ordered as strings
Solution
Type checked to-be sorted values to ensure that values are numeric before sorting. Completed testing for table sorting to ensure stability