[Test]Refactor backward compatibility test #4906
Conversation
|
/quicktest-core |
|
/quicktest-core |
|
/quicktest-core |
|
/quicktest-core |
There was a problem hiding this comment.
I like the idea here! Much cleaner, thanks @zpoint!
Left some comments on the installation/setup.
Will backward compatibility testing still be included in quicktest-core?
Also, did you change any of the commands from the test, or just restructure it?
| GCLOUD_INSTALL_CMD = """ | ||
| wget --quiet https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-cloud-sdk-424.0.0-linux-x86_64.tar.gz && | ||
| tar xzf google-cloud-sdk-424.0.0-linux-x86_64.tar.gz && | ||
| rm -rf ~/google-cloud-sdk && | ||
| mv google-cloud-sdk ~/ && | ||
| ~/google-cloud-sdk/install.sh -q && | ||
| echo "source ~/google-cloud-sdk/path.bash.inc" >> ~/.bashrc && | ||
| . ~/google-cloud-sdk/path.bash.inc | ||
| """ | ||
| UV_INSTALL_CMD = 'curl -LsSf https://astral.sh/uv/install.sh | sh' | ||
|
|
||
| # Environment paths | ||
| BASE_ENV_DIR = pathlib.Path( | ||
| '~/sky-back-compat-base').expanduser().absolute() | ||
| CURRENT_ENV_DIR = pathlib.Path( | ||
| '~/sky-back-compat-current').expanduser().absolute() | ||
| BASE_SKY_DIR = pathlib.Path('~/sky-base').expanduser().absolute() | ||
| CURRENT_SKY_DIR = pathlib.Path('./').expanduser().absolute() | ||
| SKY_WHEEL_DIR = pathlib.Path(SKY_REMOTE_PATH).expanduser().absolute() |
There was a problem hiding this comment.
Can we avoid hardcoding the installation and specific paths? We still want people to be able to run it locally and we should let them do the tool installation, since they may want to do it another way (e.g. homebrew).
We can have a wrapper that runs on CI with the install commands and hardcoded paths.
There was a problem hiding this comment.
This is the original logic in the sh file.
Since CI already has gcloud installed, I can raise an exception if gcloud is not installed, if that's the concern.
| # Create and set up virtual environments using uv | ||
| for env_dir in [self.BASE_ENV_DIR, self.CURRENT_ENV_DIR]: | ||
| if not env_dir.exists(): | ||
| _run_cmd(f'~/.local/bin/uv venv --seed --python=3.9 {env_dir}',) |
There was a problem hiding this comment.
Can we just add uv to the PATH in the CI wrapper? A dev running locally may have uv installed somewhere else.
There was a problem hiding this comment.
Same for gcloud. I kept the install command from sh. Do we need to remove the installation of gcloud and sh?
We already have those set up on CI.
Wondering if it would be easier for users to run and install automatically.
Yes
No, just restructured it. 8 test functions map to the original 8 steps in the sh. |
|
Don't commit the changes yet. Need a reply to confirm if we should remove the |
|
I see. I didn't realize these installations were added - they are tracing back to the client server PR (#4660). In my opinion we should avoid it, but @Michaelvll should chime in.
sounds good |
zpoint
changed the title
|
/quicktest-core |
|
/quicktest-core |
|
Could u help review, thanks @Michaelvll |
| @@ -0,0 +1,288 @@ | |||
| import os | |||
There was a problem hiding this comment.
If we place this in the smoke test, what will happen if we trigger smoke test locally?
There was a problem hiding this comment.
This cannot be run with smoke tests in parallel locally, right? Otherwise, the wheel deletion will not work correctly ?
There was a problem hiding this comment.
Concurrency is limited to 1 in this line.
Buildkite uses multiple workers to allow concurrent runs, but running locally with concurrency set to 1 should be fine.
Otherwise, if we want to support local concurrency, we would need to use a locking mechanism (e.g., file lock) or create 8 separate environments, which would make the test too complicated.
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orconda deactivate; bash -i tests/backward_compatibility_tests.sh(local)