new: STORIF-310 - Bucket tab filters created.#13481
new: STORIF-310 - Bucket tab filters created.#13481dchyrva-akamai wants to merge 1 commit intolinode:developfrom
Conversation
dchyrva-akamai
force-pushed
the
feature/STORIF-310
branch
from
7d67d23 to
bd33b56
Compare
dchyrva-akamai
requested review from
jdamore-linode
and removed request for
a team
dchyrva-akamai
force-pushed
the
feature/STORIF-310
branch
from
bd33b56 to
67b581b
Compare
dchyrva-akamai
force-pushed
the
feature/STORIF-310
branch
from
67b581b to
a35a650
Compare
Cloud Manager UI test results🔺 1 failing test on test run #4 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/kubernetes/lke-create.spec.ts" |
|||||||||||||||||
| ).then(() => { | ||
| cy.visitWithLogin('/object-storage/buckets'); | ||
| cy.wait('@getBuckets'); | ||
| cy.wait('@getRegions').then(({ response }) => { |
There was a problem hiding this comment.
It might not work if region is fetched before buckets. To wait for both buckets and region disregarding the order, you could do:
cy.wait(['@getBuckets', @getRegions'].then(([, regionsResponse]) => {
There was a problem hiding this comment.
Region multiselect should not be rendered until buckets finish loading. I understand that in the real application Regions might be fetched from some other place and stay in the cache, but I am not sure how it works in the test.
| (region) => region.id === selectedBucket.cluster.region | ||
| ); | ||
|
|
||
| const selectedRegionLabel = selectedRegion |
There was a problem hiding this comment.
The test should target a specific scenario - the region should either be defined, or not. In this case, I recommend to fail early in case region is undefined:
expect(selectedRegion, `expected region matching ${selectedBucket.cluster.region}`).to.exist;
|
|
||
| const bucketsDetails = new Array(2).fill({}).map(() => ({ | ||
| label: randomLabel(), | ||
| cluster: chooseCluster(), |
There was a problem hiding this comment.
The test seems to be non-deterministic. What if you select the same cluster twice? Or 2 clusters from the same region?
| * - Confirms that when region is selected, endpoint multiselect. | ||
| * shows only endpoints related to the selected region. | ||
| */ | ||
| it('should filter list of endpoinds when region is selected', () => { |
| ).filter((option) => { | ||
| if (selectedRegions.length) { | ||
| return selectedRegions.some((region) => | ||
| option.label.includes(region.value) |
There was a problem hiding this comment.
You can't rely on region being part of the endpoint. Instead, you could filter the buckets by regions first, and then map them to endpoints. (This is a mandatory change)
| const onChangeMock = vi.fn(); | ||
|
|
||
| describe('RegionMultiselect', () => { | ||
| it('should show loading text while fetching endpoints', async () => { |
There was a problem hiding this comment.
Should it be "regions" instead of "endpoints"?
| import type { RegionMultiselectValue } from './RegionMultiselect'; | ||
|
|
||
| const queryMocks = vi.hoisted(() => ({ | ||
| useRegionsQuery: vi.fn().mockReturnValue([]), |
There was a problem hiding this comment.
Shouldn't it be:
{
data: [],
isFetching: true,
}
?
There was a problem hiding this comment.
Value is mocked in each specific test case, this pattern is used across all the similar tests.
There was a problem hiding this comment.
If this is just a placeholder mock, wouldn't it make more sense to return null or empty object?
| expect(getByPlaceholderText('Loading Regions...')).toBeVisible(); | ||
| }); | ||
|
|
||
| it('should show proper placeholder after fetching regions', async () => { |
There was a problem hiding this comment.
async don't really needed.
| return regions.filter((region) => endpointRegions.has(region.id)); | ||
| }; | ||
|
|
||
| export const uniqueByKey = (arr: Array<any>, key: string) => { |
There was a problem hiding this comment.
As eslint suggests, try to avoid declaring "any" type, instead use generic types. Also, declare the returning type, for example:
export const uniqueByKey = <T extends Record<string, unknown>>(arr: T[], key: string): T[] => {
github-project-automation
bot
moved this from Review
to Changes Requested
in Cloud Manager
3 participants
Description 📝
Bucket tab filters created.
Preview 📷
How to test 🧪
pnpm dev./object-storage/bucketspage.Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅