Improve testing #321
Comments
Regarding to "Add SSR to coverage"I report the comment from issue #8:
It's not so easy to disable ssr in our case, because we are using the What we need is a way to attach istanbul instrumentation to the node process that is starting the server, in playwright.config.js fractal-web/playwright.config.js Line 41 in 9ffa473 A similar use case is described here: istanbuljs/nyc#520, where they would like to compute code coverage for manual testing. They are suggesting 2 possible tools, but both have some problems:
Istanbul documentation says that the command |
|
As briefly mentioned in the discussion in #324, we may want to include some benchmarks (AKA testing for performance regressions) in our test suites. To be clear: I don't think it makes sense to have it as a broadly-scoped feature of our test suite, nor I think it's a priority. |
|
A quick comment (based on a quick look at main...more-tests#diff-cb5fff2cfb1045f7d933c94ceef3671d2407b3f0a5abd691197979c6f00f1817R18): great to have tests for task collection! |
Thanks! I've removed |
|
It seems that when playwright creates 2 projects concurrently (in different tests) a SQL constraint fails:
Complete error: Any idea about this? I didn't try to reproduce it outside the tests yet, but I've already encountered it multiple times. I'm setting up a test that uses a "create project" fixture and it seems to clash with the create/delete project test. The 2 tests pass when I run them separately. |
We can look into it together, if it's useful, but for the moment I have no idea of why this is happening. Some very preliminary thoughts:
|
|
Something more that should be tested at some point is that resource order is somewhat predictable, both in POST and PATCH API calls. Ref: |
|
I've collected some logs of other failures. I'm still not sure about what is the exact sequence to reproduce this, but it seems that reading, creating and deleting projects concurrently causes it. It could be caused by SQLite (I'll do some tests with PostgreSQL) or by something that should be handled using transaction in the backend. In a highly concurrent environment errors can happen also during a simple select (project listing): But with the delete+insert something very bad can happen (since it leaves the database in a broken state and manual fix on the table is needed): After this the database is in an inconsistent state and it is not possible to create projects anymore: Inside the database I have the following: In this state, when I try to create a project its id would be |
|
Quick comment again: as of fractal-server 0.3.13, your own PR for using WAL in sqlite is part of fractal-server. I don't know if this matters at all. |
|
I'll also try without WAL. Here is another one happening during dataset resource creation: |
Just happened with 0.3.12 |
|
@tcompa I'm not able to reproduce these errors using PostgreSQL, so I assume it's caused by SQLite. Do you see any problem in configuring the CI pipeline to run with PostgreSQL? |
That's good to know!
Short answer: go ahead! Still, I'd like to understand what goes wrong at some point. We do test the sqlite use case in fractal-server, but apparently this test suite is somewhat different. |
I think it's only because Playwright executes a lot of calls in parallel and SQLite is not the right tool for these cases: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html#database-locking-behavior-concurrency
|
|
Just to have it clear (and only if doesn't take too much effort to run a check): This is only as a clarification, but running tests with postgres is perfectly fine.. |
In case it's useful, this is how we handle this choice in fractal-server GitHub actions: https://github.com/fractal-analytics-platform/fractal-server/blob/main/.github/workflows/ci.yml. We run both sqlite and postgresql tests, and for postgresql we include the corresponding service in the yaml file. |
|
I confirm that tests are successful with SQLite when disabling parallelism. At the moment the difference in the execution time is not very big, but I assume it will increase when we will add more tests:
Consider that currently most of the time is spent by the initial tasks collection (19.5s). |
|
I'm not sure about the impact on the pipeline total execution time. Even if tests are faster we will spend more time in starting the postgres service and installing the additional dependencies. Maybe for the moment we can just continue with SQLite and parallelism disabled. What do you think? |
Let's keep it like this for the moment, and switch to postgresql once the number of tests grows. |
Concerning this (low priority!) aspect, an option would be to use this approach: https://github.com/fractal-analytics-platform/fractal-server/blob/main/.github/workflows/benchmarks.yaml#L53-L60. |
|
Before that we have to fix our current coverage setup, that unfortunately it doesn't seem to work correctly. It's some time that I'm looking at it and it has 2 problems:
For example, this is a result from our last PR: It says that the coverage is 11.11%, however running only the unit tests the coverage is at 77%. If you notice at the uncovered lines (the last column) it says that lines I would like to try with a different code coverage tool (e.g. you can use |
|
Also notice that this seems to depend by Svelte compiler: sveltejs/svelte#7824 And according to the user that opened the issue there might be no solution for this at the moment:
|
Considerations about coverageFor JavaScript there are 2 possible coverage tools: v8 (the JavaScript engine, that provides some native coverage features) and istanbul (a library that instrument the code before running the tests). While istanbul produces a human readable result, v8 itself simply generates a coverage file in JSON format, but is not able to generate a human readable coverage report. For that tools like c8 or v8-to-istanbul are used. Unit testsIn the past vitest used So, in order to update vitest from 0.32.4 to 1.x.x I've updated Svelte from 3.58.0 to 4.x.x and Svelte Kit from 1.15.7 to 2.x.x. Then I configured the tests to run with v8. The 2 tools compute the coverage in different ways and v8 shows many more hits for statements, while branches results seems to be slightly more similar. In any case the results are not very reliable. Here is a coverage computed using v8:
Here is the same using istanbul:
In general istanbul output looks more useful, since v8 applies a Playwright coveragePlaywright coverage can track only the code that runs on the browser (so ssr coverage should be taken in a different way). Our current Playwright coverage setup is based on this article, that uses istanbul. It produces very weird results, showing line numbers that simply don't exist. Official Playwright documentation suggests a different setup, using v8 and supporting only Chromium based browsers. I tried with that but the results are useless in the same way. I'm not sure about what's going on exactly, however, looking at the generated files I suspect that there is something wrong with the source map paths and there are some svelte kit issues about source maps. Moreover, a standard raw It seems that someone is building its own Playwright Coverage tool for Svelte, but is a WIP and I wasn't able to make it working: https://github.com/jill64/svelte-playwright-coverage Backend coverageTechnically, it should be possible to generate a coverage of the backend code with v8 starting the server in this way: The application needs to exit gracefully on Ctrl+C, otherwise the coverage process is terminated too. So we need to add the following lines to fractal-web, somewhere: The coverage report is built, however both
There are some tools, like https://github.com/jvilk/merge-source-maps, that are able to merge a map of map, but is not clear how to apply this to the coverage reporter when using a raw v8 report. Moreover, it seems that there are source map problems also for the ssr part: sveltejs/kit#9608 |
|
Thanks a lot @zonia3000 for this deep exploration! As we already discussed this morning, let's put coverage-fixing on hold for a while, because of how complex it is. But this detailed log will be useful in the future, if we try once more to get it right (hopefully after some of the existing tools also improve). |
|
@zonia3000 I have built a tool that can generate coverage reports directly from v8 coverage data. It can also be integrated with testing frameworks like Playwright, Vitest, Jest, etc., but it hasn't been tested with svelte too much. You could give it a try when you have the chance. |
|
After reviewing this issue, I'd say that what is still relevant is only:
(while there is no current need of working on the coverage) |
Now done with #484 |
#320 implements a first complete version of the testing framework: unit tests (vitest), end-to-end tests (playwright), global coverage, and appropriate github actions.
Let's list here placeholders of future updates, and then move to specific issues when necessary.
A few first thoughts:
mainsomewhere, so that we can view it from the repo page (and not just from the PRs)The text was updated successfully, but these errors were encountered: