feat(ui): added labels to buckets #16855
Conversation
| @@ -61,7 +61,7 @@ const UpdateBucketOverlay: FunctionComponent<Props> = ({ | |||
| handleClose() | |||
| return | |||
| } | |||
| setBucketDraft(resp.data) | |||
| setBucketDraft(resp.data as Bucket) | |||
There was a problem hiding this comment.
setting this type here as a way of resolving a TS error that stemmed from a disparity between the expectations of the useState func and the data type being returned by the getBucket function
ui/cypress/e2e/buckets.test.ts
Outdated
| @@ -61,27 +75,28 @@ describe('Buckets', () => { | |||
| describe('Searching and Sorting', () => { | |||
| it('can sort by name and retention', () => { | |||
| cy.getByTestID('name-sorter').click() | |||
| cy.getByTestID('bucket-card') | |||
|
|
|||
| cy.get('.cf-resource-card') | |||
There was a problem hiding this comment.
why did these get less specific? that's a trigger to go add testID params.
There was a problem hiding this comment.
Great question. The original was created to test out the sort order. In order to validate those assumptions, we were getting all the bucket-cards (previously by a less specific test-ID that worked). Then we were clicking the sort order to see if the first value changed (since it should've been reflected in the UI). I tried creating a solution where we were getting the resource list body and seeing if the children were in a particular order, but couldn't get that configured in cypress. The solution I came up with was to revert to the original construct (generalized grab all the cards), then see if the first one matched our expectations when changing the sort order
There was a problem hiding this comment.
are you talking about not being able to address this:
https://github.com/influxdata/influxdb/pull/16855/files#diff-aef71c9cb50e63e41a914808cdd0aac6R54
with something like this?:
https://github.com/influxdata/influxdb/blob/master/ui/cypress/e2e/collectors.test.ts#L228
There was a problem hiding this comment.
Not really. So the label tests are working magically without any issue because of the specificity added to the test-IDs.
What you linked seems to be a much better way of addressing the tests. I'll go ahead and refactor it so it looks like:
| <ResourceList.Body emptyState={emptyState}> | ||
| {this.listBuckets} | ||
| </ResourceList.Body> | ||
| <GetResources resources={[ResourceType.Labels]}> |
There was a problem hiding this comment.
can we put this next to where buckets are fetched?
There was a problem hiding this comment.
@drdelambre when I did, the empty state was never fulfilled, so I moved it out one layer (by empty state, I mean there's a message that's typically displayed when there are no buckets output in a search)
There was a problem hiding this comment.
here i think:
ui/cypress/e2e/buckets.test.ts
Outdated
| cy.getByTestID('name-sorter') | ||
| .click() | ||
| .then(() => { | ||
| cy.get('.cf-resource-name').each((val, index) => { |
There was a problem hiding this comment.
still have to change this (and change bucket + name back to bucket on the testid in the component)
There was a problem hiding this comment.
why is that? We changed it for greater specificity to access a specific bucket card.
There was a problem hiding this comment.
so use that specificity in the test by generating ids to look up (instead of evaluating the text value). i think we're still straddling two solutions right now and just need to pick one and push it through.
There was a problem hiding this comment.
@drdelambre i'm not quite sure I understand your solution, would you mind expanding upon that with an example of what you had in mind?
There was a problem hiding this comment.
yeah. so you have the name of the resource in both the test id and in the text content of the dom node you are trying to pinpoint. So you have two sources of truth, and two directions to go to unify them:
- revert the test id specificity in the component, and test that the value within the component (with
expect(val.text()).to.include(buckets[index])) is equal to the resource name. that keeps things scoped to buckets (good) but also ties the representation of the key to the value of the key (bad). - don't test for the value of the node, but instead look for the test id that meets the key you are looking for(
cy.getByTestId(`bucket ${cool name}`)). this scopes it to the bucket (good) as well as decouples the representation from the state (good)
seems like you're leaning towards #2. either one is fine, it's just that generic resource selector has got to go.
There was a problem hiding this comment.
but wouldn't the second solution defeat the purpose of the test? If we're trying to test the sorting mechanism to present a specific order, targeting a bucket by the name would only validate that the bucket exists. It wouldn't represent the order with which that bucket exists in the context of the page
Closes #16740
Problem
Buckets did not have labels
Solution
Added labels to buckets, normalized some data, add buckets label test