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

Sub tab next cases #10

Open
wants to merge 8 commits into
base: 5.5
Choose a base branch
from
Open

Sub tab next cases #10

wants to merge 8 commits into from

Conversation

jstawow
Copy link

@jstawow jstawow commented Oct 27, 2023

No description provided.

ADD_EMPLOYEE = 'Add Employee'
}

export enum Labels {

Choose a reason for hiding this comment

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

It's generally recommended to name enums in a singular way. So it would be Label instead of Labels. This reflects that each constant within the enum (like USERNAME, PASSWORD, LOGIN, etc.) represents a single, distinct label. This singular naming convention makes the code more intuitive and easier to understand, especially for other developers who might work with it in the future.

Copy link
Author

Choose a reason for hiding this comment

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

right when we use it lebel.specific_label then it is one lebel not more

return this.page.getByText(Labels.EDIT_POST);
}

protected getDletePostButton(

Choose a reason for hiding this comment

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

typo - Delete

Copy link
Author

Choose a reason for hiding this comment

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

done

}

public getCommentInput(
randomTitle: string

Choose a reason for hiding this comment

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

I would probably not name this randomTitle but just title. The place where you call this function does not use randomTitle. Even if it would be random from the callers perspective - still using "title" as name makes more sense. Because if you would just pass not a random title, then the parameters name would be more generic.

Copy link
Author

Choose a reason for hiding this comment

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

done

// public getPostWithRandomTitleWithoutPhoto(randomTitle: string): Locator {
// return this.page.locator(`.orangehrm-buzz-post-body:has(:text("${randomTitle}")) .orangehrm-buzz-post-body-text`);
// }
// this also works, let's keep it in codebase for poor times

Choose a reason for hiding this comment

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

What does poor times mean? 😄

Copy link
Author

@jstawow jstawow Nov 15, 2023

Choose a reason for hiding this comment

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

poor times means times when we do not know how to do something then we will have 2 soluation maybe in different code only 1 of them will work so i would keep it in code

): Locator {
return this.page
.getByText(randomNewEmployeeName)
.locator("xpath=../..")

Choose a reason for hiding this comment

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

How does this locator work?

Copy link
Author

Choose a reason for hiding this comment

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

In this particular case, ../.. is an XPath expression used to select the parent of the parent element. It's essentially asking Playwright to locate an element's grandparent. This could be useful when you want to select an element relative to its grandparent in the HTML structure.

firstRandomName: string,
bySubtub?: boolean | null
) {
if (bySubtub === true) {

Choose a reason for hiding this comment

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

You can just do: if (bySubtub)

Copy link
Author

Choose a reason for hiding this comment

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

ur right

await this.searchEmployeeButton.click()
public async addEmployeeWithLoginDetails(
firstRandomName: string,
bySubtub?: boolean | null
Copy link

@piotrw91 piotrw91 Nov 14, 2023

Choose a reason for hiding this comment

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

You created an union type here which is boolean | null - do you need null here?

Copy link
Author

Choose a reason for hiding this comment

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

ur right, null no needed

@jstawow jstawow requested a review from piotrw91 November 15, 2023 09:05
Copy link

@annatooploox annatooploox left a comment

Choose a reason for hiding this comment

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

good job! you definitely improved your code!

public async searchEmployeeById(employeeId:string) {
await this.employeeIdInput.type(employeeId)
await this.searchEmployeeButton.click()
public async addEmployeeWithLoginDetails(

Choose a reason for hiding this comment

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

"...LoginCredentails" instead of "LoginDetails"? More accurate, but you choice ;)

await expect(page.locator(tableCells[0])).toBeVisible();
await expect(page.locator(tableCells[1])).toBeVisible()
})
test("Admin user should edit previousely created employee with random name", async ({

Choose a reason for hiding this comment

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

I think the title is confusing. Why "random name"? I think you know already which employee you have been created, so it is not random anymore ;) I suggest to remove word "random"

Choose a reason for hiding this comment

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

and in general, for all method's name - why you always write get...byRANDOM...? why Random? I know that you generated ranom name, but it is confusing. Of course MINOR comment

Copy link

@piotrw91 piotrw91 left a comment

Choose a reason for hiding this comment

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

LGTM!

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