-
Notifications
You must be signed in to change notification settings - Fork 30
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
Write e2e tests for Compression #479
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh 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 |
5de8a80
to
75103e0
Compare
c2fd79c
to
84c4783
Compare
cypress/support.ts
Outdated
// wait for the pod CR to be registered | ||
// eslint-disable-next-line cypress/no-unnecessary-waiting | ||
cy.wait(2500); | ||
cy.exec( | ||
`oc wait --for=condition=ready pod -l "app=rook-ceph-tools" -n openshift-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.
can we use "commandPoll" here with "oc get pod -l 'app=rook-ceph-tools' ..." ??
cy.wait can cause extra waiting (if pod gets created before 2500 ms) or can cause flake (if take more than 2500 ms)...
using "commandPoll" we do have more network calls, but IMO it is okay as we are not affecting performance of our product, just adding few more network calls in our CI...
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.
or if "commandPoll" won't work,
can we have some utility function similar to "commandPoll", which will continuously poll for a resource to check if it gets created or not instead of explicitly using cy.wait ??
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.
we are not waiting for pod to get created. we are waiting for the CR to get registered as mentioned in the comment.
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.
isn't CR will get created if it is registered ?
so if CR exists it means it got created/registered...
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.
There is a difference between a pod getting created which involves multiple steps such as container creation, image pulling, volume mounting etc. versus just creation of a pod CR. Currently the cy.wait
is only to wait for the CR to be registered in the kube-apiserver once that does we are waiting using the oc wait
command for the pod to actually come up(get ready); this approach is better than commandPoll
as commandPoll
continuously pings the api-server(similar to ddos attack) instead of that we are simply adding a watcher(internally supported by k8s) it will also not fail after 30 retries as compared to commandPoll
.
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.
so what I meant was, check for CR created or not... status of CR can still be creating/running etc... anything...
if CR exists, it ensures that it is registered as well... so instead of cy.wait (and always waiting for 2500 ms for it to registered) we can use some function to do this job similar to "commandPoll" (not necessarily same function)... that will ensure that resource exists (not worry about status) before moving forwards will further...
cypress/tests/compression.spec.ts
Outdated
// Allow job CR to be registered | ||
// eslint-disable-next-line cypress/no-unnecessary-waiting | ||
cy.wait(2500); |
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.
can we have some utility function like "commandPoll", which will continuously poll for a resource to check if it gets created or not instead of explicitly using cy.wait ??
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.
As mentioned above we are not waiting for a resource to get created but for the CR to be registered. we could reduce it to 1000ms(1s) that should also work.
84c4783
to
f661102
Compare
cypress/views/block-pool.ts
Outdated
cy.byLegacyTestID('horizontal-link-Storage Systems').click(); | ||
cy.byTestID('horizontal-link-Storage Systems').click(); |
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.
on my local, I can still see attribute as "data-test-id" only. Same is true for other tests as well, they are using "byLegacyTestID", just confirming if it is intentional change or a miss ?
cypress/views/block-pool.ts
Outdated
@@ -20,7 +20,7 @@ export const poolMessage: { | |||
|
|||
export const navigateToBlockPool = () => { | |||
ODFCommon.visitStorageDashboard(); | |||
cy.byLegacyTestID('horizontal-link-Storage Systems').click(); | |||
cy.byTestID('horizontal-link-Storage Systems').click(); | |||
cy.byLegacyTestID('item-filter').type('ocs-storagecluster-storagesystem'); | |||
cy.byTestRows('resource-row').get('td a').first().click(); | |||
cy.byLegacyTestID('horizontal-link-BlockPools').click(); |
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 is "data-test" instead...
f661102
to
1f09ca1
Compare
1f09ca1
to
44bbd19
Compare
471ce95
to
927e888
Compare
/test odf-console-e2e-aws |
5 similar comments
/test odf-console-e2e-aws |
/test odf-console-e2e-aws |
/test odf-console-e2e-aws |
/test odf-console-e2e-aws |
/test odf-console-e2e-aws |
927e888
to
7f8d469
Compare
/test odf-console-e2e-aws |
1 similar comment
/test odf-console-e2e-aws |
7f8d469
to
c281a9a
Compare
/test odf-console-e2e-aws |
4 similar comments
/test odf-console-e2e-aws |
/test odf-console-e2e-aws |
/test odf-console-e2e-aws |
/test odf-console-e2e-aws |
c281a9a
to
32ba4e5
Compare
/test odf-console-e2e-aws |
1 similar comment
/test odf-console-e2e-aws |
/retest all |
@bipuladh: The
Use 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. |
/test all |
Signed-off-by: Bipul Adhikari <[email protected]>
9f8d980
to
edd482e
Compare
@bipuladh: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Signed-off-by: Bipul Adhikari [email protected]