Skip to content
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

fix: executors e2e tests #913

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Conversation

mortada-codes
Copy link
Contributor

This PR...

Changes

Fixes

How to test it

screenshots

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@vercel
Copy link

vercel bot commented Oct 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
testkube-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 23, 2023 11:39am

@mortada-codes mortada-codes changed the title fix: delete executor container test fix: executors e2e tests Oct 19, 2023
@@ -10,4 +10,8 @@ export class ExecutorsPage {
public async openCreateExecutorDialog(): Promise<void> {
await this.page.click('xpath=//button//span[contains(text(),"Create a new executor")]'); // TODO: data-test
}

public async openExecutorSettings(executorName: string): Promise<void> {
await this.page.click(`xpath=//div/span[text()="${executorName}"]`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data-test attribute

@@ -39,9 +40,55 @@ test(`Create custom container executor`, async ({page}) => {
await api.removeExecutor(realExecutorName);
});

test.skip(`Custom container executor - general settings`, async ({page}) => {});
test(`Custom container executor - general settings`, async ({page}) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this case can be extended with setting custom command, and argument

await executorsPage.openExecutorSettings(realExecutorName);

const executorGeneralSettingsPage = new ExecutorGeneralSettingsPage(page);
const executorNameInput = await executorGeneralSettingsPage.getExecutorName();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually it's better to user "validators" instead of getting and then comparing the value.
The difference is in this case it's waiting for element matching the selector (input[id="general-settings-name-type_name"]), then getting it's value, and then comparing (expect assertion). Element will be checked once, so any state transitions can make the test unstable.

In case of validators it would be looking for the same element, but with specific text value.
So, executorGeneralSettingsPage.validateExecutorName(executorName):
and, all that's needed is checking if the element exists:

this.page.locator(`xpath=//input[@id="general-settings-name-type_name" and text="${executorName}"]`)

Additionally, on the test level I would use higher-level function instead - like executorGeneralSettingsPage.validateExecutorDetails that would then contain validateExecutorName on the Page level. The main goal is to keep tests high-level, and keep all of the details on the lower (Page) level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants