Playwright #18927
Playwright #18927
Conversation
AdrienClairembault
force-pushed
the
playwright
branch
8 times, most recently
from
5c5bc1a to
48aafcd
Compare
|
Don't worry about sonar cloud failure, it report a new OS command execution but it should be safe given its just a call to glpi's console:
|
Iv'e marked it as safe |
AdrienClairembault
force-pushed
the
playwright
branch
from
479b84b to
3a81949
Compare
|
Not a fan of using TypeScript. That's both a personal thing and also a worry that it makes writing E2E tests less desirable for the developers that already don't work with JavaScript a lot. Also, working with TypeScript is incredibly frustrating in PHPStorm. There have been too many times I've Ctrl + Click a JS function related to a library to try and see the code for it, only to be directed to the useless .d.ts file for it. Same issue started happening when TS files were being added to the Cypress tests. I don't see the benefit of it at all, and only downsides. I voiced my opinion about it before, but that was when we only had a .d.ts file added for Cypress commands all to fix VSCode behavior (which I also don't like but I know others are using it and it was an OK solution to fix autocomplete). |
There was a problem hiding this comment.
Couldn't this be handled during the installation of GLPI like we handled with Cypress?
We should have a PHP script that handles test environment data and then include it during the installation when the env is testing.
There was a problem hiding this comment.
That actually what we do not want to have anymore.
It isn't convenient because you can't run the test locally unless you are in the right environnement.
There was a problem hiding this comment.
As an additional note, maybe we could force the environnement to switch to testing when running e2e tests.
|
|
||
| public async getToken() | ||
| { | ||
| // TODO: try to reuse a single csrf token per worker if it is possible. |
There was a problem hiding this comment.
Stealing CSRF tokens from unrelated pages sounds very weird and something we shouldn't be doing. Actions should be actual form submissions when needed or use AJAX which already handles automatically sending a CSRF token.
There was a problem hiding this comment.
This is the same code from cypress migrated to playwright (nothing new here).
AJAX does not automatically send a CRSF token in this context as this code is not executed on a GLPI's page.
|
|
||
| test('can remove tiles', async ({ page }) => { | ||
| // Make sure the tile exist before trying to delete it | ||
| await expect(config_page.getTile("Request a service")).toBeVisible(); |
There was a problem hiding this comment.
Not a fan that we trade command queuing for "await hell". At the very least, we should be careful to not use await for every single line of playwright test code. Should use Promise.all when possible to run things together at least.
There was a problem hiding this comment.
IMO (and most online opinions from what I can tell) await is correct modern way of using javacript while cypress command queue is an un-intuitive nightmare that lead to callback hell.
| throw new Error(`Unknown profile: ${profile}`); | ||
| } | ||
|
|
||
| await this.request.post( |
There was a problem hiding this comment.
Should be AJAX rather than a fake form submit?
There was a problem hiding this comment.
This is an AJAX POST request, not sure what mean here.
If by "fake subit" you refer to the CSRF token, it is required since we changed this endpoint to accept only POST queries on GLPI's 11.
It is the recommended way to setup playwright. Here is an extract form the documentation:
|
Description
Setup playwright and migrate two tests cypress tests files.
It does seems faster locally, the 2 migrated tests take ~15 seconds with 6 threads on my machine (vs 28 seconds for their cypress equivalent on the CI).
On the CI, the improvement seems less impressive (unsure why, it even seems slower).
Maybe there isn't enough tests to properly compare.
Anyway, I've saved a trace of its execution to investigate it next week (the work can still be reviewed).
Regarding the tests themselves, they feel nicer to write.
Cypress was great at the start because it is easy to get started, however as you go on you face many issues when you try to do more complicated things due to its design choices (thenable everywhere that is not a real promise, command pattern, ...).
For that, playwright feels much better IMO.
It requires a bit more time to understand it at the start but after that it get quite smooth due to its design which is way more predictable.