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
Added tests for object service dashboard page #3555
Added tests for object service dashboard page #3555
Conversation
frontend/packages/noobaa-storage-plugin/integration-tests/tests/overviewDashboard.ts
Outdated
Show resolved
Hide resolved
let text = await obcCount.getText(); | ||
const initialCount = text.substring(0, 1); | ||
// Go to create form page | ||
await browser.get( |
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.
Aren't we supposed to click on "Create OBC" button and expect to be redirected to this link, instead of just redirecting to the URL?
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 but will be following a different approach, will use kubectl
to create obc.
frontend/packages/noobaa-storage-plugin/integration-tests/tests/overviewDashboard.ts
Outdated
Show resolved
Hide resolved
|
||
it('Checks whether MCG is healthy', async () => { | ||
await browser.get(`${appHost}/dashboards/object-service`); | ||
expect(clusterHealth.isPresent()).toBeFalsy(); |
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.
Why are we expecting it to be false?
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.
because there is no message for healthy status only for unhealthy and error states.
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.
Please add a comment to the code.
frontend/packages/noobaa-storage-plugin/integration-tests/tests/overviewDashboard.ts
Outdated
Show resolved
Hide resolved
frontend/packages/noobaa-storage-plugin/integration-tests/tests/overviewDashboard.ts
Outdated
Show resolved
Hide resolved
6c2fd14
to
687d550
Compare
It seems that you have added tests for all cards here. Its better to have separate |
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.
I believe we also need to add Snapshot tests (RFE) to ensure that the Pages load in the structure we expect it to have.
Otherwise, selection like below might break without any traces
export const obcCount = $(
'#content-scrollable > div:nth-child(2) > div > div > div > div > div:nth-child(1) > div > div:nth-child(3) > article > div.pf-c-card__body.co-dashboard-card__body > div:nth-child(2) > div.nb-buckets-card__row-title > div:nth-child(1)',
);
frontend/packages/noobaa-storage-plugin/integration-tests/tests/overviewDashboard.scenario.ts
Outdated
Show resolved
Hide resolved
beforeAll(async () => { | ||
await browser.get(`${appHost}/dashboards/object-service`); | ||
}); | ||
it('Creates an Object Bucket Claim and test equality', async () => { |
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.
it('Creates an Object Bucket Claim and test equality', async () => { | |
it('Creates an Object Bucket Claim and tests equality', async () => { |
frontend/packages/noobaa-storage-plugin/integration-tests/tests/overviewDashboard.scenario.ts
Outdated
Show resolved
Hide resolved
Run linter to ensure uniform formatting. |
@umangachapagain this code has been linted. Hence the frontend tests are passing. |
But there are some inconsistent new line issues between line 25 and 26, and 28 and 29. |
687d550
to
1a8e1d8
Compare
1a8e1d8
to
def25a3
Compare
/assign @rhrazdil |
def25a3
to
dd06b0e
Compare
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.
Changes to the component itself should not be a part of this PR. Or, atleast should be in a separate commit with a little explanation on why it was done. (purely based on the processes that I have seen)
@umangachapagain agreed. IMO these changes will make more sense when they are accompanied by the use case. And the owners of those particular components might be able to give a better approach(possibly using a different tag). These changes aren't structural changes but are only the addition of identifier tags which would not change the layout/behaviour of these components and would not make sense to add without any pretext to add 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.
Suggested a few changes
frontend/packages/noobaa-storage-plugin/integration-tests/tests/overviewDashboard.scenario.ts
Outdated
Show resolved
Hide resolved
let text = await obcCount.getText(); | ||
const initialCount = text.substring(0, 1); | ||
await execSync(`echo '${JSON.stringify(testBucket)}' | kubectl create -n ${testName} -f -`); | ||
await browser.sleep(10000); |
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.
It would be much nicer to wait for some condition. You can take a look here how simple waiting method implementation can look:
export const waitForCount = (elementArrayFinder: ElementArrayFinder, targetCount: number) => { |
and here how it's used:
console/frontend/packages/kubevirt-plugin/integration-tests/tests/vm.actions.scenario.ts
Line 130 in 41bb8cb
await browser.wait(until.and(waitForCount(resourceRows, 0)), PAGE_LOAD_TIMEOUT_SECS); |
frontend/packages/noobaa-storage-plugin/integration-tests/tests/overviewDashboard.scenario.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,12 @@ | |||
import { $, element, by } from 'protractor'; | |||
|
|||
export const obcCount = element(by.css('div[aria-label="Object Bucket Claim count"]')); |
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.
$()
is an alias for element(by.css())
, so these can be simplified by just using $()
.
@bipuladh: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
A reason for closing and reopening would help here @bipuladh . |
/close |
@bipuladh: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/reopen |
@bipuladh: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
2c9a2ab
to
ed36fa9
Compare
6d5163a
to
f7d572f
Compare
}); | ||
|
||
it('Test at least one noobaa bucket is present', async () => { | ||
await browser.wait(until.and(untilNoLoadersPresent)); |
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.
Since this is already checked in beforeAll, why do you need to check it again here?
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.
Its present at other places too
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. For some reason, it fails without this.
@@ -0,0 +1,106 @@ | |||
import { execSync } from 'child_process'; |
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.
The file should be named to something like object-service-dashboard
instead of overviewDashboard
f7d572f
to
8e972a5
Compare
await browser.wait(until.and(untilNoLoadersPresent)); | ||
await browser.wait(until.visibilityOf(healthOfMCG)); | ||
const color = healthOfMCG.getAttribute('fill'); | ||
expect(color).toBe('#486b00'); |
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.
Wouldn't it be nicer to import the code for the green color from https://github.com/openshift/console/blob/master/frontend/packages/ceph-storage-plugin/integration-tests/utils/consts.ts ?
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.
Makes sense. Thanks.
8e972a5
to
7c598b9
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh, gnehapk, umangachapagain The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
Integration for Object Service Dashboard
Todo: