Zs sortbyseconds #14923 #15728
Zs sortbyseconds #14923 #15728
Conversation
Fixed the buckets test and files
Fixing JStest
Fixing e2e test
Fixing flakey test
fixing jstest
ghost
requested review from
desa and
hoorayimhelping
and removed request for
a team
| @@ -89,7 +126,7 @@ describe('Buckets', () => { | |||
| .click() | |||
| }) | |||
|
|
|||
| it('closes the overlay upon a successful delete with predicate submission', () => { | |||
| it.skip('closes the overlay upon a successful delete with predicate submission', () => { | |||
There was a problem hiding this comment.
can you add a quick comment here explaining why we're skipping this please?
There was a problem hiding this comment.
adding in comment! im going to make a new issue to address this failing test and another
| @@ -26,7 +26,7 @@ import {SortTypes} from 'src/shared/utils/sort' | |||
| // Utils | |||
| import {prettyBuckets} from 'src/shared/utils/prettyBucket' | |||
|
|
|||
| type SortKey = keyof PrettyBucket | |||
| type SortKey = keyof PrettyBucket | 'retentionRules[0].everySeconds' | |||
There was a problem hiding this comment.
this is kind of setting off a spidey sense - seems like we're coupling the state and shape of our redux store to a value type - the end result is that the sorting code relies on the precise shape and existence of the store having a first item in retentionRules with an everySeconds value. if any of that isn't there or the shape changes (maybe the api gets updated), this could very likely break.
here's the thing though, i know you didn't make this pattern, and i don't know of a better way to do this without changing a lot of code, so i can't really do more than say keep en eye out on this sorting code cause it might break unexpectedly. (said without judgement of the quality of the code or the circumstances it was written under :))
There was a problem hiding this comment.
hopefully is things go south the tests will catch it, but yeah it is slightly concerning, but i dont think right now is the time to refactor all the logic. Well keep an eye to see if any related issues rear their head
fixing the comments
Closes #14923
Describe your proposed changes here.