Fix unit test waring in dr brownfield #1112
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: TimothyAsirJeyasing The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@SanjalKatiyar, @GowthamShanmugam Please review |
TimothyAsirJeyasing
force-pushed
the
fix-test-case-warning
branch
from
8f167dd to
c247795
Compare
|
|
| HealthItem: ({ title }: { title: string }) => <div>{title}</div>, | ||
| ViewDocumentation: ({ text, doclink }: { text: string; doclink: string }) => ( | ||
| <a href={doclink}>{text}</a> | ||
| ), | ||
| HealthState: { | ||
| NOT_AVAILABLE: 'NOT_AVAILABLE', | ||
| OK: 'OK', | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock('../../../../utils/osd-migration'); |
There was a problem hiding this comment.
same here, Why the util function is mocked? if ceph status has all data, utils function will be executed without any issue.
There was a problem hiding this comment.
I have mocked the internal components HealthItem, HealthBody to isolate the unit test and focus on testing the specific behavior of the OSDMigrationProgress component as i received lot of issues without mocking them mainly with HealthItem and HealthBody.
There was a problem hiding this comment.
Ideally, HealthItem and HealthBody should not give any error, In case you are seeing many error, Then we need to check the severity of the error and it needs to be investigated.
| jest.mock('@odf/shared/dashboards/status-card/states', () => ({ | ||
| ...jest.requireActual('@odf/shared/dashboards/status-card/states'), | ||
| GrayUnknownIcon: ({ title }: { title: string }) => ( | ||
| <div title={title}>MockGrayUnknownIcon</div> | ||
| ), | ||
| })); |
There was a problem hiding this comment.
Why do we need to mock all of this?
| HealthItem: ({ title }: { title: string }) => <div>{title}</div>, | ||
| ViewDocumentation: ({ text, doclink }: { text: string; doclink: string }) => ( | ||
| <a href={doclink}>{text}</a> | ||
| ), |
There was a problem hiding this comment.
same here why HealthItem and viewDcoumentation has to be mocked ?
There was a problem hiding this comment.
I was facing issues with only HealthItem and HealthBody. when i debug this, i notice that it adds a nested <default> tag while rendering which causes an issue. So i have mocked them to isolate this issue..
There was a problem hiding this comment.
oh!, ok also face this issue sometime, ok then lets mock HealthItem
Signed-off-by: Timothy Asir Jeyasingh <[email protected]>
TimothyAsirJeyasing
force-pushed
the
fix-test-case-warning
branch
from
c247795 to
2dd036b
Compare
| @@ -39,11 +31,7 @@ describe('OSDMigrationStatus', () => { | |||
|
|
|||
| getOSDMigrationStatus.mockReturnValue('Completed'); | |||
There was a problem hiding this comment.
do not mock getOSDMigrationStatus... pass a dummy Ceph CR to it with all the required statuses and use except to check if function is returning correct value...
| @@ -68,11 +56,7 @@ describe('OSDMigrationStatus', () => { | |||
|
|
|||
| getOSDMigrationStatus.mockReturnValue('In Progress'); | |||
There was a problem hiding this comment.
u have mentioned here In Progress, but test title it is saying PENDING...
|
|
||
| describe('OSDMigrationStatus', () => { | ||
| describe('OSDMigrationProgress', () => { |
There was a problem hiding this comment.
add more cases, for each status that CR reports, not just "In progress" and "Completed"...
|
|
||
| describe('OSDMigrationStatus', () => { | ||
| describe('OSDMigrationProgress', () => { |
There was a problem hiding this comment.
add case for calculateOSDMigration function as well, by passing dummy Ceph CR to it.. it should return expected result...
|
|
||
| describe('OSDMigrationStatus', () => { | ||
| describe('OSDMigrationProgress', () => { |
There was a problem hiding this comment.
add checks for doc links in ur test cases...
No description provided.