Add optional Check to azpysdk
#44438
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new optional check to the azpysdk CLI tool. The optional check allows testing packages with various optional dependency configurations defined in pyproject.toml. This enables validation of packages in scenarios where certain dependencies are intentionally missing or different dependencies are installed.
Key changes:
- Implements the
optionalcheck class following the existing Check pattern - Integrates the check into the azpysdk CLI via registration in main.py
- Adds command-line support for targeting specific optional environments
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| eng/tools/azure-sdk-tools/azpysdk/optional.py | New check implementation with methods for running optional environment tests |
| eng/tools/azure-sdk-tools/azpysdk/main.py | Registers the new optional check with the CLI parser |
| doc/tool_usage_guide.md | Documents the new optional check command and usage |
| self.install_dev_reqs(executable, args, package_dir) | ||
|
|
There was a problem hiding this comment.
Duplicate call to install_dev_reqs. This method is called twice - once on line 62 and again on line 70. The first call on line 62 should be removed as it's redundant and occurs before the CI check that might skip the package.
| self.install_dev_reqs(executable, args, package_dir) |
| f"pytest failed for {package_name} and optional environment {env_name} with exit code {pytest_result.returncode}." | ||
| ) | ||
| config_results.append(False) | ||
|
|
There was a problem hiding this comment.
When pytest succeeds (returncode == 0), no success indicator is added to config_results. This means successful test runs are not tracked, making the logic at lines 199-209 incorrect. The list config_results will only contain False values for failures, but will be missing entries for successes. This causes the index-based access at line 204 to potentially fail with an IndexError when trying to correlate results with configs. After a successful pytest run, you should append True to config_results.
| else: | |
| config_results.append(True) |
| # uninstall any configured packages from the optional config | ||
| additional_uninstalls = config.get("uninstall", []) | ||
| if additional_uninstalls: | ||
| # TODO merge PR that creates uninstall_from_venv to use here ? |
There was a problem hiding this comment.
The inline comment references a TODO about merging a PR to create uninstall_from_venv, but the code currently uses pip_uninstall which appears to already exist and be imported. This comment may be outdated or misleading. Consider updating or removing this TODO comment to reflect the current state.
| # TODO merge PR that creates uninstall_from_venv to use here ? |
| if all(config_results): | ||
| logger.info(f"All optional environment(s) for {package_name} completed successfully.") | ||
| sys.exit(0) | ||
| else: | ||
| for i, config in enumerate(optional_configs): | ||
| if not config_results[i]: | ||
| config_name = config.get("name") | ||
| logger.error( | ||
| f"Optional environment {config_name} for {package_name} completed with non-zero exit-code. Check test results above." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
The method signature declares a return type of int, but the function uses sys.exit() instead of returning an integer value. This is inconsistent with the declared return type and prevents the calling code from properly handling the result. Consider returning the appropriate integer exit code instead of calling sys.exit() directly.
| if optional: | ||
| if env_name != optional: | ||
| logger.info( | ||
| f"{env_name} does not match targeted environment {optional}, skipping this environment." | ||
| ) | ||
| continue |
There was a problem hiding this comment.
When filtering optional environments (if a specific environment is targeted via the --optional flag), the code may skip all configs without tracking that they were skipped. If all environments are skipped, config_results will be empty, and all([]) returns True, incorrectly indicating success. Consider tracking whether any environment was actually processed, or explicitly handle the case where config_results is empty after filtering.
|
|
||
| # TODO copying from generation.py, remove old code later | ||
| # TODO remove pytest() function from ci_tools.functions as it was only used in the old version of this logic | ||
| def prepare_and_test_optional(self, package_name: str, package_dir: str, temp_dir: str, optional: str) -> int: |
There was a problem hiding this comment.
The parameter name optional shadows the class name optional, which could cause confusion. Consider renaming this parameter to something more specific like target_env_name or environment_name to better indicate its purpose and avoid the name collision.
| class optional(Check): | ||
| def __init__(self) -> None: | ||
| super().__init__() | ||
|
|
||
| def register( | ||
| self, subparsers: "argparse._SubParsersAction", parent_parsers: Optional[List[argparse.ArgumentParser]] = None | ||
| ) -> None: | ||
| """Register the optional check. The optional check invokes 'optional' requirements for a given package. View the pyproject.toml within the targeted package folder to see configuration.""" | ||
| parents = parent_parsers or [] | ||
| p = subparsers.add_parser( | ||
| "optional", | ||
| parents=parents, | ||
| help="Run the optional check to invoke 'optional' requirements for a given package.", | ||
| ) | ||
| p.set_defaults(func=self.run) | ||
|
|
||
| p.add_argument( | ||
| "-o", | ||
| "--optional", | ||
| dest="optional", | ||
| help="The target environment. If not provided, all optional environments will be run.", | ||
| required=False, | ||
| ) | ||
|
|
||
| def run(self, args: argparse.Namespace) -> int: | ||
| """Run the optional check command.""" | ||
| logger.info("Running optional check...") | ||
|
|
||
| set_envvar_defaults({"PROXY_URL": "http://localhost:5004"}) | ||
| targeted = self.get_targeted_directories(args) | ||
|
|
||
| results: List[int] = [] | ||
|
|
||
| for parsed in targeted: | ||
| package_dir = parsed.folder | ||
| package_name = parsed.name | ||
| executable, staging_directory = self.get_executable(args.isolate, args.command, sys.executable, package_dir) | ||
| logger.info(f"Processing {package_name} for optional check") | ||
|
|
||
| self.install_dev_reqs(executable, args, package_dir) | ||
|
|
||
| if in_ci(): | ||
| if not is_check_enabled(package_dir, "optional", False): | ||
| logger.info(f"Package {package_name} opts-out of optional check.") | ||
| continue | ||
|
|
||
| try: | ||
| self.install_dev_reqs(executable, args, package_dir) | ||
| except CalledProcessError as exc: | ||
| logger.error(f"Failed to install dependencies for {package_name}: {exc}") | ||
| results.append(exc.returncode) | ||
| continue | ||
|
|
||
| try: | ||
| self.prepare_and_test_optional(package_name, package_dir, staging_directory, args.optional) | ||
| except Exception as e: | ||
| logger.error(f"Optional check for package {package_name} failed with exception: {e}") | ||
| results.append(1) | ||
| continue | ||
|
|
||
| return max(results) if results else 0 | ||
|
|
||
| # TODO copying from generation.py, remove old code later | ||
| # TODO remove pytest() function from ci_tools.functions as it was only used in the old version of this logic | ||
| def prepare_and_test_optional(self, package_name: str, package_dir: str, temp_dir: str, optional: str) -> int: | ||
| optional_configs = get_config_setting(package_dir, "optional") | ||
|
|
||
| if len(optional_configs) == 0: | ||
| logger.info(f"No optional environments detected in pyproject.toml within {package_dir}.") | ||
| return 0 | ||
|
|
||
| config_results = [] | ||
|
|
||
| for config in optional_configs: | ||
| env_name = config.get("name") | ||
|
|
||
| if optional: | ||
| if env_name != optional: | ||
| logger.info( | ||
| f"{env_name} does not match targeted environment {optional}, skipping this environment." | ||
| ) | ||
| continue | ||
|
|
||
| environment_exe = prepare_environment(package_dir, temp_dir, env_name) | ||
|
|
||
| # install the package (either building manually or pulling from prebuilt directory) | ||
| create_package_and_install( | ||
| distribution_directory=temp_dir, | ||
| target_setup=package_dir, | ||
| skip_install=False, | ||
| cache_dir=None, # todo, resolve this for CI builds | ||
| work_dir=temp_dir, | ||
| force_create=False, | ||
| package_type="sdist", | ||
| pre_download_disabled=False, | ||
| python_executable=environment_exe, | ||
| ) | ||
| dev_reqs = os.path.join(package_dir, "dev_requirements.txt") | ||
| test_tools = os.path.join(REPO_ROOT, "eng", "test_tools.txt") | ||
|
|
||
| # install the dev requirements and test_tools requirements files to ensure tests can run | ||
| try: | ||
| install_into_venv(environment_exe, ["-r", dev_reqs, "-r", test_tools], package_dir) | ||
| except CalledProcessError as exc: | ||
| logger.error( | ||
| f"Unable to complete installation of dev_requirements.txt and/or test_tools.txt for {package_name}, check command output above." | ||
| ) | ||
| config_results.append(False) | ||
| break | ||
|
|
||
| # install any packages that are added in the optional config | ||
| additional_installs = config.get("install", []) | ||
| if additional_installs: | ||
| try: | ||
| install_into_venv(environment_exe, additional_installs, package_dir) | ||
| except CalledProcessError as exc: | ||
| logger.error( | ||
| f"Unable to complete installation of additional packages {additional_installs} for {package_name}, check command output above." | ||
| ) | ||
| config_results.append(False) | ||
| break | ||
|
|
||
| # uninstall any configured packages from the optional config | ||
| additional_uninstalls = config.get("uninstall", []) | ||
| if additional_uninstalls: | ||
| # TODO merge PR that creates uninstall_from_venv to use here ? | ||
| uninstall_result = pip_uninstall(additional_uninstalls, python_executable=environment_exe) | ||
| if not uninstall_result: | ||
| logger.error( | ||
| f"Unable to complete removal of packages targeted for uninstall {additional_uninstalls} for {package_name}, check command output above." | ||
| ) | ||
| config_results.append(False) | ||
| break | ||
|
|
||
| self.pip_freeze(environment_exe) | ||
|
|
||
| # invoke tests | ||
| log_level = os.getenv("PYTEST_LOG_LEVEL", "51") | ||
| junit_path = os.path.join(package_dir, f"test-junit-optional-{env_name}.xml") | ||
|
|
||
| pytest_args = [ | ||
| "-rsfE", | ||
| f"--junitxml={junit_path}", | ||
| "--verbose", | ||
| "--cov-branch", | ||
| "--durations=10", | ||
| "--ignore=azure", | ||
| "--ignore=.tox", | ||
| "--ignore-glob=.venv*", | ||
| "--ignore=build", | ||
| "--ignore=.eggs", | ||
| "--ignore=samples", | ||
| f"--log-cli-level={log_level}", | ||
| ] | ||
| pytest_args.extend(config.get("additional_pytest_args", [])) | ||
|
|
||
| logger.info(f"Invoking tests for package {package_name} and optional environment {env_name}") | ||
|
|
||
| pytest_command = ["-m", "pytest", *pytest_args] | ||
| pytest_result = self.run_venv_command( | ||
| environment_exe, pytest_command, cwd=package_dir, immediately_dump=True | ||
| ) | ||
|
|
||
| if pytest_result.returncode != 0: | ||
| if pytest_result.returncode == 5 and is_error_code_5_allowed(package_dir, package_name): | ||
| logger.info( | ||
| "pytest exited with code 5 for %s, which is allowed for management or opt-out packages.", | ||
| package_name, | ||
| ) | ||
| # Align with tox: skip coverage when tests are skipped entirely | ||
| continue | ||
| logger.error( | ||
| f"pytest failed for {package_name} and optional environment {env_name} with exit code {pytest_result.returncode}." | ||
| ) | ||
| config_results.append(False) | ||
|
|
||
| if all(config_results): | ||
| logger.info(f"All optional environment(s) for {package_name} completed successfully.") | ||
| sys.exit(0) | ||
| else: | ||
| for i, config in enumerate(optional_configs): | ||
| if not config_results[i]: | ||
| config_name = config.get("name") | ||
| logger.error( | ||
| f"Optional environment {config_name} for {package_name} completed with non-zero exit-code. Check test results above." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
The new optional check class lacks test coverage. While test_optional_functionality.py tests the configuration parsing, there are no tests for the optional.run() method or the prepare_and_test_optional() method. Consider adding tests to verify the check's behavior, especially around error handling, result aggregation, and edge cases like empty config lists or filtered environments.
| for i, config in enumerate(optional_configs): | ||
| if not config_results[i]: | ||
| config_name = config.get("name") | ||
| logger.error( | ||
| f"Optional environment {config_name} for {package_name} completed with non-zero exit-code. Check test results above." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
The error handling uses break to exit the loop when errors occur (lines 131, 143, 155), but then attempts to access config_results by index based on the full optional_configs list (line 204). Since break exits early, config_results will have fewer entries than optional_configs, causing an IndexError. The result tracking and error reporting logic needs to be redesigned to properly correlate results with configurations.
|
|
||
| try: | ||
| self.prepare_and_test_optional(package_name, package_dir, staging_directory, args.optional) | ||
| except Exception as e: |
There was a problem hiding this comment.
The try/except block catches Exception, but prepare_and_test_optional() calls sys.exit() at lines 201 and 209, which raises SystemExit. Since SystemExit inherits from BaseException (not Exception), it will not be caught by this handler. This means when processing multiple packages, the first package whose optional check completes (success or failure) will terminate the entire process, preventing other packages from being checked. This breaks the multi-package processing loop.
| except Exception as e: | |
| except (Exception, SystemExit) as e: |
1 participant
#42601
todo: first merge devtest which refactors some things i should use here ...
next: