-
Notifications
You must be signed in to change notification settings - Fork 291
test(Playwright): Set up Playwright [WPB-17365] #19061
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
test(Playwright): Set up Playwright [WPB-17365] #19061
Conversation
5c3e71d
to
9fda508
Compare
playwright.config.ts
Outdated
// use: { ...devices['iPhone 12'] }, | ||
// }, | ||
|
||
/* Test against branded browsers. */ |
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.
Not sure which browsers we need, so I've commented our all except for chromium
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 can decide what browsers we need, maybe we can pick those we support.
Do you think we should configure it using env var?
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.
If you can configure it to use env var, I'd try for that; then you'd be able to set up tests using different browsers with tagging the ones which cover items important to try in other browsers
test/e2e_tests/backend/backend.ts
Outdated
|
||
import {getCredentials} from '../utils/credentialsReader'; | ||
|
||
const onePasswordItemName = 'BackendConnection staging-with-webapp-master'; |
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.
Looks like onePasswordItemName is gonna be used a lot in various tests?
It would be great to extract it as reusable const then
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.
Question: Does this method mean it'll prompt for 1Password interaction multiple times in a test run?
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.
Maybe I can read it one time per test suite run and store it in some data class. Would that 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.
Yeah and there's a few options for how to go about that, for example in Android and iOS we are creating an env file using 1Password's template capabilities
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.
Overall looks good, I really like the usage of page objects and data factories 🚀
Added few concerns.
test/e2e_tests/backend/backend.ts
Outdated
const axiosInstance = axios.create({ | ||
baseURL: BASE_URL, | ||
withCredentials: true, | ||
headers: { | ||
'Content-Type': 'application/json', | ||
}, | ||
}); |
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 move out this initialization?
I would prefer to initialize axiosInstance
once for each test session.
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.
Do you mean initialize BE client with axiosInstance
per each test method? Or did I misunderstand your implications?
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.
For each call you are initializing axiosInstance
, you don't need a new instance for each call. You can initialize only once and then you could use it everywhere.
test/e2e_tests/backend/backend.ts
Outdated
const deleteResponse = await axiosInstance.request({ | ||
url: '/self', | ||
method: 'DELETE', | ||
headers: { | ||
Authorization: `Bearer ${user.token}`, | ||
}, | ||
data: deletePayload, | ||
}); |
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 create a module that expose api helper functions (get, put .. etc).
Then we don't have to pass the token every time. We can also maintain the user data using singleton class.
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 elaborate of maintaining the user data using a singleton? I thought of just using ClientUser class instances with this user data. Could be useful if we have more than one user in test case.
test/e2e_tests/backend/clientUser.ts
Outdated
set token(value: string) { | ||
this.accessToken = value; | ||
} | ||
} |
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 would prefer to have object value factories (TS friendly) for mock data:
interface User {
email: string;
password: string;
firstName: string;
lastName: string;
username: string;
token: string;
}
export const getUser = (user: Partial<User> = {}) => ({
...user,
email: user?.email ?? faker.internet.email({lastName: user?.lastName, provider: 'wire.engineering'}).toLowerCase(),
password: user?.password ?? faker.internet.password({length: 8, pattern: /[A-Za-z\d!@#$]/}),
firstName: user?.firstName ?? faker.person.firstName(),
lastName: user?.lastName ?? faker.person.lastName(),
username: user?.username ?? `${user?.lastName}${faker.string.alpha({length: 5, casing: 'lower'})}`.toLowerCase(),
token: user?.token ?? null,
});
const testUser1 = getUser();
const testUser2 = getUser({email: '[email protected]'});
This will be more readable.
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 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.
+1. Builder or Factories for data like this tend to be a good idea long term.
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, initially I have created a constructor for different cases, but I'm not sure it will be used. So I've decided not to overcomplicate things in the beginning but probably it makes sense to have it from the get go.
test/e2e_tests/pages/login.page.ts
Outdated
this.loginErrorText = page.locator('[data-uie-name="error-message"]'); | ||
} | ||
|
||
async isEmailFieldVisible(): Promise<boolean> { |
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.
No return type needed.
async isEmailFieldVisible(): Promise<boolean> { | |
async isEmailFieldVisible() { |
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.
No return type needed.
@e-maad but it does return a boolean and this fits with the page object model design. Why wouldn't a return type be needed?
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.
You don't need to explicitly define return type, in TS it should be auto handled based on the returned value.
playwright.config.ts
Outdated
// use: { ...devices['iPhone 12'] }, | ||
// }, | ||
|
||
/* Test against branded browsers. */ |
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.
If you can configure it to use env var, I'd try for that; then you'd be able to set up tests using different browsers with tagging the ones which cover items important to try in other browsers
test/e2e_tests/README.md
Outdated
|
||
# Tests | ||
|
||
E2E tests (login.spec.ts and credentialsReader.spec.ts) can be found inside [test folder](/test/e2e_tests/). The folder contains [page objects](/test/e2e_tests/pages), [backend classes](/test/e2e_tests/backend), and [credentialsReader.ts](/test//e2e_tests/utils/credentialsReader.ts) for access 1Password credentials. |
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.
Small item: Given the e2e tests are going to grow beyond those two, should their names be removed from this item?
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, I've just tried to explain what was done in this PR. I'll rewrite it better! Thx for the input
test/e2e_tests/backend/backend.ts
Outdated
|
||
import {getCredentials} from '../utils/credentialsReader'; | ||
|
||
const onePasswordItemName = 'BackendConnection staging-with-webapp-master'; |
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.
Question: Does this method mean it'll prompt for 1Password interaction multiple times in a test run?
test/e2e_tests/backend/backend.ts
Outdated
throw new Error('zuid cookie not found in register response'); | ||
} | ||
|
||
// 2. Get activation code |
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.
Getting activation/verification code is something which is gonna be needed separately once we get to automating 2FA scenarios again. While not necessary for this PR, consider extracting this part
test/e2e_tests/backend/clientUser.ts
Outdated
set token(value: string) { | ||
this.accessToken = value; | ||
} | ||
} |
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.
+1. Builder or Factories for data like this tend to be a good idea long term.
test/e2e_tests/pages/login.page.ts
Outdated
this.loginErrorText = page.locator('[data-uie-name="error-message"]'); | ||
} | ||
|
||
async isEmailFieldVisible(): Promise<boolean> { |
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.
No return type needed.
@e-maad but it does return a boolean and this fits with the page object model design. Why wouldn't a return type be needed?
… to .env file from template
…r via test fixture
…b.com:wireapp/wire-webapp into feat/WPB-17365_implement_playwright_framework
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 - the new changes look very nice!
}, | ||
); | ||
} | ||
} |
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.
Great work 👏
For the future use I would be prefer to not put all the api calls in the single class.
For example:
export class UserRepository extends Backend {
...
}
export class AuthRepository extends Backend {
...
}
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.
Yep, that's the plan! Thank you :)
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 🚀
test/e2e_tests/backend/user.ts
Outdated
...user, | ||
email: user.email ?? faker.internet.email({lastName, provider: 'wire.engineering'}).toLowerCase(), | ||
password: user.password ?? faker.internet.password({length: 8, pattern: /[A-Za-z\d!@#$]/}), | ||
firstName, |
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.
No need for a variable.
firstName, | |
firstName: user.firstName ?? faker.person.firstName(), |
|
Description
Refer to the epic: https://wearezeta.atlassian.net/browse/WPB-17365
Checklist