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.
There was a problem hiding this comment.
I prefer to have a separate command/script/whatever to be sure that we do not have to embed testing data in the base GLPI installation logic. Also, having this logic in the tools directory is a good option to be sure that it will not be embed in the release archive.
There was a problem hiding this comment.
As an additional note, maybe we could force the environnement to switch to
testingwhen running e2e tests.
This is what we do for the web test suite, but the impossibility to use a DB transaction sometimes cause issues requires to have a specific cleaning logic executed after the tests are executed.
There was a problem hiding this comment.
Yes, as discussed together if we want to force a specific env then it should be a 3rd database reserved for e2e tests like symfony does.
But this isn't the priority right now I think.
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.
No reason not to use the docker containers provided and the convenient run_tests script or push to a PR for the CI to run them.
There was a problem hiding this comment.
Actually, there are still something like 50% of the GLPI regular devs that are not using docker at all and want to be able to run the tests locally.
There was a problem hiding this comment.
The script is not perfect and lack configuration options to run a single test for example.
Also the cost of spinning up the containers make it painful for repeatedly running a single test when doing something like test driven development.
I think it might also complicate things like xdebug integration, which might be critical in some cases.
Overall, we are mostly speaking from experience here.
Indeed, we've had feedback from others members of the team that they were often confused when trying to run the tests for the first time (often to debug one of their pull request where a single test failed).
Some members even defaulted to using the testing environnement by default for their work to avoid dealing with the setup issues, so clearly the previous solution was not a good fit for everyone.
With the new behavior, it should be "plug and play" no matter what you are using (local env, docker script, ...) and it avoid putting environnement specific code into the installation script which is not a great pattern.
There was a problem hiding this comment.
The script has support for specifying scopes, but I know it wasn't handled for all the test types.
I added an interactive mode a while ago to prevent the need to keep recreating the containers for every test run.
XDebug is indeed annoying, but I think it was explicitly disabled in the docker image. Maybe it could be re-enabled and have the XDebug env variable optionally set in the test runner to avoid always having that performance hit.
Having the test commands that existed before the local docker environment in the composer file added to the confusion.
From personal experience, running tests locally without docker is not reliable at all. Several times I've had tests fail that way when there was nothing wrong with the code, but instead the tests failed because of some extra data/configuration I had in my developer instance. The server-side tests expect to be run in a clean environment which isn't possible if you just run them directly on your local GLPI. Of course JS and Lint tests at least are fine to run outside Docker.
The only time I run tests directly are for specific E2E tests that I know don't rely on global configuration stuff. I just manually added the E2E entity and user to my development DB. This isn't required for me though. I just think being able to run Cypress not in headless mode can be useful for understanding issues in certain cases. I'm definitely not an expert, but couldn't it also be configured to launch the GUI from the Docker container?
|
|
||
| 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.
There was a problem hiding this comment.
The tests present in the Playwright documentation are using await for all asynchronous commands. IMHO, even if it requires to use await on every command, it does not make the tests less readable. I even think it is more readable than multi level of promises callbacks embed in each other.
| 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.
There was a problem hiding this comment.
Why is it not just a $.ajax call? We already inject the X-Glpi-Csrf-Token header with these calls.
If this is how it was done in Cypress (and the taking CSRF tokens from another page), I probably missed the change in a review or I wasn't looking at Cypress changes at the time.
Our CSRF implementation is weird. Idk why there are per-form tokens and per-page reusable token which can all be used for whatever we want on any page.
There was a problem hiding this comment.
Glpi's $.ajax load a CSRF token by extracting a token from the current page (parsing the DOM, same as we are doing here).
It works for GLPI because there is always by definition a page already loaded.
From the context of the tests you may not be on a page yet and thus this process need to be independent.
This is not some GLPI's javascript code attached to a page, this is a dedicated process running on a nodejs script.
There was a problem hiding this comment.
OK. I understand now. I don't have any idea to do it different right now, so it seems OK as it is.
It is the recommended way to setup playwright. Here is an extract form the documentation:
|
You can add a |
I'll restore the tests but unsure about the reporters. |
Whatever the reporter used, we need to be able to compare metrics. The |
|
Regarding general performances feedback, here is a quick overview on a test that I've migrated this morning. I've made care to migrate it as close as possible to its cypress version (even if it could be optimized a bit more) to make sure it is a good comparison. Here are the two tests:
Running the test on cypress take 5s, with a total 12s execution time (cypress is very slow to initialize - but note that this mater less when running multiple tests as this initialization is only done once).
Comparing with playwright, we get a 3.5s execution time with a 11s total execution time. Note that the 11s total is "penalized" by the mandatory This extra setup takes a good 5s by itself (but we are still a bit faster overall for the total execution time, showcasing that playwright initialization is incredibly fast compared to cypress).
So over one single test, on my laptop, playwright seems faster (3.5s vs 5s). Note that an individual test might appear slower when running things in parallel, as the GLPI's server can get a bit overwhelmed.
This is explained because this test is executed at the very start of the process. This might seems a potential issue but it isn't because the parallelization gain are more important that this initial "speed bump".
Sadly, these parallelization gains are less effective on the CI because we only have 2 threads available. |
|
On a side note, there is also the option of running only a subset of the tests on the CI ("smoke tests") and run the whole suite only on the nightly action and before releases. This seems to be a very common practice, but this require the tests to not be flaky at all (which is not the state of the cypress suite, and too early to say for the playwright suite). |
I don't run tests locally unless I've already pushed to a PR and the CI reports issues, or I am working on specific tests. E2E tests take half an hour, so I push to let the hosted CI run them and switch to another task instead of sitting and waiting for a complete suite to run. |
|
More feedback on the CI execution time, it seems the 15 migrated tests take 1m05 on cypress for 58s on playwright. |
|
I updated the local docker scripts and ran both the Playwright and Cypress tests and then compared the times for all common tests. Cypress took 59.66 seconds while Playwright took 92.73 seconds. That made Playwright 55.4% slower than Cypress just on individual test execution. Rerunning Playwright tests a few times did not noticeably improve test execution time so it isn't likely to be a one-time performance issue. It also didn't seem to solve the reliability issues. Both Cypress and Playwright had tests outright fail all retries at least once while I was comparing and they had to be run a few times to get a full run without failures. None of the 2x slower tests on Playwright were tests that had to be retried. There is quite the difference between what I see locally and in CI for both performance and reliability so maybe I missed something when configuring things to run in the local docker environment? |
|
Do you have any screenshots of the results (how many threads were used ?) and maybe the changes to the tests scripts so I can try it on my side ? For the flakiness, note that if you run the full suite (cypress then playwright), some flakyness from cypress can "leak" into the playwrights tests. For example, if the display preference test from cypress fails, it will create a failure on playwright due to an unexpected "Pending reason" column being displayed:
|
This is something I suspect as well, I feel like it should be much quicker on the CI than the actual result (compared to what I get locally). |
|
Running with 2 thread on a local docker seems reliable on my side and comparable to the execution time of the github actions CI given than I am a laptop CPU (still a lot slower than running locally without docker tho...).
|
All I have is the ctf-report and an HTML report which don't show the threads used. I didn't touch the configuration, but instead just moved the commands run in the CI workflow to scripts like we did with the other test suites. I'm not even sure where the thread count is configured. Even with a single thread, I expected Playwright to be the same or faster than Cypress, at least once the initial setup was done.
To be fair to both frameworks and rule out me missing something with the test runner to handle interactive mode properly, I had the containers recreated every time but also I ran the initial Playwright tests first. I split the old E2E tests and Playwright tests into their own "suites" so running For the CLI output, all I see is: I published my changes to a branch on my fork for you to see what I did. |
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.