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

Add e2e Playwright tests #69

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

neo-garaix
Copy link
Collaborator

Adding Playwright tests to the project

  • No we can test MapBuilder through Playwright
  • Added Cats project directly in source files in order to have 2 different source of layers
  • Updated test README to indicate how to run e2e tests

@Gustry
Copy link
Member

Gustry commented Jan 29, 2025

I think this PR should be merged the last one, after #61 #60 #59 #58

Copy link
Member

@Gustry Gustry left a comment

Choose a reason for hiding this comment

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

Quick review for now

I haven't reviewed deeply, just a review

* Performs the addition of the Cats project to the map
* @param {Page} page The page object
*/
export async function addCatProjectToMap(page) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export async function addCatProjectToMap(page) {
export async function addCatProjectToMap(page) {

I think you should remove this function, and add by default the folder in the docker conf INI file

See 3liz/lizmap-web-client#5227 for instance.

Adding a folder is/must be a test in LWC core. It will speed up your tests here, as you do not need to log in as admin and "manually" add your folder in the admin panel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, grouped answer here

await page.getByRole('checkbox', {name: 'No base map'}).click();
}

const locator = page.locator('id=jforms_mapBuilderAdmin_config_baseLayerDefault');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const locator = page.locator('id=jforms_mapBuilderAdmin_config_baseLayerDefault');
const locator = page.locator('#jforms_mapBuilderAdmin_config_baseLayerDefault');

Is-it not better ? (if yes, search and replace all other occurences) L21 ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I helped myself with other Playwright test but they are equivalent. I'm still going to change them with more explicit ones like #...

@@ -0,0 +1,95 @@
import { test, expect } from '@playwright/test';
Copy link
Member

Choose a reason for hiding this comment

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

Can you look to TS check, like in LWC core, // @ts-check ?
And all in spec.js.


test('Open/Close attribute table', async ({ page }) => {

await page.locator('button.dispayDataButton').click();
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary, but I would be nice to use POM https://playwright.dev/docs/pom
To design the interface in the test model page.

* Performs the removal of the Cats project from the map
* @param {Page} page The page object
*/
export async function removeCatProjectToMap(page) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to test this ? into the module.

Removing a projet should be kept in LWC core I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It wasn't a test but a way to "clean" tests.
In mapBuilder we insert projects onto the "builder map" by adding them through the administration panel but I wasn't sure if I should add it completely ( like the demo project already in ) or add it like this.
I will definitely add it to the git project so we can test the module with two projects already loaded in the map.
But I will check to get a lighter project for testing purposes.

@@ -0,0 +1,5 @@
## Cats
Copy link
Member

Choose a reason for hiding this comment

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

I know the cat project is nice ;-)
But for testing, I'm not a big fan of adding this 4MB GPKG tests/lizmap/instances/cats/data_cats/cats.gpkg and a few tests/lizmap/instances/cats/media/data_cats/NAME.jpg.

GPKG is a binary files, with a few dis-avantages in the GIS world (lock file etc, not nice with git...)

Keep in mind for later about "test data", even if it's nice for testing to have some cats ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will remove it and find another lighter project which will fit easier for testing purposes.

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.

2 participants