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
Bug 1798581: Add tests for pvc creation with different options #3708
Bug 1798581: Add tests for pvc creation with different options #3708
Conversation
/retest |
frontend/packages/ceph-storage-plugin/integration-tests/tests/pvc.scenario.ts
Outdated
Show resolved
Hide resolved
frontend/packages/ceph-storage-plugin/integration-tests/views/pvc.view.ts
Outdated
Show resolved
Hide resolved
frontend/packages/ceph-storage-plugin/integration-tests/views/pvc.view.ts
Outdated
Show resolved
Hide resolved
frontend/packages/ceph-storage-plugin/integration-tests/views/pvc.view.ts
Outdated
Show resolved
Hide resolved
frontend/packages/ceph-storage-plugin/integration-tests/views/pvc.view.ts
Outdated
Show resolved
Hide resolved
987259e
to
0546534
Compare
frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/pvc.scenario.ts
Outdated
Show resolved
Hide resolved
|
||
describe('Test PVC creation with options.', () => { | ||
beforeAll(async () => { | ||
await browser.get(`${appHost}/`); |
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 a bit safer to await isLoaded()
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.
isLoaded() function doesn't work here, because the page being loaded as default is Overview Dashboard, so I'd need dashboardIsLoaded()
But actually I wouldn't want the test to wait for the dashboard to be loaded, it might fail on https://bugzilla.redhat.com/show_bug.cgi?id=1753666 or just make the test run longer unnecessarily.
I do check that Persistent Volume Claims page is loaded at the beginning of each test as part of PVC creation function.
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.
ack
frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/pvc.scenario.ts
Outdated
Show resolved
Hide resolved
frontend/packages/ceph-storage-plugin/integration-tests/views/pvc.view.ts
Outdated
Show resolved
Hide resolved
frontend/packages/ceph-storage-plugin/integration-tests/views/pvc.view.ts
Show resolved
Hide resolved
/retest |
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.
Only two comments, looks solid otherwise
frontend/packages/ceph-storage-plugin/integration-tests/views/pvc.view.ts
Outdated
Show resolved
Hide resolved
frontend/packages/ceph-storage-plugin/integration-tests/views/pvc.view.ts
Outdated
Show resolved
Hide resolved
Please run the linter as well as squash the commits. |
/lgtm |
/lgtm cancel |
/retest |
/test analyze |
frontend/packages/ceph-storage-plugin/integration-tests/views/pvc.view.ts
Outdated
Show resolved
Hide resolved
frontend/packages/ceph-storage-plugin/integration-tests/tests/pvc.scenario.ts
Outdated
Show resolved
Hide resolved
true, | ||
); | ||
await goToPersistentVolumeClaims(); | ||
await crudView.resourceRowsPresent(); |
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.
None of the test uses expect to verify if test passes or not. Please convert the checks to expect statements.
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.
fixed
await inputPVCSize.sendKeys(newPvcSize); | ||
await crudView.saveChangesBtn.click(); | ||
if (waitForBinding === true) | ||
await browser.wait(until.textToBePresentInElement(pvcStatus, 'Bound')); |
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've tested this function in loop and there is problem because the first function expands storage section in the sidenav and then clicks persistent volume claim. The next time you run the function it collapse (instead of expand) the storage section and and hence doesn't find the persistent volume claim. I suggest to add a click on storage in the sidenav in the end of the function to prevent an error each time the function is running twice or more. Something like:
click(element(by.cssContainingText('a[data-component="pf-nav-expandable"]','Storage')));
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. This comment got lost somehow, I didn't see it until today. I'll fix it in my next PR, will it be ok?
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.
Sure Elana. Not urgent
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.
Could you please retry with the latest version of the function? I tried creating PVC a few times in a row, it worked correctly. I think one of the changes requested by Neha fixed this.
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 just want to know that we are on the same page. It is not running the whole test in a loop. This will work for sure. It is importing the createNewPersistentVolumeClaim to another test and running it in a loop to create few PVCs. I'll test it later on if we are talking about the same scenario but I've checked the code and nothing has changed about this.
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, I used createNewPersistentVolumeClaim a few times in a row inside a different test and couldn't reproduce the problem. Could you please show me your code?
/lgtm |
1eb3774
to
1bb8a30
Compare
/test e2e-gcp-console |
/assign @rhamilto |
Comment pointed out by @shyRozen can be addressed in a follow-up PR. |
/test e2e-gcp-console |
@ebondare: This pull request references Bugzilla bug 1798581, which is invalid:
Comment 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. |
/bugzilla refresh |
@rhrazdil: This pull request references Bugzilla bug 1798581, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 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. |
/approve |
@rawagner Please have a look. Needs your approval. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh, ebondare, rawagner, rhrazdil, shyRozen 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 |
@ebondare: All pull requests linked via external trackers have merged. Bugzilla bug 1798581 has been moved to the MODIFIED state. 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. |
No description provided.