Test built extension wheels in CI #9420
Conversation
️✔️Azure CLI Extensions Breaking Change Test
|
|
Hi @DomAyre, |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances CI testing by adding validation of built extension wheels. Instead of only verifying that wheels can be built, it now installs built wheels and tests them with --help to catch import errors and missing files (like missing __init__.py).
Key changes:
- Modified
test_source_wheels()andtest_extension()to accept an optional wheel directory parameter - Added wheel installation and basic smoke testing after building wheels
- Changed execution flow to build wheels first, then test both source and built wheels
| check_output(['python', 'setup.py', 'bdist_wheel', '-q', '-d', built_whl_dir], cwd=s) | ||
| check_output(['azdev', 'extension', 'build', ext_name, '--dist-dir', built_whl_dir]) | ||
| except CalledProcessError as err: | ||
| raise("Unable to build extension {} : {}".format(s, err)) |
There was a problem hiding this comment.
The raise statement should instantiate an exception, not call it as a function. This should be raise RuntimeError(...) or similar exception class.
| raise("Unable to build extension {} : {}".format(s, err)) | |
| raise RuntimeError("Unable to build extension {} : {}".format(s, err)) |
scripts/ci/test_source.py
Outdated
| def test_source_wheels(): | ||
| if whl_dir is not None: | ||
| logger.info(f'installing extension wheel: {ext_name}') | ||
| wheel_path = next(whl_dir.glob(f"{ext_name}*.whl")) |
There was a problem hiding this comment.
The next() call will raise StopIteration if no matching wheel file is found. Add a default value or handle the exception to provide a more informative error message. Consider: wheel_path = next(whl_dir.glob(f\"{ext_name}*.whl\"), None) followed by a check.
| wheel_path = next(whl_dir.glob(f"{ext_name}*.whl")) | |
| wheel_path = next(whl_dir.glob(f"{ext_name}*.whl"), None) | |
| if wheel_path is None: | |
| logger.error(f"No wheel file found for extension '{ext_name}' in directory '{whl_dir}'.") | |
| raise FileNotFoundError(f"No wheel file found for extension '{ext_name}' in directory '{whl_dir}'.") |
scripts/ci/test_source.py
Outdated
| logger.info(f'installing extension wheel: {ext_name}') | ||
| wheel_path = next(whl_dir.glob(f"{ext_name}*.whl")) | ||
| subprocess.run( | ||
| f"az extension add -y -s {wheel_path}".split(" "), |
There was a problem hiding this comment.
Using .split(\" \") will incorrectly split paths that contain spaces. Use a list directly: [\"az\", \"extension\", \"add\", \"-y\", \"-s\", str(wheel_path)]
| f"az extension add -y -s {wheel_path}".split(" "), | |
| ["az", "extension", "add", "-y", "-s", str(wheel_path)], |
scripts/ci/test_source.py
Outdated
| check=True, | ||
| ) | ||
| subprocess.run( | ||
| f"az {ext_name} --help".split(" "), |
There was a problem hiding this comment.
Using .split(\" \") will incorrectly split extension names that contain spaces (if any exist). Use a list directly: [\"az\", ext_name, \"--help\"]
| f"az {ext_name} --help".split(" "), | |
| ["az", ext_name, "--help"], |
scripts/ci/test_source.py
Outdated
| stderr=subprocess.DEVNULL, | ||
| ) | ||
| subprocess.run( | ||
| f"az extension remove -n {ext_name}".split(" "), |
There was a problem hiding this comment.
Using .split(\" \") will incorrectly split extension names that contain spaces (if any exist). Use a list directly: [\"az\", \"extension\", \"remove\", \"-n\", ext_name]
| f"az extension remove -n {ext_name}".split(" "), | |
| ["az", "extension", "remove", "-n", ext_name], |
scripts/ci/test_source.py
Outdated
| if wheels_out_dir: | ||
| wheels_out_dirs = [ | ||
| os.environ.get('WHEELS_OUTPUT_DIR'), | ||
| str(whl_dir), |
There was a problem hiding this comment.
When whl_dir is None, str(whl_dir) converts it to the string \"None\" instead of keeping it as None. This bypasses the null check on line 130. Use str(whl_dir) if whl_dir else None instead.
| str(whl_dir), | |
| str(whl_dir) if whl_dir else None, |
3 participants
Why
This is a tiny step in addressing
As an extension developer, I authored a PR which broke my extension by introducing code which was missing an
__init__.pyfile which stopped it being included in the built extension package--with-containersflag to policy and fragment gen #9409This wasn't caught by CI because it doesn't run any testing on the built package wheel, only that a wheel can be built without errors.
Ideally all the tests we define as extension authors should run on the thing that will ultimately be released, but the way we have to run our testing through azdev makes this non trivial as azdev doesn't support installing extensions from source wheel.
So for now, we can do better than nothing by running the extension with
--helpwhich should at least catch broken imports like my case.How
ci/test_source.py::test_source_wheels()to optionally output the wheels to a provided dirci/test_source.py::test_extension()to optionally install built wheels and attempt to run that extensiontest_source_wheels()beforetest_extension()with a tempdir containing the wheelsThis checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)