-
Notifications
You must be signed in to change notification settings - Fork 99
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
[Outreachy Task Submission] Fix irregularities for e2e testing in Localhost vs Live deployment for login.cy.js #1003
Conversation
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 irregularities noted cause issues when running the tests locally. However the tests as set up run correctly(on the CI - which is where we need them to run correctly). Suggested changes in this PR will therefore not be approved for merging.
An update to the documentation will however be made to guide running the tests locally with minimal fuss.
Thanks.
} | ||
|
||
click_login_button() { | ||
cy.get(LoginLocators.loginButton).click(); | ||
cy.wait(5000); |
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.
An explicit 5 second wait is way too long. Explicit waits (even as short as 1second) are generally bad practice and should be avoided always unless totally unavoidable.
Also this is unnecessary at this point. There have been no issues at this step running this locally. I will ask you remove this wait.
this.click_through_onboarding(); | ||
this.change_laguage(); | ||
// this.change_laguage(); |
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.
In the CI environment, the platform will always launch in non-english language. Why this happens, is unknown. That's why we have this step in the login process.
What to do, is disable this step to change the language when running the tests locally. :)
@@ -60,8 +59,8 @@ class LoginFunctions { | |||
} | |||
|
|||
verify_login() { | |||
cy.get(LoginLocators.loginButton).should('not.exist'); | |||
cy.get(LoginLocators.accountBtn).should('exist'); | |||
cy.get(LoginLocators.loginButton).should('exist'); |
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.
These changes don't look right.
On logging in, the loginButton should not exist, and the accountButton should exist.
Maybe we may need to change from existing to visibility instead i.e change should('not.exist'
) to should('be.visible')
instead.
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.
Actually, I thought that on verification of logging in, it should be the other way around.
Also, I did this because when I was running it on my local machine, there were inconsistencies when I ran it several different times.
Introduction
This fixes #4914. E2E testing for the
login.cy.js
started performing as expected for me in both Localhost and Live deployments.My approach
I tweaked the following things:
onboarding-button-greeting
not being found.change_language()
method because the language selector doesn't always appear in Localhost.env.json
file, and also for the CI/CD checks to not fail due to an absence of environment variables.How to test this PR
e2e-testing
folder.cypress.env.json
file, if not already so, change thebaseURL
tohttp://localhost:4200
.npm run start
in your command line.1-login
. You will notice a failure response.1-login
passes.Video
Here's a video I did after making these changes:
e2e.login.edit.-.Made.with.Clipchamp.mp4