ci: build the e2e virtualenv once per worker and re-enable test-vscode-e2e - #6098
Open
tripleaceme wants to merge 3 commits into
Open
tripleaceme wants to merge 3 commits into
tripleaceme wants to merge 3 commits into
Conversation
Eleven end-to-end tests fail on the 60 second per-test timeout. Each of them creates a virtual environment, installs sqlmesh into it from source and then waits for the language server, all inside the test's own budget. Measured on a 4 core machine that fixed work costs about 44 seconds: 18 for the install and a further 25 for the first import of sqlmesh, which is where Python compiles the dependency tree into its bytecode cache. Move the environment into a worker-scoped fixture with a timeout of its own, so that it is built and warmed once per worker rather than once per test. The fixture keeps the environment in a temporary directory of its own, because the per-test directory is removed after every test. The tests whose virtual environment is itself the subject of the test keep building their own. venv_naming.spec.ts asserts on the directory name, the tcloud tests install a mock tcloud package that must not leak into the rest of the worker, and bad_setup.spec.ts deliberately installs without the lsp extra. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
Turns the job back on with the same path filter the unit job uses, so Python-only pull requests are not gated on it. code-server is pinned rather than installed from latest. That was not the cause of the failures - the render selector broke identically on 4.107.1, the newest release at the time the job was disabled - but an unpinned editor means the next UI change lands as a mystery failure rather than as a visible version bump. The per-test timeout goes from 60 to 180 seconds. The first test a worker runs pays for the editor and extension host starting up, which measured at 2.5-4 minutes locally against 21-39 seconds for every later test in the same worker, so 60 seconds only ever fit the latter. 180 is chosen against measurements taken on a machine roughly twice as slow as the runner, and is the number most worth revisiting once there is real CI data. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #6071. Builds the test virtualenv once per worker, as agreed on the issue, and turns
test-vscode-e2eback on.Build the virtualenv once per worker
fixtures.tsgains a worker-scopedsharedPythonEnvironmentalongside the existingsharedCodeServer. It creates its own directory, installssqlmesh[lsp,bigquery]pluscustom_materializationsonce, and warms the import. It is notauto, so only tests that ask for it pay for it.Measured cost that is now paid once per worker rather than once per test:
uv venvuv pip install -e repo[lsp,bigquery]+ custom materializationsimport sqlmesh.lsp.mainThat second figure was not in the diagnosis I posted on the issue, and it is the larger half — Python compiling the dependency tree into
__pycache__, inside the LSP handshake the tests wait on. Sharing the virtualenv without warming the import barely moved the needle, so the warm-up is doing as much work here as the install.Converted: all 3 in
configuration.spec.ts, and the 2 environment-injection tests inpython_env.spec.ts.Deliberately not converted, because their own virtualenv is the thing under test:
venv_naming.spec.ts(the venv's name is the assertion),tcloud.spec.tsandpython_env.spec.ts's tcloud cases (they install a mocktcloud, which in a shared environment would leak into every later test in the worker), andbad_setup.spec.ts(installs without thelspextra on purpose).Re-enable the job
Same path filter as the unit job, so Python-only pull requests are not gated on it.
code-serveris pinned instead of installed from latest. That was not the cause of the failures — the render selector broke identically on 4.107.1, the newest release when the job was disabled — but leaving it unpinned means the next editor UI change arrives as a mystery failure rather than a visible version bump.The timeout, and where I would push back on my own change
The per-test timeout goes from 60s to 180s, and I want to be straight that this is the weakest part of the PR.
Removing ~44s of fixed cost was not enough on its own:
At 240s after the change, 9 passed, 0 failed. The per-test durations show why: the first test a worker runs costs 2.5–4.0 minutes; every later test in that worker costs 21–39 seconds.
configuration.spec.tstests 2 and 3 went from timing out to 29–31s, which is the fixture working exactly as intended. The residual is the editor and extension host starting cold on the worker's first page load, which no virtualenv fixture removes.180s is chosen against measurements from a machine roughly twice as slow as
ubuntu-2204-8, and under desktop load. It is the number most worth revisiting once there is real CI data — the first green run on the runner is the only measurement that really counts here, and I would rather set it from that than from my laptop.One behaviour worth knowing: a test timeout restarts the worker, which destroys the worker fixture and rebuilds the virtualenv for the next test. One slow test therefore makes a whole file look broken. That is an argument for
retriesstaying on.Test Plan
All numbers above are macOS on a 4-core laptop against code-server 4.137.0 at
--workers=2 --retries=0. Nothing here has been run on Linux or on the actual runner.Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO