Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ui): front end sorting for numeric values now being handled correctly #16237

Merged
merged 4 commits into from
Dec 17, 2019
Merged

fix(ui): front end sorting for numeric values now being handled correctly #16237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41a7c50
fix(ui): front end table sort now works for numbers, need to add testing
asalem1 Dec 17, 2019
9b55897
fix(ui): added tests to sorting on front end
asalem1 Dec 17, 2019
e34b1c7
chore(ui): added PR to changelog
asalem1 Dec 17, 2019
0181561
fix(ui): skipping test until flake is resolved
asalem1 Dec 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

### Features

1. [16234](https://github.com/influxdata/influxdb/pull/16234): add support for notification endpoints to influx templates/pkgs.
1. [16234](https://github.com/influxdata/influxdb/pull/16234): add support for notification endpoints to influx templates/pkgs.

### Bug Fixes
1. [16225](https://github.com/influxdata/influxdb/pull/16225): Ensures env vars are applied consistently across cmd, and fixes issue where INFLUX_ env var prefix was not set globally.

1. [16225](https://github.com/influxdata/influxdb/pull/16225): Ensures env vars are applied consistently across cmd, and fixes issue where INFLUX\_ env var prefix was not set globally.

1. [16235](https://github.com/influxdata/influxdb/pull/16235): Removed default frontend sorting when flux queries specify sorting
1. [16238](https://github.com/influxdata/influxdb/pull/16238): Store canceled task runs in the correct bucket
1. [16237](https://github.com/influxdata/influxdb/pull/16237): Updated Sortby functionality for table frontend sorts to sort numbers correctly

### UI Improvements

Expand Down
25 changes: 21 additions & 4 deletions ui/cypress/e2e/explorer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,9 @@ describe('DataExplorer', () => {
})
})

describe('visualize tables', () => {
// skipping until the sigin flake gets resolve in:
// https://github.com/influxdata/influxdb/issues/16253
describe.skip('visualize tables', () => {
const numLines = 360

beforeEach(() => {
Expand All @@ -623,7 +625,7 @@ describe('DataExplorer', () => {
})
})

it('can view table data', () => {
it('can view table data & sort values numerically', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look and try to reproduce it locally and see whether I can make it less flakey

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

until we sort this out, i think you should skip this test and link to this issue: #16253

// build the query to return data from beforeEach
cy.getByTestID(`selector-list m`).click()
cy.getByTestID('selector-list v').click()
Expand Down Expand Up @@ -654,8 +656,23 @@ describe('DataExplorer', () => {
})
cy.getByTestID('_value-table-header').click()
cy.get('.table-graph-cell__sort-asc').should('exist')
// 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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@asalem1 asalem1 Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')
.should('exist')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi, cy.get('foo').should('exist') is almost always superfluous. the get operation implicitly asserts existence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asalem1

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?

.then(el => {
// get the column index
const columnIndex = el[0].getAttribute('data-column-index')
let prev = Infinity
// get all the column values for that one and see if they are in order
cy.get(`[data-column-index="${columnIndex}"]`).each(val => {
const num = Number(val.text())
if (isNaN(num) === false) {
expect(num < prev).to.equal(true)
prev = num
}
})
})
})
})
})
Expand Down
12 changes: 11 additions & 1 deletion ui/src/dashboards/utils/tableGraph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,17 @@ export const sortTableData = (
const dataValues = _.drop(data, 1)
const sortedData = [
data[0],
..._.orderBy<string[][]>(dataValues, sortIndex, [sort.direction]),
..._.orderBy<string[][]>(
dataValues,
row => {
const sortedValue = row[sortIndex]
if (isNaN(Number(sortedValue))) {
return sortedValue
}
return Number(sortedValue)
},
[sort.direction]
),
] as string[][]

const sortedTimeVals = fastMap<string[], string>(
Expand Down