-
Notifications
You must be signed in to change notification settings - Fork 1
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
Configure E2E testing package #1493
Conversation
- Includes a very basic test
I guess this was needed
- By default schedule actions use default branch
- Fixture is parameterized so it can be reused
- This may change later
@@ -0,0 +1,16 @@ | |||
# Editor configuration, see https://editorconfig.org |
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.
Should we use prettier like all our other packages?
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 copied this from portal frontend, so does it need to be updated as well?
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, I have no idea what this file does.
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 configures the code style for editors. It's set up the same for portal and ALCS frontends, not for services. Perhaps it is part of Angular scaffolding?
For now I'll leave it, but I'll install prettier as a dev dep and include .prettierrc
as well.
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.
Added prettier and eslint
e2e/playwright.config.ts
Outdated
@@ -35,16 +36,27 @@ export default defineConfig({ | |||
{ | |||
name: 'chromium', | |||
use: { ...devices['Desktop Chrome'] }, | |||
testIgnore: '**', |
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.
Is this temporary? Whats the long term plan 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.
This is a base project, so is never run by itself. It just includes Chrome for use in other projects.
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.
After re-investigating this, I don't think this works for multiple browsers. I'll come up with a better solution.
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.
Project dependencies did not work how I thought. I've changed the way this works. Now, projects only select browser, and frontend is selected via directory. Each top-level directory will have its own fixtures file that sets the base URL via the environment (this will allow us to modify the URL for dev/test/local as needed).
- Frontend now selected via directory - Fixtures now per-frontend - Base URL is set in frontend's fixtures file
e2e/package.json
Outdated
@@ -18,7 +18,17 @@ | |||
"url": "https://github.com/bcgov/alcs/issues" | |||
}, | |||
"homepage": "https://github.com/bcgov/alcs#readme", | |||
"dependencies": { | |||
"@playwright/test": "^1.32.0", | |||
"@types/node": "^20.11.24", |
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.
nitpick: This doesn't matter since this won't get shipped but types should go in devDependencies
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 be. I considered it as a main dependency since running the E2E tests is what this package does. I think you could make the case either way.
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 transpiled code will be JS not TS, meaning the types won't be used.
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.
Good point. Changed.
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 👍
inboxLoggedIn: Page; | ||
} | ||
|
||
export const test = base.extend<FixtureOptions & Fixtures>({ |
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.
Ideally the name of fixture should represent the action that it performs, in this case it is login
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 does, on line 18: inboxLoggedIn
. The way fixtures work in Playwright is that you extend the main test function with an object that contains the fixtures.
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.
👍
template.env
and instructions to setup local secrets