-
Notifications
You must be signed in to change notification settings - Fork 274
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(tests): Create rootless machine e2e verfication #7446
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Tamara Babalova <[email protected]>
Signed-off-by: Tamara Babalova <[email protected]>
.getByRole('checkbox', { name: 'Machine with root privileges' }) | ||
.locator('..'); | ||
this.userModeNetworkingCheckbox = this.page.getByRole('checkbox', { | ||
name: 'User mode networking (traffic relayed by a user process). See [documentation](https://docs.podman.io/en/latest/markdown/podman-machine-init.1.html#user-mode-networking).', |
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.
anything in the parenthesis can be omitted since it is gonna work with just first part of the string. This would work if exact
is not set to true.
readonly closeButton: Locator; | ||
readonly createMachineButton: Locator; | ||
|
||
constructor(page: Page) { |
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.
On general it is fine, just I would like you and Tibor to look at podman-onboarding-page.ts
which has the same parts as this new page object model. With this new page object, the onboarding could make use of 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.
@ScrewTSW ^^
this.createMachineButton = this.page.getByRole('button', { name: 'Create' }); | ||
} | ||
|
||
async createMachine(machineName: string, isRootless: boolean): Promise<ResourcesPage> { |
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 make isRootless = false
default value, since it is the default.
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.
and change the name to rootful, and make it true by default.
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 could also incorporate other params like userModeNetworking and imagePath? Also start the Machine? So we have covered all three sliders.
As of cpu/memory/disk size. On WSL, user cannot really change these, but it might change with hyperv. Just to note that. no need to change it now.
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 thought about extending the method to the other parameters as well but then I would have to add the functionality as well and not sure if it's within this issue for now as it's always extendable in future if needed.
await playExpect(this.rootPriviledgesCheckbox).toBeVisible(); | ||
const upperElement = this.rootPriviledgesCheckbox.locator('..'); | ||
const clickableCheckbox = upperElement.getByText('Enable'); | ||
await clickableCheckbox.click(); |
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 also add playExpect(checkbox).toBeEnabled
? To check it was enabled?
await playExpect(this.podmanMachineConfiguration).toBeVisible(); | ||
await this.machineNameBox.fill(machineName); | ||
|
||
if (!isRootless) { |
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 also always make sure that the boolean switch is set/unset by the value you pass into function? so, if would be removed, and there would be a logic that will set rootful/rootless by the value passed. Unless it already is set to a correct value
|
||
this.machineNameBox = this.podmanMachineConfiguration.getByRole('textbox', { name: 'Name' }); | ||
this.imagePathBox = this.page.getByRole('textbox', { name: 'Image Path (Optional) ' }); | ||
this.browseImagesButton = this.page.getByRole('button', { name: 'button-Image Path (Optional)' }); |
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.
Align the locators with what is in the above mentioned onboarding page.
const successfulCreationMessage = this.page.getByText('Successful operation'); | ||
const goBackToResourcesButton = this.page.getByRole('button', { name: 'Go back to resources' }); | ||
|
||
await playExpect(successfulCreationMessage).toBeVisible({ timeout: 100000 }); |
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 say that we need to improve the handling of what might happen once you click on Create button.
we might got into what is here in case of success. What if there is an error? We should fail the test and throw that error that might appear in the form, right? Maybe also open log?
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.
Also, if there will be existing machine, this new one gonna get a dialogs regarding switching of the default connection. So it is going to be important to consider that aspect as well.
}); | ||
|
||
afterAll(async () => { | ||
await deletePodmanMachine(page, PODMAN_MACHINE_NAME); |
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 truly curious how this test is going to work with a test env. in mind. Like we have usually podman machine already present. This means that the connection could be switched. But at the test end, we need to make sure switch it back again.
This test would need some parameters to operate the workflow in a right way. and to fix the test among the rest.
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.
checkboxes are not handled in a uniform manner, any reason ?
Yes, I wanted to access the parent element but forgot I did it in the definition already, I'll change it for consistency. |
What does this PR do?
Implements test verification of creation of a rootless machine in Podman Desktop.
Screenshot / video of UI
What issues does this PR fix or reference?
#4665
How to test this PR?
npm run test:e2e