-
Notifications
You must be signed in to change notification settings - Fork 354
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.
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
test: [M3-8485] - Unit Tests for Object Storage Gen2 feature #10862
test: [M3-8485] - Unit Tests for Object Storage Gen2 feature #10862
Conversation
475d702
to
35c2bab
Compare
Coverage Report: ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ confirmed tests pass - thanks for adding test coverage!
Some v small comments + one question below
packages/manager/src/features/ObjectStorage/BucketLanding/BucketDetailsDrawer.test.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/ObjectStorage/BucketLanding/BucketDetailsDrawer.test.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/ObjectStorage/BucketLanding/BucketTable.test.tsx
Show resolved
Hide resolved
@@ -141,7 +141,7 @@ export const BucketDetailsDrawer = React.memo( | |||
)} | |||
{/* @TODO OBJ Multicluster: use region instead of cluster if isObjMultiClusterEnabled | |||
to getBucketAccess and updateBucketAccess. */} | |||
{ | |||
{Boolean(endpoint_type) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the changes in this file just for stuff that was missed earlier? I don't see anything mentioned in the AC or ticket description about them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these changes are stuff that was missed earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests passed! thank you adding unit test coverage.
The ticket mentions updating integration tests to reflect the new changes. Is this still required (perhaps updating |
@hkhalil-akamai the ticket asked to perform integration tests to ensure the UI changes function as expected without affecting other feature. If we need to update the integration tests I guess handling that in a different ticket/PR would be better |
…cts-unit-and-itegration-tests
…cts-unit-and-itegration-tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using renderWithThemeAndHookFormContext
is unnecessary since the component doesn't need a Hook Form Context. Use renderWithTheme
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the component doesn't need a Hook Form, but M3-8501(optimzing AccessSelect.tsx using Hook Form) is in progress. Hence, I decided to go with renderWithThemeAndHookFormContext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh understood. Sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference, each PR should be self-contained and generally should not include changes for another ticket. This prevents issues when, for example, the other ticket is closed or modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted 🗒️
const { getByTestId } = renderWithThemeAndHookFormContext({ | ||
component: ( | ||
<BucketDetailsDrawer | ||
onClose={mockOnClose} | ||
open={true} | ||
selectedBucket={e3Bucket} | ||
/> | ||
), | ||
options: { | ||
flags: { objMultiCluster: false, objectStorageGen2: { enabled: true } }, | ||
}, | ||
}); | ||
|
||
const rateLimitTable = getByTestId('bucket-rate-limit-table'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is failing since isObjectStorageGen2Enabled
is derived from an RQ hook and is initially undefined
. Awaiting via await findByTestId
will wait for the query to resolve.
const { getByTestId } = renderWithThemeAndHookFormContext({ | |
component: ( | |
<BucketDetailsDrawer | |
onClose={mockOnClose} | |
open={true} | |
selectedBucket={e3Bucket} | |
/> | |
), | |
options: { | |
flags: { objMultiCluster: false, objectStorageGen2: { enabled: true } }, | |
}, | |
}); | |
const rateLimitTable = getByTestId('bucket-rate-limit-table'); | |
const { findByTestId } = renderWithTheme( | |
<BucketDetailsDrawer | |
onClose={mockOnClose} | |
open={true} | |
selectedBucket={e3Bucket} | |
/>, | |
{ | |
flags: { objMultiCluster: false, objectStorageGen2: { enabled: true } }, | |
} | |
); | |
const rateLimitTable = await findByTestId('bucket-rate-limit-table'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for pointing this out! I've made the changes in 8974ea4
const { getByText } = renderWithThemeAndHookFormContext({ | ||
component: ( | ||
<BucketDetailsDrawer | ||
onClose={mockOnClose} | ||
open={true} | ||
selectedBucket={e1Bucket} | ||
/> | ||
), | ||
options: { | ||
flags: { objMultiCluster: false, objectStorageGen2: { enabled: true } }, | ||
}, | ||
}); | ||
expect( | ||
getByText( | ||
/This endpoint type supports up to 750 Requests Per Second \(RPS\)./ | ||
) | ||
).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { getByText } = renderWithThemeAndHookFormContext({ | |
component: ( | |
<BucketDetailsDrawer | |
onClose={mockOnClose} | |
open={true} | |
selectedBucket={e1Bucket} | |
/> | |
), | |
options: { | |
flags: { objMultiCluster: false, objectStorageGen2: { enabled: true } }, | |
}, | |
}); | |
expect( | |
getByText( | |
/This endpoint type supports up to 750 Requests Per Second \(RPS\)./ | |
) | |
).toBeInTheDocument(); | |
const { findByText } = renderWithTheme( | |
<BucketDetailsDrawer | |
onClose={mockOnClose} | |
open={true} | |
selectedBucket={e1Bucket} | |
/>, | |
{ | |
flags: { objMultiCluster: false, objectStorageGen2: { enabled: true } }, | |
} | |
); | |
expect( | |
await findByText( | |
/This endpoint type supports up to 750 Requests Per Second \(RPS\)./ | |
) | |
).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making requested changes! Approved pending CI passes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh understood. Sounds good!
…10862) * upcoming:[M3-8485] - Browsing Buckets & Objects - Unit Tests * test: Unit test for 'Endpoint Type' column in the Bucket Landing Table * test: [M3-8485] - updated tests for object storage bucket feature * test: [M3-8485] fixed failing unit test * test: [M3-8485] - udpated tests that need to wait for OBJGen2 flag
Description 📝
Unit tests for Bucket Landing page, Bucket Detail Drawer and Object Detail Drawers.
Changes 🔄
Target release date 🗓️
N/A
How to test 🧪
Verification steps
As an Author I have considered 🤔
Check all that apply