-
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
chore(test): added tests for multiple lxd server versions [WD-8616] #653
Conversation
Demo starting at https://lxd-ui-653.demos.haus |
e79994a
to
e0820a7
Compare
b752a68
to
8875114
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.
As just discussed in person: Let's combine the browser and lxd-backend verison as project, that is communicated to playwright. This way we can avoid passing the LXD_VERSION as environment variable and get it on the playwright report on top level.
Signed-off-by: Mason Hu <mason.hu@canonical.com>
8875114
to
e4dd0a6
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.
QA from the report looks good. One question for a possible simplification below.
"test-js": "vitest --run" | ||
"test-js": "vitest --run", | ||
"test-e2e-edge": "npx playwright test --project chromium:lxd-latest-edge firefox:lxd-latest-edge", | ||
"test-e2e-stable": "npx playwright test --project chromium:lxd-5.0-stable firefox:lxd-5.0-stable" |
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.
Nice additions here 👍
}; | ||
|
||
export const test = base.extend<TestOptions>({ | ||
lxdVersion: ["latest-edge", { option: true }], |
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 am confused as the value passed here seems to be hardcoded. Is the actual value coming from the use block in playwright config? Maybe we don't need the value here, then?
Though, please keep this lxd-test file, because I am going to need it for the coverage very soon -- even if it turns out to be empty and just passes the base test at this point.
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 value set here is the default for the project use
parameter. I referenced the playwright docs for parameterising projects. I think { option: true } is necessary but not sure if we can pass in a empty value for the first parameter of the array. Although passing in an empty value will look strange as well I rate, wdyt?
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 we need this definition here, I think keeping latest-edge
as a default is reasonable.
Maybe we should document the test setup, as it lately became a bit more complex. Could you add a future story to extend the architecture file?
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.
will do, I did add a bit of context in the HACKING.md, but I think adding to the architecture file is also necessary. Will create a task for it 👍
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, I like the solution :)
Done
QA