Repository navigation
Conversation
fbaf8a4 to
97e0c1d
Compare
|
@LHBruneton-C2C Green ! API's and urls are now mocked up and we do not use external services. |
tonio
left a comment
There was a problem hiding this comment.
Thanks for this, removing the dependency on external hosts (local file server + the stack's geor:public_layer) is a big improvement over what's on main, and the CI is easy to debug (direct then gateway health checks, logs and traces uploaded only on failure). The encoding cases (misnamed .cst, Latin-9 œ) are exactly the kind of regression e2e tests should catch.
Some points before merging:
1. Binary test data in git history. e2e-tests/files/donnees-2023-reg02.parquet is 38 MB (about 46 MB for all fixtures). Deleting it later won't remove it from history. It reads more like a load test than a functional one, and a-epci2025.parquet already covers parquet. Could we drop it, or generate it in a fixture, or download it only in a nightly job? Could you also add the source and license of the datasets (MEL, data.gouv.fr) to the README?
2. Scope. This PR also contains two functional changes unrelated to tests:
run_in_threadpoolinstaging.py: a good fix (it also helps with the Airflow executor, which does a blocking HTTP call inside anasync defhandler), but it deserves its own PR or commit.- Rejecting multi-layer GeoPackages: a user-facing behavior change that should be agreed on separately (see the inline comment).
Could you split these into separate commits or PRs and squash the rest? "vibe coded e2e tests" and the two "add e2e ci" commits shouldn't land as-is. The PR description is also empty.
3. CI cost. The job takes about 15 min and runs on every PR to main, docs and Dependabot bumps included. Please add a paths: filter (apps/**, libs/**, e2e-tests/**, docker/**, the workflow file), or run it on a label, nightly or on workflow_dispatch.
4. Tooling. apps/frontend/ still has a Cypress setup (4 npm deps, test:e2e* scripts, a commented-out job in frontend-checks.yml) whose only test checks a "Lorem ipsum" placeholder. With this PR, Playwright (Python) becomes the de facto e2e framework. I think that's the right choice, since these tests mostly check pipeline output (WFS hits, coordinates, encodings) and fit the Python/uv/pytest stack. We should remove the dead Cypress setup in a follow-up and record the decision in AGENTS.md, so we don't end up with two frameworks.
5. Coverage that silently doesn't run in CI. The latest run shows 2 skipped:
test_import_csv_lat_lon_2154is skipped because the defaultPROJECTIONShas no EPSG:2154.- The credential tests are skipped because the secrets are not set (and are never available for fork PRs anyway).
Please fix the first (inline comment), and state explicitly that FTP and Basic Auth are not covered in CI.
e7761cc to
8345b2a
Compare
Needs #221