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 1872469: Port login tests from protractor to Cypress #6521
Bug 1872469: Port login tests from protractor to Cypress #6521
Conversation
@dtaylor113: This pull request references Bugzilla bug 1872469, 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. 3 validation(s) were run on this bug
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. |
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 need to update test-prow.sh
to run the Cypress login spec when login
is passed as a parameter. See
frontend/packages/integration-tests-cypress/tests/app/auth-multi-user-login.spec.ts
Outdated
Show resolved
Hide resolved
frontend/packages/integration-tests-cypress/tests/app/auth-multi-user-login.spec.ts
Outdated
Show resolved
Hide resolved
frontend/packages/integration-tests-cypress/tests/app/auth-multi-user-login.spec.ts
Outdated
Show resolved
Hide resolved
Hey, Dave. I'm glad we're porting these to Cypress, but we also want to make sure there isn't a real bug in logout that is causing the Protractor tests to fail. |
Maybe we'll be able to reproduce with the Cypress tests and get a video to help debug. |
yes, this PR is just another option and debugging tool, which is why it's a WIP :-) |
f3a96d4
to
2c45f76
Compare
Just verified protractor's login tests as well as these new Cypress login tests worked multiple times when run locally. |
2c45f76
to
b8c8d6a
Compare
@dtaylor113: This pull request references Bugzilla bug 1872469, which is valid. 3 validation(s) were run on this bug
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. |
6ca623b
to
67e51a6
Compare
@spadgett Added to bottom of
..and updated |
76e647e
to
1518bfa
Compare
54835de
to
141e118
Compare
141e118
to
4aa3da7
Compare
4aa3da7
to
84af560
Compare
@dtaylor113: This pull request references Bugzilla bug 1872469, which is valid. 3 validation(s) were run on this bug
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. |
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 @dtaylor113. Let's pull the unrelated changes into a separate PR. I don't want to block an important test fix from code comments on the other changes.
frontend/packages/integration-tests-cypress/reporter-config.json
Outdated
Show resolved
Hide resolved
config.env.BRIDGE_KUBEADMIN_PASSWORD = process.env.BRIDGE_KUBEADMIN_PASSWORD; | ||
config.env.appHost = `${process.env.BRIDGE_BASE_ADDRESS || 'http://localhost:9000'}${( |
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 looks like it's the same as baseUrl
. Can we have one config value tracking?
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.
wow, talk about cut & paste blinders :-) sorry, removed appHost
.
84af560
to
d23df91
Compare
@dtaylor113: This pull request references Bugzilla bug 1872469, which is valid. 3 validation(s) were run on this bug
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. |
d23df91
to
97d331b
Compare
frontend/packages/integration-tests-cypress/tests/app/auth-multiuser-login.spec.ts
Show resolved
Hide resolved
97d331b
to
d5aecdc
Compare
test-prow-e2e.sh
Outdated
elif [ "$SCENARIO" == "login" ]; then | ||
./test-cypress.sh 'tests/app/auth-multiuser-login.spec.ts' | ||
else | ||
./test-cypress.sh $SCENARIO |
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 looks wrong... We should not run any cypress tests for other scenarios. You can remove this last else
.
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.
Hmm..I can change, but protractor will run with an unknown scenario.
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.
ok, updated
d5aecdc
to
2c73227
Compare
test-prow-e2e.sh
Outdated
elif [ "$SCENARIO" == "login" ]; then | ||
./test-cypress.sh 'tests/app/auth-multiuser-login.spec.ts' | ||
else | ||
echo "Cypress: unknown parameter: $1" |
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.
Don't echo here. This is expected if you're running a protractor suite. Just remove the else.
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.
Ok, removed
2c73227
to
52986b6
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.
/lgtm
Thanks @dtaylor113 !
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtaylor113, spadgett 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 |
@dtaylor113: All pull requests linked via external trackers have merged: Bugzilla bug 1872469 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. |
This PR removes most login tests from protractor and adds them to Cypress.