test: add BasePage testing util #343

DanLeshinsky · 2023-12-25T12:46:23Z

Description

this pull request suggests using POM to make our test suite more structured, by adding a BasePage for common actions among all automation tests.

close #297

github-actions · 2023-12-25T12:48:37Z

Preview: https://s3.amazonaws.com/noam-gaash.co.il/7332763046/open-bus/2a01092d15c75bb73f7c9876452ae8c66ffa0a26/index.html
Preview Storybook: https://s3.amazonaws.com/noam-gaash.co.il/7332763046/open-bus/2a01092d15c75bb73f7c9876452ae8c66ffa0a26/storybook/index.html

DanLeshinsky · 2023-12-25T13:17:25Z

Adding BasePage for automation tests.

DanLeshinsky · 2023-12-25T13:18:24Z

I added BasePage for automation tests.

shootermv

as far as i can see here -
this BasePage is a kind of util you plan to be used for a tests at this project,
am i right?

NoamGaash · 2023-12-25T13:31:43Z

Hi, thanks for your contribution!
I see you're having some failing commit statuses. please consider taking a look at this section:
https://github.com/hasadna/open-bus-map-search/blob/main/CONTRIBUTION.md#why-do-i-get-a-red-x-commit-status

DanLeshinsky · 2023-12-25T13:33:46Z

as far as i can see here - this BasePage is a kind of util you plan to be used for a tests at this project, am i right?

Yes, indeed this is the aim of BasePage

DanLeshinsky · 2023-12-25T13:34:54Z

Hi, thanks for your contribution! I see you're having some failing commit statuses. please consider taking a look at this section: https://github.com/hasadna/open-bus-map-search/blob/main/CONTRIBUTION.md#why-do-i-get-a-red-x-commit-status

Thanks, I'll try a lit bit later to fix it.

shootermv · 2023-12-25T13:40:24Z

@DanLeshinsky i will be glad to talk with you on a our discord channel may be i can be useful here

NoamGaash

Despite POM is listed as official Playwright tutorial, I believe it may become an antipattern when dealing with small projects such as this one. I'm afraid it will scare newcomers, and I'd like to make it simple-stupid as possible to let this project grow.
Nevertheless, if it will make some of our tests cleaner, we definitely should consider that. therefore I believe it would be fair to ask for some kind of POC that demonstrate how it will makes at least one of our tests more maintainable before merging it into our main development branch. WDYT?

src/test_pages/BasePage.ts

DanLeshinsky · 2023-12-25T21:56:16Z

Despite POM is listed as official Playwright tutorial, I believe it may become an antipattern when dealing with small projects such as this one. I'm afraid it will scare newcomers, and I'd like to make it simple-stupid as possible to let this project grow. Nevertheless, if it will make some of our tests cleaner, we definitely should consider that. therefore I believe it would be fair to ask for some kind of POC that demonstrate how it will makes at least one of our tests more maintainable before merging it into our main development branch. WDYT?

Of course!!! It would be great to demonstrate it! Which way can I do it? Video/Zoom is good one?

NoamGaash · 2023-12-26T07:11:30Z

Of course!!! It would be great to demonstrate it! Which way can I do it? Video/Zoom is good one?

Code demonstration, by creating tests that will actually use this functionality.

shootermv · 2023-12-26T08:56:36Z

POM is listed as official Playwright

can you post a link to tutorial pls?

DanLeshinsky · 2023-12-26T10:55:46Z

POM is listed as official Playwright

can you post a link to tutorial pls?

This one:
https://playwright.dev/docs/pom

DanLeshinsky · 2023-12-26T11:21:16Z

Of course!!! It would be great to demonstrate it! Which way can I do it? Video/Zoom is good one?

Code demonstration, by creating tests that will actually use this functionality.

To be honest, I already wrote tests that use BasePage for timeline, but separated it into two PRs. Do you want me to create the second PR to take a look or combine all the files into one PR? Locally it's running correct using the POM.

NoamGaash · 2023-12-26T13:53:33Z

@DanLeshinsky feel free to submit this other branch. consider using add/BasePage_297_1_2 as the target branch for your pull request

…_1_2

test: add Timeline tests

src/test_pages/BasePage.ts

NoamGaash · 2023-12-27T08:08:40Z

src/test_pages/TimelinePage.ts

+  }
+
+  public async closeLineNumber() {
+    await this.clickOnElement(this.close_line_number)


don't you agree that

Suggested change

await this.clickOnElement(this.close_line_number)

await this.close_line_number.click()

will be simpler?

additionally, using page.locator("span[aria-label='close']").click() let the user immediately see and understand which selector is used. using this.close_line_number is better in terms of reusability, but since we don't reuse this thing in many places, I think it's a little הכנה למזגן at this stage

I can't agree with that because inside this.clickOnElement it's wrapped with test.step that can help with debugging, let's leave it as it.

additionally, using page.locator("span[aria-label='close']").click() let the user immediately see and understand which selector is used. using this.close_line_number is better in terms of reusability, but since we don't reuse this thing in many places, I think it's a little הכנה למזגן at this stage

This is the difference between somebody who is not working as an Automation developer but knows all different aspects of it and even can write automation by itself. I'll explain why I tell this )) Writing tests it's like writing a story. If you want to learn the system, learn it from automation tests. And when somebody new look on your test he should understand on the spot what this locator means (because it takes second to press ctrl + mouth click). When you fix your own test, you should know immediately what this locator means, but not start searching the css or xpath or whatever in browser. And as well your code is more organized like this. All locators in the same place, all functions and getters in other.

I don't understand your comment.
I don't "knows all different aspects of it", and so do you.
I prefer simple solutions rather than smart ones.
We removed Yarn recently, not because it wasn't the right solution for the project. We did it because it's the right solution for us.
We have many junior contributors, and I don't find the complexity justified in this case.

As you see in the ticket #296 I've planned dozens of tests to be done on this page, and I hope the functionality allowing to plan more additional tests. I hope the solution of POM is not the to smart ones, but the needed one. But of course I will be more than happy if junior contributors will learn to understand the difference between the main idea of using it or not.

NoamGaash

Let's give it a shot, maybe we'll all learn something :)

add/Timeline_Tests_296_Tests_1_2

c2d3feb

DanLeshinsky requested a review from NoamGaash as a code owner December 25, 2023 12:46

DanLeshinsky requested a review from ArkadiK94 December 25, 2023 12:46

DanLeshinsky closed this Dec 25, 2023

DanLeshinsky reopened this Dec 25, 2023

add/BasePage_297_1_2

f956459

DanLeshinsky force-pushed the add/BasePage_297_1_2 branch from decbbc3 to f956459 Compare December 25, 2023 13:20

DanLeshinsky removed request for NoamGaash and ArkadiK94 December 25, 2023 13:26

shootermv reviewed Dec 25, 2023

View reviewed changes

NoamGaash requested changes Dec 25, 2023

View reviewed changes

src/test_pages/BasePage.ts Outdated Show resolved Hide resolved

src/test_pages/BasePage.ts Outdated Show resolved Hide resolved

src/test_pages/BasePage.ts Outdated Show resolved Hide resolved

DanLeshinsky changed the title ~~add/BasePage_297_1_2~~ tests: adding BasePage Dec 25, 2023

NoamGaash changed the title ~~tests: adding BasePage~~ test: adding BasePage Dec 25, 2023

NoamGaash changed the title ~~test: adding BasePage~~ test: add BasePage Dec 25, 2023

NoamGaash added ci improvements to our CI pipeline clarification needed labels Dec 25, 2023

shootermv changed the title ~~test: add BasePage~~ test: add BasePage testing util Dec 26, 2023

DanLeshinsky added 6 commits December 26, 2023 22:09

BasePage with TimelinePage and Tests Rejects

5c0733f

Merge branch 'add/BasePage_297_1_2' into add/Timeline_Tests_296_Tests…

b32ce56

…_1_2

Update BasePage.ts

89948d6

Update timeline.spec.ts

ea9f9b1

Merge pull request #349 from hasadna/add/Timeline_Tests_296_Tests_1_2

4e5532e

test: add Timeline tests

Merge branch 'main' into add/BasePage_297_1_2

1c7a442

DanLeshinsky commented Dec 26, 2023

View reviewed changes

src/test_pages/BasePage.ts Show resolved Hide resolved

DanLeshinsky requested review from NoamGaash and shootermv December 26, 2023 22:12

NoamGaash reviewed Dec 27, 2023

View reviewed changes

NoamGaash removed the clarification needed label Dec 27, 2023

DanLeshinsky requested a review from NoamGaash December 27, 2023 08:24

NoamGaash approved these changes Jan 8, 2024

View reviewed changes

DanLeshinsky merged commit 10ee688 into main Jan 9, 2024
16 checks passed

DanLeshinsky deleted the add/BasePage_297_1_2 branch January 9, 2024 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add BasePage testing util #343

test: add BasePage testing util #343

DanLeshinsky commented Dec 25, 2023 •

edited by NoamGaash

Loading

github-actions bot commented Dec 25, 2023 •

edited

Loading

DanLeshinsky commented Dec 25, 2023

DanLeshinsky commented Dec 25, 2023

shootermv left a comment

NoamGaash commented Dec 25, 2023

DanLeshinsky commented Dec 25, 2023

DanLeshinsky commented Dec 25, 2023

shootermv commented Dec 25, 2023 •

edited

Loading

NoamGaash left a comment

DanLeshinsky commented Dec 25, 2023

NoamGaash commented Dec 26, 2023 •

edited

Loading

shootermv commented Dec 26, 2023

DanLeshinsky commented Dec 26, 2023 •

edited

Loading

DanLeshinsky commented Dec 26, 2023

NoamGaash commented Dec 26, 2023

NoamGaash Dec 27, 2023

NoamGaash Dec 27, 2023

DanLeshinsky Dec 27, 2023

DanLeshinsky Dec 27, 2023

NoamGaash Dec 29, 2023

DanLeshinsky Jan 4, 2024

NoamGaash left a comment

	await this.clickOnElement(this.close_line_number)
	await this.close_line_number.click()

test: add BasePage testing util #343

test: add BasePage testing util #343

Conversation

DanLeshinsky commented Dec 25, 2023 • edited by NoamGaash Loading

Description

github-actions bot commented Dec 25, 2023 • edited Loading

DanLeshinsky commented Dec 25, 2023

DanLeshinsky commented Dec 25, 2023

shootermv left a comment

Choose a reason for hiding this comment

NoamGaash commented Dec 25, 2023

DanLeshinsky commented Dec 25, 2023

DanLeshinsky commented Dec 25, 2023

shootermv commented Dec 25, 2023 • edited Loading

NoamGaash left a comment

Choose a reason for hiding this comment

DanLeshinsky commented Dec 25, 2023

NoamGaash commented Dec 26, 2023 • edited Loading

shootermv commented Dec 26, 2023

DanLeshinsky commented Dec 26, 2023 • edited Loading

DanLeshinsky commented Dec 26, 2023

NoamGaash commented Dec 26, 2023

NoamGaash Dec 27, 2023

Choose a reason for hiding this comment

NoamGaash Dec 27, 2023

Choose a reason for hiding this comment

DanLeshinsky Dec 27, 2023

Choose a reason for hiding this comment

DanLeshinsky Dec 27, 2023

Choose a reason for hiding this comment

NoamGaash Dec 29, 2023

Choose a reason for hiding this comment

DanLeshinsky Jan 4, 2024

Choose a reason for hiding this comment

NoamGaash left a comment

Choose a reason for hiding this comment

DanLeshinsky commented Dec 25, 2023 •

edited by NoamGaash

Loading

github-actions bot commented Dec 25, 2023 •

edited

Loading

shootermv commented Dec 25, 2023 •

edited

Loading

NoamGaash commented Dec 26, 2023 •

edited

Loading

DanLeshinsky commented Dec 26, 2023 •

edited

Loading