Skip to content

Conversation

@DomAyre
Copy link
Contributor

@DomAyre DomAyre commented Nov 14, 2025

Why

This repos CI uses azdev to run each extensions tests, however this means it's testing the local modules which might have breaking differences compared to what we ship to customers.

How

  • Add a new autouse pytest fixture (currently scoped to the confcom extension but designed to apply to all extensions) which builds wheels for each extension being tested, and swaps out the non-test modules

The modules actually running the tests are the only part not swapped for the built version to handle tests which rely on unshipped code/data


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
  • My extension version conforms to the Extension version schema

Note: This is strictly a testing only change, so no version bump or history update is required

@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Nov 14, 2025

️✔️Azure CLI Extensions Breaking Change Test
️✔️Non Breaking Changes

@azure-client-tools-bot-prd
Copy link

Hi @DomAyre,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

@yonzhan
Copy link
Collaborator

yonzhan commented Nov 14, 2025

Thank you for your contribution! We will review the pull request and get back to you soon.

@github-actions
Copy link

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).
After that please run the following commands to enable git hooks:

pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>

@github-actions
Copy link

@DomAyre DomAyre marked this pull request as ready for review November 17, 2025 14:53
Copilot AI review requested due to automatic review settings November 17, 2025 14:53
Copilot finished reviewing on behalf of DomAyre November 17, 2025 14:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new testing approach that runs tests against built extension wheels instead of local unbuilt code, ensuring tests validate what is actually shipped to customers.

Key Changes:

  • Added a session-scoped pytest fixture that builds wheels for extensions being tested
  • Implemented module swapping to replace non-test modules with wheel versions while preserving test modules that may rely on unshipped code

# tests pick up the wheel code over the unbuilt code
sys.path.insert(0, build_dir.glob("*.whl").__next__().as_posix())
for module in list(sys.modules.values()):
if extension in module.__name__ and module not in modules_to_test:
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable extension is referenced on line 73 outside the loop where it's defined (lines 44-67). This will only work correctly if there's exactly one extension in extensions_to_build. If there are multiple extensions, only modules from the last extension will be reloaded. Consider moving the module reloading logic inside the extension loop or storing all extension names to iterate over later.

Suggested change
if extension in module.__name__ and module not in modules_to_test:
if any(ext in module.__name__ for ext in extensions_to_build) and module not in modules_to_test:

Copilot uses AI. Check for mistakes.

# Add the wheel to the path and reload extension modules so the
# tests pick up the wheel code over the unbuilt code
sys.path.insert(0, build_dir.glob("*.whl").__next__().as_posix())
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic on line 71 uses __next__() which will raise StopIteration if no wheel files are found in build_dir. This could happen if the build process fails silently or if the wheel files have unexpected names. Consider adding error handling or validation to ensure at least one wheel was built successfully.

Suggested change
sys.path.insert(0, build_dir.glob("*.whl").__next__().as_posix())
wheel_files = list(build_dir.glob("*.whl"))
if not wheel_files:
raise RuntimeError(f"No wheel files found in {build_dir}. The build may have failed or produced unexpected file names.")
sys.path.insert(0, wheel_files[0].as_posix())

Copilot uses AI. Check for mistakes.
Comment on lines +36 to +38
while not any(parent.cmdline()[0].endswith(i) for i in ["bash", "sh"]):
parent = parent.parent()
RUN_ID = parent.children()[0].pid
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition on line 36 assumes the parent process command line will end with "bash" or "sh", which may not be true in all environments (e.g., zsh, fish, Windows, or when run from other tools). This could cause an infinite loop or crash if no such parent is found. Consider adding a maximum depth check or handling other shell types.

Suggested change
while not any(parent.cmdline()[0].endswith(i) for i in ["bash", "sh"]):
parent = parent.parent()
RUN_ID = parent.children()[0].pid
# Recognize common shell process names
shell_names = ["bash", "sh", "zsh", "fish", "cmd.exe", "powershell.exe"]
max_depth = 10
depth = 0
while True:
try:
cmdline = parent.cmdline()
if cmdline and any(cmdline[0].endswith(shell) for shell in shell_names):
break
parent = parent.parent()
depth += 1
if parent is None or depth >= max_depth:
# Could not find a shell parent, fallback to current process
parent = psutil.Process(os.getpid())
break
except (psutil.Error, IndexError):
# Fallback if process info is unavailable
parent = psutil.Process(os.getpid())
break
# Use the first child PID if available, else fallback to current PID
children = parent.children()
RUN_ID = children[0].pid if children else parent.pid

Copilot uses AI. Check for mistakes.

if not any(build_dir.glob(f"{extension_name}*.whl")):
subprocess.run(
["azdev", "extension", "build", extension.replace("azext_", ""), "--dist-dir", build_dir.as_posix()],
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 62 duplicates the string replacement extension.replace("azext_", "") which was already computed on line 46 as extension_name. Use the extension_name variable instead to avoid duplication.

Suggested change
["azdev", "extension", "build", extension.replace("azext_", ""), "--dist-dir", build_dir.as_posix()],
["azdev", "extension", "build", extension_name, "--dist-dir", build_dir.as_posix()],

Copilot uses AI. Check for mistakes.
@kairu-ms kairu-ms merged commit 2dec301 into Azure:main Nov 20, 2025
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants