From 0fa522f7625d511f77393bdf27c6d4100649cfff Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Tue, 22 Sep 2026 14:14:37 -0400 Subject: [PATCH 1/5] Reduce functional test collection overhead Model validation retained clients, command tables, and model copies during collection, while operation-level parametrization inflated the test count. Collect service or command cases and build transient state during execution so it can be released between cases. Grouped checks still report the individual operations and shapes that fail, so the set of validations performed is unchanged. Reuse shared service models and parsed documentation to avoid repeated work, and skip redundant logger cache invalidations during cleanup. --- tests/conftest.py | 14 ++- .../autocomplete/test_completion_files.py | 99 +++++++++---------- .../test_configured_endpoint_url.py | 16 +-- .../botocore/docs/test_streaming_body.py | 42 ++++---- tests/functional/botocore/test_auth_config.py | 27 ++--- tests/functional/botocore/test_h2_required.py | 11 +-- .../botocore/test_paginator_config.py | 5 +- .../botocore/test_response_shadowing.py | 77 ++++++++------- .../botocore/test_supported_protocols.py | 19 ++-- .../functional/botocore/test_waiter_config.py | 41 ++++---- tests/functional/docs/test_examples.py | 54 +++++----- tests/functional/test_no_event_streams.py | 67 ++++++------- tests/functional/test_shadowing.py | 72 +++++++------- 13 files changed, 269 insertions(+), 275 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index e3f9e437ed54..b07b7d6d0f61 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -19,6 +19,14 @@ from prompt_toolkit.output import DummyOutput import awscli.logger +from awscli.clidriver import create_clidriver + + +@pytest.fixture +def cli_driver(): + # Each model-validation command gets its own driver so previously checked + # command tables and models do not accumulate in a module-wide cache. + return create_clidriver() @pytest.fixture(autouse=True) @@ -33,7 +41,11 @@ def clear_loggers(): for logger_name in loggers: logger = logging.getLogger(logger_name) logger.handlers = [] - logger.setLevel(logging.NOTSET) + if logger.level != logging.NOTSET: + logger.setLevel(logging.NOTSET) + # setLevel clears every logger's effective-level cache. Do this once even + # when no levels changed, rather than once per already-reset logger. + logging.root.setLevel(logging.root.level) awscli.logger.disable_crt_logging() diff --git a/tests/functional/autocomplete/test_completion_files.py b/tests/functional/autocomplete/test_completion_files.py index 4a4c46efaabf..21c83eb56089 100644 --- a/tests/functional/autocomplete/test_completion_files.py +++ b/tests/functional/autocomplete/test_completion_files.py @@ -90,65 +90,60 @@ class ServiceTestData: service_name: str -def get_models_with_completions(): +def get_services_with_completions(): session = clidriver.create_clidriver().session loader = session.get_component('data_loader') - services_with_completions = loader.list_available_services('completions-1') - models = [] - for service_name in services_with_completions: - service_model = loader.load_service_model(service_name, 'service-2') - api_version = service_model['metadata']['apiVersion'] - completions = loader.load_service_model( - service_name, 'completions-1', api_version - ) - models.append( - ServiceTestData(service_model, completions, service_name) - ) - return models - - -def _get_invalid_completion_operations(): + return loader.list_available_services('completions-1') + + +@pytest.fixture(scope='module', params=get_services_with_completions()) +def service_test_data(request): + # Only the current service's documents live in this fixture; collection + # retains service names, not another copy of every service model. + service_name = request.param + session = clidriver.create_clidriver().session + loader = session.get_component('data_loader') + service_model = loader.load_service_model(service_name, 'service-2') + api_version = service_model['metadata']['apiVersion'] + completions = loader.load_service_model( + service_name, 'completions-1', api_version + ) + return ServiceTestData(service_model, completions, service_name) + + +def _get_invalid_completion_operations(service_test_data): cases = [] - for test_data in get_models_with_completions(): - known_ops = set(test_data.service_model['operations']) - for op_name in test_data.completions['operations']: - if op_name not in known_ops: - cases.append((test_data.service_name, op_name)) - for resource in test_data.completions.get('resources', {}).values(): - op_name = resource.get('operation') - if op_name and op_name not in known_ops: - cases.append((test_data.service_name, op_name)) + known_ops = set(service_test_data.service_model['operations']) + for op_name in service_test_data.completions['operations']: + if op_name not in known_ops: + cases.append(op_name) + resources = service_test_data.completions.get('resources', {}) + for resource in resources.values(): + op_name = resource.get('operation') + if op_name and op_name not in known_ops: + cases.append(op_name) return cases @pytest.mark.validates_models -@pytest.mark.parametrize( - "service_name, operation_name", - _get_invalid_completion_operations(), - ids=lambda val: str(val), -) def test_completions_operations_exist_in_model( - service_name, operation_name, record_property + service_test_data, record_property ): - record_property('aws_service', service_name) - record_property('aws_operation', operation_name) - pytest.fail( - f"Completions file for '{service_name}' references operation " - f"'{operation_name}' which does not exist in the service model. " - f"The completions-1.json file must be updated to remove or " - f"update references to this operation." + invalid_operations = _get_invalid_completion_operations(service_test_data) + record_property('aws_service', service_test_data.service_name) + for operation_name in invalid_operations: + record_property('aws_operation', operation_name) + assert not invalid_operations, ( + f"Completions file for '{service_test_data.service_name}' references " + f"operations absent from the service model: {invalid_operations}. " + "The completions-1.json file must remove or update these references." ) @pytest.mark.validates_models -@pytest.mark.parametrize( - "test_data", - get_models_with_completions(), - ids=lambda test_data: test_data.service_name, -) -def test_verify_generated_completions_are_valid(test_data): - completions = test_data.completions - service_model = test_data.service_model +def test_verify_generated_completions_are_valid(service_test_data): + completions = service_test_data.completions + service_model = service_test_data.service_model _validate_schema(completions) # Validate that every operation named in the completions # file references a known operation. @@ -256,11 +251,11 @@ def _validate_jmespath_expression( arg_gen = ArgumentGenerator(use_member_names=True) sample_output = arg_gen.generate_skeleton(output_shape) search_result = jmespath.search(jmespath_expr, sample_output) - assert ( - search_result is not None - ), "Expression is blob or another unsupported type" + assert search_result is not None, ( + "Expression is blob or another unsupported type" + ) assert search_result, f"Expression is broken: {jmespath_expr}" sample_arg = search_result[0] - assert isinstance( - sample_arg, str - ), f"Expression not a string: {jmespath_expr}" + assert isinstance(sample_arg, str), ( + f"Expression not a string: {jmespath_expr}" + ) diff --git a/tests/functional/botocore/configured_endpoint_urls/test_configured_endpoint_url.py b/tests/functional/botocore/configured_endpoint_urls/test_configured_endpoint_url.py index f7b7bfc95dd6..869d5560aafd 100644 --- a/tests/functional/botocore/configured_endpoint_urls/test_configured_endpoint_url.py +++ b/tests/functional/botocore/configured_endpoint_urls/test_configured_endpoint_url.py @@ -21,7 +21,7 @@ from botocore.compat import urlsplit from botocore.config import Config -from tests import ClientHTTPStubber +from tests import ALL_SERVICES, ClientHTTPStubber, create_session ENDPOINT_TESTDATA_FILE = Path(__file__).parent / "profile-tests.json" @@ -124,7 +124,7 @@ def _do_create_client( f.write(config_file_contents) f.flush() - return botocore.session.Session(profile=profile).create_client( + return create_session(profile=profile).create_client( service, **client_args ) @@ -159,15 +159,9 @@ def assert_endpoint_url_used_for_operation( def _known_service_names_and_ids(): - my_session = botocore.session.get_session() - loader = my_session.get_component('data_loader') - available_services = loader.list_available_services('service-2') - - result = [] - for service_name in available_services: - model = my_session.get_service_model(service_name) - result.append((model.service_name, model.service_id)) - return sorted(result) + return sorted( + (model.service_name, model.service_id) for model in ALL_SERVICES + ) SERVICE_TO_OPERATION = {'s3': 'list_buckets', 'dynamodb': 'list_tables'} diff --git a/tests/functional/botocore/docs/test_streaming_body.py b/tests/functional/botocore/docs/test_streaming_body.py index 6cd41d6b9d9a..7b4e1b994bf0 100644 --- a/tests/functional/botocore/docs/test_streaming_body.py +++ b/tests/functional/botocore/docs/test_streaming_body.py @@ -13,30 +13,36 @@ from botocore import xform_name from botocore.docs.service import ServiceDocumenter +from tests import ALL_SERVICES from tests.functional.botocore.docs import BaseDocsFunctionalTest class TestStreamingBodyDocumentation(BaseDocsFunctionalTest): def test_all_streaming_body_are_properly_documented(self): - for service in self._session.get_available_services(): - client = self._session.create_client( - service, - region_name='us-east-1', - aws_access_key_id='foo', - aws_secret_access_key='bar', - ) - service_model = client.meta.service_model - for operation in service_model.operation_names: - operation_model = service_model.operation_model(operation) - if operation_model.has_streaming_output: - self.assert_streaming_body_is_properly_documented( - service, xform_name(operation) - ) + for service_model in ALL_SERVICES: + operations = [ + xform_name(operation) + for operation in service_model.operation_names + if service_model.operation_model( + operation + ).has_streaming_output + ] + if not operations: + continue + service = service_model.service_name + # Generate the complete service once, not once per streaming + # operation. Each operation's documentation is still checked. + ServiceDocumenter( + service, self._session, self.root_services_path + ).document_service() + for operation in operations: + self.assert_streaming_body_is_properly_documented( + service, operation + ) - def assert_streaming_body_is_properly_documented(self, service, operation): - ServiceDocumenter( - service, self._session, self.root_services_path - ).document_service() + def assert_streaming_body_is_properly_documented( + self, service, operation + ): contents = self.get_client_method_contents(service, operation) method_docs = self.get_method_document_block(operation, contents) self.assert_contains_line('StreamingBody', method_docs) diff --git a/tests/functional/botocore/test_auth_config.py b/tests/functional/botocore/test_auth_config.py index ad60fb416e4a..089855920063 100644 --- a/tests/functional/botocore/test_auth_config.py +++ b/tests/functional/botocore/test_auth_config.py @@ -14,7 +14,7 @@ from botocore.config import Config from botocore.handlers import get_bearer_auth_supported_services -from tests import create_session, mock +from tests import ALL_SERVICES, create_session, mock # In the future, a service may have a list of credentials requirements where one # signature may fail and others may succeed. e.g. a service may want to use bearer @@ -46,15 +46,11 @@ def _all_test_cases(): - session = create_session() - loader = session.get_component('data_loader') - - services = loader.list_available_services('service-2') auth_services = [] auth_operations = [] - for service in services: - service_model = session.get_service_model(service) + for service_model in ALL_SERVICES: + service = service_model.service_name signing_name = service_model.signing_name auth_config = service_model.metadata.get('auth', {}) if signing_name in KNOWN_MIXED_AUTH_SERVICES: @@ -62,11 +58,11 @@ def _all_test_cases(): # Skip service due to known mixed auth configurations. continue if auth_config: - auth_services.append([service, auth_config]) + auth_services.append((service, auth_config)) for operation in service_model.operation_names: operation_model = service_model.operation_model(operation) if operation_model.auth: - auth_operations.append([service, operation_model]) + auth_operations.append((service, operation_model)) return auth_services, auth_operations @@ -74,14 +70,9 @@ def _all_test_cases(): @pytest.mark.validates_models -@pytest.mark.parametrize( - "auth_service, auth_config", - AUTH_SERVICES -) +@pytest.mark.parametrize("auth_service, auth_config", AUTH_SERVICES) def test_all_requirements_match_for_service( - auth_service, - auth_config, - record_property + auth_service, auth_config, record_property ): record_property("aws_service", auth_service) # Validates that all service-level signature types have the same requirements @@ -91,7 +82,9 @@ def test_all_requirements_match_for_service( @pytest.mark.validates_models @pytest.mark.parametrize("auth_service, operation_model", AUTH_OPERATIONS) -def test_all_requirements_match_for_operation(auth_service, operation_model, record_property): +def test_all_requirements_match_for_operation( + auth_service, operation_model, record_property +): record_property("aws_service", operation_model.service_model.service_name) record_property("aws_operation", operation_model.name) # Validates that all operation-level signature types have the same requirements diff --git a/tests/functional/botocore/test_h2_required.py b/tests/functional/botocore/test_h2_required.py index 1ae133c398ea..21ee7d020659 100644 --- a/tests/functional/botocore/test_h2_required.py +++ b/tests/functional/botocore/test_h2_required.py @@ -11,7 +11,8 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import pytest -from botocore.session import get_session + +from tests import ALL_SERVICES _H2_REQUIRED = object() # Service names to list of known HTTP 2 operations @@ -27,15 +28,11 @@ def _all_test_cases(): - session = get_session() - loader = session.get_component('data_loader') - - services = loader.list_available_services('service-2') h2_services = [] h2_operations = [] - for service in services: - service_model = session.get_service_model(service) + for service_model in ALL_SERVICES: + service = service_model.service_name h2_config = service_model.metadata.get('protocolSettings', {}).get( 'h2' ) diff --git a/tests/functional/botocore/test_paginator_config.py b/tests/functional/botocore/test_paginator_config.py index 39c4c2ba04a2..d871438c839a 100644 --- a/tests/functional/botocore/test_paginator_config.py +++ b/tests/functional/botocore/test_paginator_config.py @@ -17,6 +17,8 @@ import pytest from jmespath.exceptions import JMESPathError +from tests import ALL_SERVICES + KNOWN_PAGE_KEYS = set( [ 'input_token', @@ -142,8 +144,9 @@ def _pagination_configs(): session = botocore.session.get_session() loader = session.get_component('data_loader') services = loader.list_available_services('paginators-1') + service_models = {model.service_name: model for model in ALL_SERVICES} for service_name in services: - service_model = session.get_service_model(service_name) + service_model = service_models[service_name] page_config = loader.load_service_model(service_name, 'paginators-1') for op_name, single_config in page_config['pagination'].items(): yield (op_name, single_config, service_model) diff --git a/tests/functional/botocore/test_response_shadowing.py b/tests/functional/botocore/test_response_shadowing.py index a5cbaac047d0..b92d28b08a60 100644 --- a/tests/functional/botocore/test_response_shadowing.py +++ b/tests/functional/botocore/test_response_shadowing.py @@ -15,36 +15,47 @@ from tests import ALL_SERVICES -def _all_service_error_shapes(): - for service_model in ALL_SERVICES: - yield from service_model.error_shapes - - -def _all_operations(): - for service_model in ALL_SERVICES: - for operation_name in service_model.operation_names: - yield service_model.operation_model(operation_name).output_shape - - -def _assert_not_shadowed(key, shape): - if not shape: - return - - assert ( - key not in shape.members - ), f'Found shape "{shape.name}" that shadows the botocore response key "{key}"' - - -@pytest.mark.parametrize("operation_output_shape", _all_operations()) -def test_response_metadata_is_not_shadowed(operation_output_shape): - _assert_not_shadowed('ResponseMetadata', operation_output_shape) - - -@pytest.mark.parametrize("error_shape", _all_service_error_shapes()) -def test_exceptions_do_not_shadow_response_metadata(error_shape): - _assert_not_shadowed('ResponseMetadata', error_shape) - - -@pytest.mark.parametrize("error_shape", _all_service_error_shapes()) -def test_exceptions_do_not_shadow_error(error_shape): - _assert_not_shadowed('Error', error_shape) +def _assert_not_shadowed(key, service_name, shapes): + errors = [ + f'{service_name}.{name}: shape "{shape.name}" shadows ' + f'the botocore response key "{key}"' + for name, shape in shapes + if shape is not None and key in shape.members + ] + assert not errors, '\n'.join(errors) + + +@pytest.mark.parametrize( + 'service_model', ALL_SERVICES, ids=lambda model: model.service_name +) +def test_response_metadata_is_not_shadowed(service_model): + _assert_not_shadowed( + 'ResponseMetadata', + service_model.service_name, + ( + (name, service_model.operation_model(name).output_shape) + for name in service_model.operation_names + ), + ) + + +@pytest.mark.parametrize( + 'service_model', ALL_SERVICES, ids=lambda model: model.service_name +) +def test_exceptions_do_not_shadow_response_metadata(service_model): + _assert_not_shadowed( + 'ResponseMetadata', + service_model.service_name, + ((shape.name, shape) for shape in service_model.error_shapes), + ) + + +@pytest.mark.parametrize( + 'service_model', ALL_SERVICES, ids=lambda model: model.service_name +) +def test_exceptions_do_not_shadow_error(service_model): + _assert_not_shadowed( + 'Error', + service_model.service_name, + ((shape.name, shape) for shape in service_model.error_shapes), + ) diff --git a/tests/functional/botocore/test_supported_protocols.py b/tests/functional/botocore/test_supported_protocols.py index 901202e47a88..f6b1f39f0133 100644 --- a/tests/functional/botocore/test_supported_protocols.py +++ b/tests/functional/botocore/test_supported_protocols.py @@ -12,19 +12,16 @@ # language governing permissions and limitations under the License. import pytest from botocore.args import PRIORITY_ORDERED_SUPPORTED_PROTOCOLS -from botocore.session import get_session + +from tests import ALL_SERVICES def _multi_protocol_test_cases(): - session = get_session() - loader = session.get_component('data_loader') - services = loader.list_available_services('service-2') multi_protocol_services = [] supported_protocols = [] - for service in services: - service_model = session.get_service_model(service) + for service_model in ALL_SERVICES: if 'protocols' in service_model.metadata: - multi_protocol_services.append(service) + multi_protocol_services.append(service_model.service_name) supported_protocols.append( service_model.metadata.get('protocols', []) ) @@ -32,15 +29,11 @@ def _multi_protocol_test_cases(): def _single_protocol_test_cases(): - session = get_session() - loader = session.get_component('data_loader') - services = loader.list_available_services('service-2') single_protocol_services = [] supported_protocol = [] - for service in services: - service_model = session.get_service_model(service) + for service_model in ALL_SERVICES: if 'protocols' not in service_model.metadata: - single_protocol_services.append(service) + single_protocol_services.append(service_model.service_name) supported_protocol.append(service_model.metadata.get('protocol')) return list(zip(single_protocol_services, supported_protocol)) diff --git a/tests/functional/botocore/test_waiter_config.py b/tests/functional/botocore/test_waiter_config.py index 481e60f7e5c4..e324f0035135 100644 --- a/tests/functional/botocore/test_waiter_config.py +++ b/tests/functional/botocore/test_waiter_config.py @@ -13,7 +13,6 @@ import botocore.session import jmespath import pytest -from botocore.exceptions import UnknownServiceError from botocore.utils import ArgumentGenerator from jsonschema import Draft4Validator @@ -76,26 +75,26 @@ } -def _waiter_configs(): +def _services_with_waiters(): session = botocore.session.get_session() - validator = Draft4Validator(WAITER_SCHEMA) - for service_name in session.get_available_services(): - client = session.create_client(service_name, 'us-east-1') - try: - # We use the loader directly here because we need the entire - # json document, not just the portions exposed (either - # internally or externally) by the WaiterModel class. - loader = session.get_component('data_loader') - waiter_model = loader.load_service_model(service_name, 'waiters-2') - except UnknownServiceError: - # The service doesn't have waiters - continue - yield validator, waiter_model, client + loader = session.get_component('data_loader') + waiter_services = set(loader.list_available_services('waiters-2')) + return [ + name + for name in session.get_available_services() + if name in waiter_services + ] -@pytest.mark.parametrize("validator, waiter_model, client", _waiter_configs()) -def test_lint_waiter_configs(validator, waiter_model, client): +@pytest.mark.parametrize('service_name', _services_with_waiters()) +def test_lint_waiter_configs(service_name): + # Keep clients and their cached models out of collected test parameters. + session = botocore.session.get_session() + loader = session.get_component('data_loader') + waiter_model = loader.load_service_model(service_name, 'waiters-2') + validator = Draft4Validator(WAITER_SCHEMA) _validate_schema(validator, waiter_model) + client = session.create_client(service_name, 'us-east-1') for waiter_name in client.waiter_names: _lint_single_waiter(client, waiter_name, client.meta.service_model) @@ -117,7 +116,7 @@ def _lint_single_waiter(client, waiter_name, service_model): # Needs to reference an existing operation name. if operation_name not in service_model.operation_names: raise AssertionError( - "Waiter config references unknown " f"operation: {operation_name}" + f"Waiter config references unknown operation: {operation_name}" ) # Needs to have at least one acceptor. if not waiter.config.acceptors: @@ -145,9 +144,9 @@ def _validate_acceptor(acceptor, op_model, waiter_name): # The JMESPath expression should have the potential to match something # in the response shape. output_shape = op_model.output_shape - assert ( - output_shape is not None - ), f"Waiter '{waiter_name}' has JMESPath expression with no output shape: {op_model}" + assert output_shape is not None, ( + f"Waiter '{waiter_name}' has JMESPath expression with no output shape: {op_model}" + ) # We want to check if the JMESPath expression makes sense. # To do this, we'll generate sample output and evaluate the # JMESPath expression against the output. We'll then diff --git a/tests/functional/docs/test_examples.py b/tests/functional/docs/test_examples.py index b378ce25cf38..12d2a7410e16 100644 --- a/tests/functional/docs/test_examples.py +++ b/tests/functional/docs/test_examples.py @@ -1055,15 +1055,13 @@ def test_examples(command, subcommand): @pytest.mark.filterwarnings('ignore::DeprecationWarning') @pytest.mark.parametrize("example_file", RST_DOC_EXAMPLES) def test_rst_doc_examples(command_validator, example_file): - verify_has_only_ascii_chars(example_file) - verify_is_valid_rst(example_file) - verify_cli_commands_valid(example_file, command_validator) - verify_no_http_links(example_file) + contents = verify_has_only_ascii_chars(example_file) + document = verify_is_valid_rst(example_file, contents) + verify_cli_commands_valid(example_file, command_validator, document) + verify_no_http_links(example_file, contents) -def verify_no_http_links(filename): - with open(filename) as f: - contents = f.read() +def verify_no_http_links(filename, contents): match = HTTP_LINK_REGEX.search(contents) if match: error_line_number = line_num(contents, match.span()[0]) @@ -1073,8 +1071,8 @@ def verify_no_http_links(filename): marker_idx = error_line.find('http://') - 1 marker_line = (" " * marker_idx) + '^' raise AssertionError( - 'Found http:// link in the examples file %s, line %s\n' - '%s\n%s' % (filename, error_line_number, error_line, marker_line) + f'Found http:// link in the examples file {filename}, line {error_line_number}\n' + f'{error_line}\n{marker_line}' ) @@ -1082,31 +1080,34 @@ def verify_has_only_ascii_chars(filename): with open(filename, 'rb') as f: bytes_content = f.read() try: - bytes_content.decode('ascii') + contents = bytes_content.decode('ascii') except UnicodeDecodeError as e: # The test has failed so we'll try to provide a useful error # message. offset = e.start spread = 20 - bad_text = bytes_content[offset - spread : e.start + spread] + bad_text = bytes_content[ + offset - spread : e.start + spread + ].decode('ascii', errors='backslashreplace') underlined = ' ' * spread + '^' error_text = '\n'.join([bad_text, underlined]) line_number = bytes_content[:offset].count(b'\n') + 1 raise AssertionError( - "Non ascii characters found in the examples file %s, line %s:" - "\n\n%s\n" % (filename, line_number, error_text) + f"Non ascii characters found in the examples file {filename}, line {line_number}:" + f"\n\n{error_text}\n" ) + # Match the universal-newline translation of the previous text reads. + return contents.replace('\r\n', '\n').replace('\r', '\n') -def verify_is_valid_rst(filename): - _, errors = parse_rst(filename) +def verify_is_valid_rst(filename, contents): + document, errors = parse_rst(contents) if errors: raise AssertionError(_make_error_msg(filename, errors)) + return document -def parse_rst(filename): - with open(filename) as f: - contents = f.read() +def parse_rst(contents): parser = docutils.parsers.rst.Parser() components = (docutils.parsers.rst.Parser,) settings = docutils.frontend.OptionParser( @@ -1133,7 +1134,7 @@ def _make_error_msg(filename, errors): lines = f.readlines() relative_name = filename[len(EXAMPLES_DIR) + 1 :] failure_message = [ - 'The file "%s" contains invalid RST: ' % relative_name, + f'The file "{relative_name}" contains invalid RST: ', '', ] for error in errors: @@ -1146,21 +1147,20 @@ def _make_error_msg(filename, errors): if line_number > 0: line_number -= 1 current_message = [ - 'Line %s: %s' % (error['line_number'], error['msg']), - ' %s' % lines[line_number], + f"Line {error['line_number']}: {error['msg']}", + f' {lines[line_number]}', ] failure_message.extend(current_message) return '\n'.join(failure_message) -def verify_cli_commands_valid(filename, command_validator): - cli_commands = find_all_cli_commands(filename) +def verify_cli_commands_valid(filename, command_validator, document): + cli_commands = find_all_cli_commands(document) for command in cli_commands: command_validator.validate_cli_command(command, filename) -def find_all_cli_commands(filename): - document, _ = parse_rst(filename) +def find_all_cli_commands(document): visitor = CollectCLICommands(document) document.walk(visitor) return visitor.cli_commands @@ -1219,8 +1219,8 @@ def validate_cli_command(self, command, filename): break except Exception as e: raise AssertionError( - "Failed to parse this example as shell command: %s\n\n" - "Error:\n%s\n" % (command, e) + f"Failed to parse this example as shell command: {command}\n\n" + f"Error:\n{e}\n" ) try: diff --git a/tests/functional/test_no_event_streams.py b/tests/functional/test_no_event_streams.py index 2b6cde301969..2692b76daeb7 100644 --- a/tests/functional/test_no_event_streams.py +++ b/tests/functional/test_no_event_streams.py @@ -19,50 +19,47 @@ _ALLOWED_COMMANDS = ['s3api select-object-content'] -def _generate_command_tests(): - driver = create_clidriver() +def _generate_command_tests(driver, command_name): help_command = driver.create_help_command() - for command_name, command_obj in help_command.command_table.items(): - sub_help = command_obj.create_help_command() - if hasattr(sub_help, 'command_table'): - for sub_name, sub_command in sub_help.command_table.items(): - op_help = sub_command.create_help_command() - model = op_help.obj - if isinstance(model, OperationModel): - # Extract the properties needed for tests to avoid - # parametrizing entire model objects, which may cause - # excessive memory usage. - if not model.has_event_stream_input and not model.has_event_stream_output: - # Only parameterize the models that actually have event streams. - # This has shown to improve test execution performance. - continue - model_description = { - 'service_name': model.service_model.service_name, - 'name': model.name, - } - yield command_name, sub_name, model_description + command_obj = help_command.command_table[command_name] + sub_help = command_obj.create_help_command() + if hasattr(sub_help, 'command_table'): + for sub_name, sub_command in sub_help.command_table.items(): + op_help = sub_command.create_help_command() + model = op_help.obj + if isinstance(model, OperationModel): + if ( + not model.has_event_stream_input + and not model.has_event_stream_output + ): + continue + model_description = { + 'service_name': model.service_model.service_name, + 'name': model.name, + } + yield command_name, sub_name, model_description @pytest.mark.validates_models @pytest.mark.parametrize( - "command_name, sub_name, model", _generate_command_tests() + 'command_name', + tuple(create_clidriver().create_help_command().command_table), ) def test_no_event_stream_unless_allowed( - command_name, - sub_name, - model, - record_property + command_name, cli_driver, record_property ): - full_command = f'{command_name} {sub_name}' - # Store the service and operation in - # PyTest custom properties - record_property( - 'aws_service', model['service_name'] - ) - record_property('aws_operation', model['name']) + disallowed = [] + for _, sub_name, model in _generate_command_tests( + cli_driver, command_name + ): + full_command = f'{command_name} {sub_name}' + record_property('aws_service', model['service_name']) + record_property('aws_operation', model['name']) + if full_command not in _ALLOWED_COMMANDS: + disallowed.append(full_command) supported_commands = '\n'.join(_ALLOWED_COMMANDS) - assert full_command in _ALLOWED_COMMANDS, ( - f'The {full_command} command uses event streams ' + assert not disallowed, ( + f'The {", ".join(disallowed)} commands use event streams ' 'which is only supported for these operations:\n' f'{supported_commands}' ) diff --git a/tests/functional/test_shadowing.py b/tests/functional/test_shadowing.py index 997538226096..f7a13b45f6ab 100644 --- a/tests/functional/test_shadowing.py +++ b/tests/functional/test_shadowing.py @@ -16,43 +16,35 @@ from awscli.clidriver import create_clidriver -def _generate_command_tests(): - driver = create_clidriver() +def _generate_command_tests(driver, command_name): help_command = driver.create_help_command() top_level_params = set(driver.create_help_command().arg_table.keys()) - for command_name, command_obj in list(help_command.command_table.items()): - sub_help = command_obj.create_help_command() - if hasattr(sub_help, 'command_table'): - for sub_name, sub_command in sub_help.command_table.items(): - op_help = sub_command.create_help_command() - model = op_help.obj - arg_table = op_help.arg_table - if not isinstance(model, OperationModel): - continue - yield ( - command_name, - sub_name, - model.service_model.service_name, - model.name, - arg_table, - top_level_params - ) + command_obj = help_command.command_table[command_name] + sub_help = command_obj.create_help_command() + if hasattr(sub_help, 'command_table'): + for sub_name, sub_command in sub_help.command_table.items(): + op_help = sub_command.create_help_command() + model = op_help.obj + arg_table = op_help.arg_table + if not isinstance(model, OperationModel): + continue + yield ( + command_name, + sub_name, + model.service_model.service_name, + model.name, + tuple(arg_table), + top_level_params, + ) @pytest.mark.validates_models @pytest.mark.parametrize( - "command_name, sub_name, service_name, " - "operation_name, arg_table, builtins", - _generate_command_tests() + 'command_name', + tuple(create_clidriver().create_help_command().command_table), ) def test_no_shadowed_builtins( - command_name, - sub_name, - service_name, - operation_name, - arg_table, - builtins, - record_property + command_name, cli_driver, record_property ): """Verify no command params are shadowed or prefixed by the built-in param. @@ -80,18 +72,20 @@ def test_no_shadowed_builtins( """ errors = [] - for arg_name in arg_table: - if any(p.startswith(arg_name) for p in builtins): - # Then we are shadowing or prefixing a top level argument - errors.append( + for case in _generate_command_tests(cli_driver, command_name): + _, sub_name, service_name, operation_name, arg_table, builtins = case + shadowed = [ + arg_name + for arg_name in arg_table + if any(p.startswith(arg_name) for p in builtins) + ] + if shadowed: + record_property('aws_service', service_name) + record_property('aws_operation', operation_name) + errors.extend( 'Shadowing/Prefixing a top level option: ' f'{command_name}.{sub_name}.{arg_name}' + for arg_name in shadowed ) if errors: - # Store the service and operation in - # PyTest custom properties - record_property( - 'aws_service', service_name - ) - record_property('aws_operation', operation_name) raise AssertionError('\n' + '\n'.join(errors)) From 94b31c66dc093572d4c9cfd40314ee644cb8c306 Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Tue, 22 Sep 2026 15:15:18 -0400 Subject: [PATCH 2/5] Stop configure sso tests from resolving start URLs Vanity URL support made `aws configure sso` resolve any start URL that isn't an AWS-owned domain by following redirects over the network. Most tests in `test_sso.py` use placeholder start URLs such as `https://starturl`, so every one of them made a real HTTP request that failed at DNS and fell back to the region prompt. The tests passed, but each one waited for the lookup to fail, which took about eight seconds apiece on some machines, and their behavior depended on the network they ran on. An autouse fixture now makes `resolve_start_url` raise the same `ConfigurationError` the real resolver raises on failure. The tests exercise the same fallback path as before, without network access. The tests that cover vanity URL resolution already patch `resolve_start_url` themselves, and those patches override the fixture. --- .../unit/customizations/configure/test_sso.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/unit/customizations/configure/test_sso.py b/tests/unit/customizations/configure/test_sso.py index b52e4e3dcb8d..d7cd5c0a4eae 100644 --- a/tests/unit/customizations/configure/test_sso.py +++ b/tests/unit/customizations/configure/test_sso.py @@ -41,7 +41,10 @@ display_account, get_account_sorting_key, ) -from awscli.customizations.exceptions import ParamValidationError +from awscli.customizations.exceptions import ( + ConfigurationError, + ParamValidationError, +) from awscli.customizations.sso.utils import ( PrintOnlyHandler, do_sso_login, @@ -293,6 +296,19 @@ def mock_do_sso_login(): return login_mock +@pytest.fixture(autouse=True) +def mock_resolve_start_url(): + # The placeholder start URLs in these tests are not AWS-owned, so the + # command would otherwise try to resolve them over the network. Failing + # fast exercises the same fallback to the region prompt without any + # network access. Tests covering resolution patch this explicitly. + with mock.patch( + 'awscli.customizations.configure.sso_commands.resolve_start_url', + side_effect=ConfigurationError('Failed to resolve start URL'), + ) as resolve_start_url: + yield resolve_start_url + + @pytest.fixture def sso_cmd_factory( env, botocore_session, prompter, mock_do_sso_login, selector From a8089a9da4757d639cc3b4e86d1fd4be4d4f2c71 Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Tue, 22 Sep 2026 15:18:03 -0400 Subject: [PATCH 3/5] Avoid reverse DNS lookups in AuthCodeFetcher tests `AuthCodeFetcher` starts an `http.server.HTTPServer` for the OAuth callback. When the server binds, the standard library calls `socket.getfqdn`, which does a reverse DNS lookup of the local hostname. On machines where that hostname can't be resolved locally, the lookup waits for a DNS timeout of about five seconds. Every login test that used the same-device flow and every `AuthCodeFetcher` test paid that wait, even though nothing reads the server name the lookup produces. The login functional tests already mock `fetch_token`, so they never use the callback server. They now mock `AuthCodeFetcher` outright, as the `sso login` functional tests already do. The `AuthCodeFetcher` unit tests need a real server to send requests to, so they patch only `socket.getfqdn` while the server is created. The server still binds a real port and handles real requests. The same lookup happens when users run `aws login` or `aws sso login` with the browser flow. This commit changes only the tests. --- tests/functional/login/test_login.py | 11 +++++++++++ tests/unit/customizations/sso/test_utils.py | 15 +++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/tests/functional/login/test_login.py b/tests/functional/login/test_login.py index a17adf87a037..17db45f5cced 100644 --- a/tests/functional/login/test_login.py +++ b/tests/functional/login/test_login.py @@ -27,6 +27,17 @@ ) +@pytest.fixture(autouse=True) +def mock_auth_code_fetcher(): + # Same-device login creates a local callback server, and binding it does + # a reverse DNS lookup that can stall for seconds. These tests mock + # fetch_token, so the server is never used. + with mock.patch( + 'awscli.customizations.login.login.AuthCodeFetcher', autospec=True + ) as auth_code_fetcher: + yield auth_code_fetcher + + @pytest.fixture def mock_token_loader(): return mock.Mock() diff --git a/tests/unit/customizations/sso/test_utils.py b/tests/unit/customizations/sso/test_utils.py index 816ec4d9e703..1d99aa25eb94 100644 --- a/tests/unit/customizations/sso/test_utils.py +++ b/tests/unit/customizations/sso/test_utils.py @@ -312,13 +312,20 @@ def test_missing_expected_query_params(self): fetcher.set_auth_code_and_state.assert_not_called() +def _create_auth_code_fetcher(**kwargs): + # Binding the fetcher's HTTPServer does a reverse DNS lookup of the host, + # which can stall for seconds. The resulting server name is unused. + with mock.patch('socket.getfqdn', return_value='localhost'): + return AuthCodeFetcher(**kwargs) + + class TestAuthCodeFetcher: """Tests for the AuthCodeFetcher class, which is the local web server we use to handle the OAuth 2.0 callback """ def setup_method(self): - self.fetcher = AuthCodeFetcher() + self.fetcher = _create_auth_code_fetcher() self.url = ( f'http://127.0.0.1:{self.fetcher.http_server.server_address[1]}/' ) @@ -368,12 +375,12 @@ def test_get_auth_code_and_state_timeout(): since we need to override the constants """ with pytest.raises(PendingAuthorizationExpiredError): - AuthCodeFetcher().get_auth_code_and_state() + _create_auth_code_fetcher().get_auth_code_and_state() def test_auth_code_fetcher_uses_redirect_port(): redirect_port = _get_available_port() - fetcher = AuthCodeFetcher(redirect_port=redirect_port) + fetcher = _create_auth_code_fetcher(redirect_port=redirect_port) try: assert fetcher.http_server.server_port == redirect_port assert fetcher.redirect_uri_with_port() == ( @@ -384,7 +391,7 @@ def test_auth_code_fetcher_uses_redirect_port(): def test_auth_code_fetcher_binds_to_loopback_only(): - fetcher = AuthCodeFetcher() + fetcher = _create_auth_code_fetcher() try: assert fetcher.http_server.server_address[0] == '127.0.0.1' finally: From 4fe53eb21c9fd8a6a383d5a1982fe6400f332b93 Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Wed, 23 Sep 2026 19:13:51 -0400 Subject: [PATCH 4/5] Restore one operation per model-validation testcase The internal build system parses the JUnitXML report of tests marked `validates_models`. It expects each testcase to record one `aws_service` and `aws_operation` pair so that each failure maps to a single operation. c1a691e4b grouped the event-stream and shadowing checks by top-level command and the completions check by service, then called `record_property` inside a loop, so one testcase could record several pairs. The event-stream test also recorded properties for allow-listed operations before checking for failures. A new streaming operation on s3 would have been reported as SelectObjectContent. The current report avoids this only because `removals.py` removes every other event-stream operation. This is the same regression e6b7544d0 fixed in January. Each testcase is again one parametrized service and operation. The parameters now come from the botocore models in `ALL_SERVICES` instead of from walking the CLI command table at collection time. Every xdist worker collects every file, so collection work repeats in each worker. Removing that walk was the goal of c1a691e4b. Each test builds the command table for only the service it checks. The shadowing test parametrizes over every operation and indexes each service on first use with a fresh driver, so command tables do not accumulate across services. The completions test parametrizes over violations and emits one skipped placeholder when there are none, as it did before c1a691e4b. Mapping a botocore service to its top-level command requires the public `service_model` property, which loads that service's model. Loading it for all 431 commands uses about 400MB per process. Instead, a command whose name matches a botocore service is assumed to be bound to that service, and `iter_operation_commands` checks the binding before yielding. Only commands with a different name, such as `s3api`, load a model. If the assumption is wrong, `iter_operation_commands` raises `RuntimeError` and the test fails. An operation the CLI does not expose passes without recording properties, so the report lists only operations that were checked. An autouse fixture in `tests/conftest.py` fails any `validates_models` test that records a property name twice. A change that reintroduces this regression now fails in the test suite instead of producing a report that misattributes failures. --- tests/conftest.py | 79 ++++++++++++- .../autocomplete/test_completion_files.py | 50 +++++--- tests/functional/test_no_event_streams.py | 99 ++++++++++------ tests/functional/test_shadowing.py | 111 ++++++++++++------ 4 files changed, 247 insertions(+), 92 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index b07b7d6d0f61..68683e4c83f1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,16 +10,18 @@ # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +import collections import logging import platform import pytest +from botocore.model import OperationModel from prompt_toolkit.application import create_app_session from prompt_toolkit.input import create_pipe_input from prompt_toolkit.output import DummyOutput import awscli.logger -from awscli.clidriver import create_clidriver +from awscli.clidriver import ServiceCommand, create_clidriver @pytest.fixture @@ -29,6 +31,81 @@ def cli_driver(): return create_clidriver() +@pytest.fixture(scope='session') +def service_command_names(): + """Map each botocore service name to the top-level CLI command bound to + it, built once per process from a throwaway driver.""" + # A command whose name is a botocore service name is assumed to be bound + # to that service; iter_operation_commands verifies this before yielding, + # so only renamed commands need their model loaded here. + driver = create_clidriver() + available = set(driver.session.get_available_services()) + names = {} + command_table = driver.create_help_command().command_table + for command_name, command in command_table.items(): + if not isinstance(command, ServiceCommand): + continue + if command_name in available: + service_name = command_name + else: + service_name = command.service_model.service_name + if service_name in names: + raise RuntimeError( + f'Service {service_name!r} is bound to both the ' + f'{names[service_name]!r} and {command_name!r} commands.' + ) + names[service_name] = command_name + if not names: + raise RuntimeError( + 'The top-level command table has no ServiceCommand entries, so ' + 'service commands cannot be looked up by service name.' + ) + return names + + +@pytest.fixture(scope='session') +def iter_operation_commands(): + # Session scoped so that module-scoped fixtures can depend on it. + + def iter_commands(driver, command_name, service_name): + """Yield (command_name, sub_name, sub_command, operation_model) for + every model-backed sub-command of a top-level CLI command, after + checking that the command is bound to the given botocore service.""" + command = driver.create_help_command().command_table[command_name] + actual = command.service_model.service_name + if actual != service_name: + raise RuntimeError( + f'CLI command {command_name!r} is bound to service ' + f'{actual!r}, not {service_name!r}' + ) + sub_table = command.create_help_command().command_table + for sub_name, sub_command in sub_table.items(): + obj = sub_command.create_help_command().obj + if isinstance(obj, OperationModel): + yield command_name, sub_name, sub_command, obj + + return iter_commands + + +@pytest.fixture(autouse=True) +def validates_models_properties_are_unique(request): + # An internal build system parses the JUnitXML report of validates_models + # tests and needs one aws_service/aws_operation/shape set per testcase, so + # a property recorded twice in one test would misattribute a failure. + yield + if request.node.get_closest_marker('validates_models') is None: + return + counts = collections.Counter( + name for name, _ in request.node.user_properties + ) + duplicates = sorted(name for name, count in counts.items() if count > 1) + if duplicates: + pytest.fail( + 'validates_models tests must record each property at most once ' + f'per testcase; duplicates: {duplicates}' + ) + + @pytest.fixture(autouse=True) def clear_loggers(): """Ensure all loggers have no residual state before test runs diff --git a/tests/functional/autocomplete/test_completion_files.py b/tests/functional/autocomplete/test_completion_files.py index 21c83eb56089..e04255f5d7e7 100644 --- a/tests/functional/autocomplete/test_completion_files.py +++ b/tests/functional/autocomplete/test_completion_files.py @@ -19,6 +19,7 @@ from awscli import clidriver from awscli.botocore.model import ServiceModel from awscli.botocore.utils import ArgumentGenerator +from tests import ALL_SERVICES COMPLETIONS_SCHEMA = { "type": "object", @@ -111,32 +112,43 @@ def service_test_data(request): return ServiceTestData(service_model, completions, service_name) -def _get_invalid_completion_operations(service_test_data): +def _get_invalid_completion_operations(): cases = [] - known_ops = set(service_test_data.service_model['operations']) - for op_name in service_test_data.completions['operations']: - if op_name not in known_ops: - cases.append(op_name) - resources = service_test_data.completions.get('resources', {}) - for resource in resources.values(): - op_name = resource.get('operation') - if op_name and op_name not in known_ops: - cases.append(op_name) + known = {model.service_name: model for model in ALL_SERVICES} + session = clidriver.create_clidriver().session + loader = session.get_component('data_loader') + for service_name in loader.list_available_services('completions-1'): + service_model = known[service_name] + completions = loader.load_service_model( + service_name, 'completions-1', service_model.api_version + ) + known_ops = set(service_model.operation_names) + for op_name in completions['operations']: + if op_name not in known_ops: + cases.append((service_name, op_name)) + for resource in completions.get('resources', {}).values(): + op_name = resource.get('operation') + if op_name and op_name not in known_ops: + cases.append((service_name, op_name)) return cases @pytest.mark.validates_models +@pytest.mark.parametrize( + 'service_name, operation_name', + _get_invalid_completion_operations(), + ids=lambda val: str(val), +) def test_completions_operations_exist_in_model( - service_test_data, record_property + service_name, operation_name, record_property ): - invalid_operations = _get_invalid_completion_operations(service_test_data) - record_property('aws_service', service_test_data.service_name) - for operation_name in invalid_operations: - record_property('aws_operation', operation_name) - assert not invalid_operations, ( - f"Completions file for '{service_test_data.service_name}' references " - f"operations absent from the service model: {invalid_operations}. " - "The completions-1.json file must remove or update these references." + record_property('aws_service', service_name) + record_property('aws_operation', operation_name) + pytest.fail( + f"Completions file for '{service_name}' references operation " + f"'{operation_name}' which does not exist in the service model. " + "The completions-1.json file must be updated to remove or " + "update references to this operation." ) diff --git a/tests/functional/test_no_event_streams.py b/tests/functional/test_no_event_streams.py index 2692b76daeb7..ad42edd03b26 100644 --- a/tests/functional/test_no_event_streams.py +++ b/tests/functional/test_no_event_streams.py @@ -11,55 +11,86 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import pytest -from botocore.model import OperationModel -from awscli.clidriver import create_clidriver +from tests import ALL_SERVICES # Excluded commands must be registered in awscli/customizations/removals.py _ALLOWED_COMMANDS = ['s3api select-object-content'] -def _generate_command_tests(driver, command_name): - help_command = driver.create_help_command() - command_obj = help_command.command_table[command_name] - sub_help = command_obj.create_help_command() - if hasattr(sub_help, 'command_table'): - for sub_name, sub_command in sub_help.command_table.items(): - op_help = sub_command.create_help_command() - model = op_help.obj - if isinstance(model, OperationModel): - if ( - not model.has_event_stream_input - and not model.has_event_stream_output - ): - continue - model_description = { - 'service_name': model.service_model.service_name, - 'name': model.name, - } - yield command_name, sub_name, model_description +def _has_event_stream_member(service_model): + # Checking every operation resolves and caches its input and output + # shapes on ALL_SERVICES, which every xdist worker would then hold for + # the whole run. Shapes from shape_for are not cached, so use them to + # skip services that have no event stream member at all. + return any( + service_model.shape_for(shape_name).event_stream_name + for shape_name in service_model.shape_names + ) + + +def _event_stream_operations(): + operations = [] + for service_model in ALL_SERVICES: + if not _has_event_stream_member(service_model): + continue + for operation_name in service_model.operation_names: + operation_model = service_model.operation_model(operation_name) + if ( + operation_model.has_event_stream_input + or operation_model.has_event_stream_output + ): + operations.append((service_model.service_name, operation_name)) + return operations + + +_EVENT_STREAM_OPERATIONS = _event_stream_operations() @pytest.mark.validates_models @pytest.mark.parametrize( - 'command_name', - tuple(create_clidriver().create_help_command().command_table), + 'service_name, operation_name', + _EVENT_STREAM_OPERATIONS, + ids=[ + f'{service_name}-{operation_name}' + for service_name, operation_name in _EVENT_STREAM_OPERATIONS + ], ) def test_no_event_stream_unless_allowed( - command_name, cli_driver, record_property + service_name, + operation_name, + service_command_names, + iter_operation_commands, + cli_driver, + record_property, ): - disallowed = [] - for _, sub_name, model in _generate_command_tests( - cli_driver, command_name - ): - full_command = f'{command_name} {sub_name}' - record_property('aws_service', model['service_name']) - record_property('aws_operation', model['name']) - if full_command not in _ALLOWED_COMMANDS: - disallowed.append(full_command) + command_name = service_command_names.get(service_name) + if command_name is None: + # The CLI does not expose this service. + return + # An operation can be exposed under more than one name when a + # customization keeps a hidden alias, so check every command for it. + commands = iter_operation_commands(cli_driver, command_name, service_name) + full_commands = [ + f'{command_name} {sub_name}' + for _, sub_name, _, operation_model in commands + if operation_model.name == operation_name + ] + if not full_commands: + # The CLI does not expose this operation: it is removed in + # removals.py or replaced by a customization. + return + # Store the service and operation in PyTest custom properties + record_property('aws_service', service_name) + record_property('aws_operation', operation_name) + disallowed = [ + full_command + for full_command in full_commands + if full_command not in _ALLOWED_COMMANDS + ] supported_commands = '\n'.join(_ALLOWED_COMMANDS) assert not disallowed, ( - f'The {", ".join(disallowed)} commands use event streams ' + f'The {", ".join(disallowed)} command uses event streams ' 'which is only supported for these operations:\n' f'{supported_commands}' ) diff --git a/tests/functional/test_shadowing.py b/tests/functional/test_shadowing.py index f7a13b45f6ab..cbf34c61c518 100644 --- a/tests/functional/test_shadowing.py +++ b/tests/functional/test_shadowing.py @@ -11,40 +11,72 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import pytest -from botocore.model import OperationModel from awscli.clidriver import create_clidriver +from tests import ALL_SERVICES -def _generate_command_tests(driver, command_name): - help_command = driver.create_help_command() - top_level_params = set(driver.create_help_command().arg_table.keys()) - command_obj = help_command.command_table[command_name] - sub_help = command_obj.create_help_command() - if hasattr(sub_help, 'command_table'): - for sub_name, sub_command in sub_help.command_table.items(): - op_help = sub_command.create_help_command() - model = op_help.obj - arg_table = op_help.arg_table - if not isinstance(model, OperationModel): - continue - yield ( - command_name, - sub_name, - model.service_model.service_name, - model.name, - tuple(arg_table), - top_level_params, +def _all_operations(): + return [ + (model.service_name, name) + for model in ALL_SERVICES + for name in model.operation_names + ] + + +class _ShadowingIndex: + def __init__(self, command_names, iter_operation_commands): + self.builtins = set(create_clidriver().create_help_command().arg_table) + self._command_names = command_names + self._iter_operation_commands = iter_operation_commands + self._services = {} + + def lookup(self, service_name, operation_name): + # Returns a list of (command_name, sub_name, arg_names) for every + # command exposing the operation, which is empty when the CLI does + # not expose it. A customization can keep a hidden alias, so there + # may be more than one. + if service_name not in self._services: + self._services[service_name] = self._index_service(service_name) + return self._services[service_name].get(operation_name, []) + + def _index_service(self, service_name): + command_name = self._command_names.get(service_name) + if command_name is None: + return {} + # A fresh driver per service keeps command tables from accumulating + # across services. Only strings are kept once it is discarded. + driver = create_clidriver() + operations = {} + commands = self._iter_operation_commands( + driver, command_name, service_name + ) + for _, sub_name, sub_command, operation_model in commands: + operations.setdefault(operation_model.name, []).append( + (command_name, sub_name, tuple(sub_command.arg_table)) ) + return operations + + +@pytest.fixture(scope='module') +def shadowing_index(service_command_names, iter_operation_commands): + return _ShadowingIndex(service_command_names, iter_operation_commands) + + +_ALL_OPERATIONS = _all_operations() @pytest.mark.validates_models @pytest.mark.parametrize( - 'command_name', - tuple(create_clidriver().create_help_command().command_table), + 'service_name, operation_name', + _ALL_OPERATIONS, + ids=[ + f'{service_name}-{operation_name}' + for service_name, operation_name in _ALL_OPERATIONS + ], ) def test_no_shadowed_builtins( - command_name, cli_driver, record_property + service_name, operation_name, shadowing_index, record_property ): """Verify no command params are shadowed or prefixed by the built-in param. @@ -66,26 +98,29 @@ def test_no_shadowed_builtins( through every command table and ensure we're not shadowing any builtins. - Also, rather than being a test generator, we're going to just - aggregate all the failures in one pass and surface them as - a single test failure. + Each test case covers a single operation, so a failure reports + every shadowed option of that operation and records exactly one + service and operation. """ + exposed = shadowing_index.lookup(service_name, operation_name) + if not exposed: + # The CLI does not expose this operation. + return + # Store the service and operation in PyTest custom properties + record_property('aws_service', service_name) + record_property('aws_operation', operation_name) errors = [] - for case in _generate_command_tests(cli_driver, command_name): - _, sub_name, service_name, operation_name, arg_table, builtins = case + for command_name, sub_name, arg_names in exposed: shadowed = [ arg_name - for arg_name in arg_table - if any(p.startswith(arg_name) for p in builtins) + for arg_name in arg_names + if any(p.startswith(arg_name) for p in shadowing_index.builtins) ] - if shadowed: - record_property('aws_service', service_name) - record_property('aws_operation', operation_name) - errors.extend( - 'Shadowing/Prefixing a top level option: ' - f'{command_name}.{sub_name}.{arg_name}' - for arg_name in shadowed - ) + errors.extend( + 'Shadowing/Prefixing a top level option: ' + f'{command_name}.{sub_name}.{arg_name}' + for arg_name in shadowed + ) if errors: raise AssertionError('\n' + '\n'.join(errors)) From 94d7a4ba3bbc9a51a8fdf3c9305d0ac8cecb0f8c Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Tue, 3 Nov 2026 15:39:49 +0000 Subject: [PATCH 5/5] Reduce time and memory of example validation tests The example style checks added recently made test_examples.py run about four times longer and use about 600 MB more memory. Most of the cost came from the validation setup, not from the checks themselves. With this change, running test_examples.py in a single process went from 5:00 to 1:10 wall clock and from 1,543 MB to 699 MB max RSS on a 2-CPU Linux machine. That is about 4.3x faster and less than half the memory, and below what the file used before the style checks were added. Each `--cli-input-json`, `--cli-input-yaml` and `--generate-cli-skeleton` argument registers its required-args handler for the event of every command when its argument table is built. The CLI builds one argument table per run, but these tests build thousands, so the stale handlers piled up and each `emit` walked millions of them. `_cli_required_args` now unregisters them and calls only the current command's handlers directly, which matches what the CLI sees. The `command_table` and `command_validator` fixtures each created a driver, and each driver loaded every service model it touched, so every model was held in memory twice for the whole module. `ServiceCommandTable` now loads one service at a time in a fresh driver and releases the previous one. The examples are grouped by service directory and very few refer to another service, so drivers are rarely rebuilt. The ten per-rule style tests are merged into one `test_example_style` per file, so each file is read and block-parsed once instead of ten times. It still runs every rule and reports every rule that fails, so coverage is unchanged, but failures no longer have separate per-rule test IDs. Two smaller costs are gone as well. The docutils `OptionParser` was rebuilt for every file, which took longer than parsing most examples, so it is now cached. A module-scoped `gc.freeze()` stops full garbage collections from rescanning the roughly one million objects left behind by test collection. --- tests/functional/docs/test_examples.py | 204 ++++++++++++++----------- 1 file changed, 115 insertions(+), 89 deletions(-) diff --git a/tests/functional/docs/test_examples.py b/tests/functional/docs/test_examples.py index 12d2a7410e16..e67e4486539e 100644 --- a/tests/functional/docs/test_examples.py +++ b/tests/functional/docs/test_examples.py @@ -22,7 +22,10 @@ """ +import collections.abc import contextlib +import functools +import gc import json import os import re @@ -35,6 +38,7 @@ import pytest from botocore.validate import ParamValidator +import awscli.clidriver from awscli.argparser import ( ArgTableArgParser, MainArgParser, @@ -42,6 +46,7 @@ SubCommandArgParser, ) from awscli.clidriver import ServiceOperation +from awscli.customizations.arguments import OverrideRequiredArgsArgument from awscli.customizations.waiters import WaitCommand from awscli.testutils import BaseAWSHelpOutputTest, create_clidriver @@ -1028,17 +1033,25 @@ def extract_error_line(content, error_start, error_end): return content[error_line_begin:error_line_end] -@pytest.fixture(scope="module") -def command_validator(): - # CLIDriver can take up a lot of resources so we'll just create one - # instance and use it for all the validation tests. - driver = create_clidriver() - return CommandValidator(driver) +@pytest.fixture(scope="module", autouse=True) +def freeze_collected_objects(): + # About a million objects, mostly the collected tests, exist before the + # first test here and outlive the module. Freezing them stops every + # full garbage collection from scanning them again. + gc.collect() + gc.freeze() + yield + gc.unfreeze() @pytest.fixture(scope="module") def command_table(): - return create_clidriver().subcommand_table + return ServiceCommandTable() + + +@pytest.fixture(scope="module") +def command_validator(command_table): + return CommandValidator(create_clidriver(), command_table) @pytest.mark.parametrize("command, subcommand", EXAMPLE_COMMAND_TESTS) @@ -1107,12 +1120,17 @@ def verify_is_valid_rst(filename, contents): return document +@functools.cache +def _rst_option_parser(): + # Creating the option parser takes longer than parsing most examples. + return docutils.frontend.OptionParser( + components=(docutils.parsers.rst.Parser,) + ) + + def parse_rst(contents): parser = docutils.parsers.rst.Parser() - components = (docutils.parsers.rst.Parser,) - settings = docutils.frontend.OptionParser( - components=components - ).get_default_values() + settings = _rst_option_parser().get_default_values() document = docutils.utils.new_document('', settings=settings) errors = [] @@ -1166,14 +1184,51 @@ def find_all_cli_commands(document): return visitor.cli_commands +class ServiceCommandTable(collections.abc.Mapping): + """The CLI's top-level command table, with one service loaded at a time. + + A driver keeps the model of every service it loads, which adds up to + hundreds of MB for all of them, and so does the loader that the drivers + from ``awscli.testutils`` share. The examples are checked one service + at a time, so each service is looked up in a new driver with its own + loader, and the previous driver is released. + """ + + def __init__(self): + self._names = set(awscli.clidriver.create_clidriver().subcommand_table) + self._service_name = None + self._driver = None + + def driver(self, service_name): + if service_name != self._service_name: + self._driver = awscli.clidriver.create_clidriver() + self._service_name = service_name + return self._driver + + def __getitem__(self, name): + if name not in self._names: + raise KeyError(name) + return self.driver(name).subcommand_table[name] + + def __contains__(self, name): + return name in self._names + + def __iter__(self): + return iter(self._names) + + def __len__(self): + return len(self._names) + + class CommandValidator: - def __init__(self, driver): - self.driver = driver - help_command = self.driver.create_help_command() - self._service_command_table = help_command.command_table + def __init__(self, driver, command_table): + # The driver only parses the global arguments and the service name. + # The commands of the service come from command_table. + help_command = driver.create_help_command() + self._command_table = command_table self._global_arg_table = help_command.arg_table self._main_parser = MainArgParser( - self._service_command_table, + help_command.command_table, driver.session.user_agent(), 'Some description', self._global_arg_table, @@ -1231,7 +1286,8 @@ def validate_cli_command(self, command, filename): service_name = parsed_args.command # Get the service command - service_cmd = self._service_command_table[service_name] + service_cmd = self._command_table[service_name] + session = self._command_table.driver(service_name).session # Create the service parser cmd_table = service_cmd.create_help_command().command_table @@ -1263,7 +1319,7 @@ def record_message(message, file=None): try: while True: - with self._cli_required_args(command_obj, args): + with self._cli_required_args(command_obj, args, session): subcommand = None if command_obj.subcommand_table: subcommand_parser = SubCommandArgParser( @@ -1311,7 +1367,7 @@ def record_message(message, file=None): ) @contextlib.contextmanager - def _cli_required_args(self, command_obj, args): + def _cli_required_args(self, command_obj, args, session): """Apply the CLI's rules for which arguments are required. Arguments such as ``--cli-input-json`` and ``--generate-cli-skeleton`` @@ -1328,13 +1384,26 @@ def _cli_required_args(self, command_obj, args): if isinstance(command_obj, ServiceOperation): lineage_names = lineage_names[-2:] required = {name: arg.required for name, arg in arg_table.items()} + # Arguments such as --cli-input-json register their handler for the + # event of every command when their argument table is built. The + # CLI builds one argument table, but these tests build thousands, and + # the handlers left behind would make every emit slower. So they + # are unregistered, and only this command's handlers are called. + event = 'before-building-argument-table-parser' + handlers = [ + arg.override_required_args + for arg in arg_table.values() + if isinstance(arg, OverrideRequiredArgsArgument) + ] try: - self.driver.session.emit( - 'before-building-argument-table-parser.' - + '.'.join(lineage_names), + for handler in handlers: + session.unregister(event, handler) + handler(argument_table=arg_table, args=args) + session.emit( + f'{event}.' + '.'.join(lineage_names), argument_table=arg_table, args=args, - session=self.driver.session, + session=session, ) yield finally: @@ -1383,73 +1452,30 @@ def _assert_name_contains_only_allowed_characters(filename): @pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_whitespace(example_file): - example = Example(example_file) - errors = check_whitespace(example) - _assert_follows_rule('whitespace', example, errors) - - -@pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_location(command_table, example_file): - example = Example(example_file) - errors = check_location(example, command_table) - _assert_follows_rule('location', example, errors) - - -@pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_title(example_file): - example = Example(example_file) - errors = check_title(example) - _assert_follows_rule('title', example, errors) - - -@pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_description(example_file): - example = Example(example_file) - errors = check_description(example) - _assert_follows_rule('description', example, errors) - - -@pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_command(example_file): - example = Example(example_file) - errors = check_command(example) - _assert_follows_rule('command', example, errors) - - -@pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_command_name(example_file): - example = Example(example_file) - errors = check_command_name(example) - _assert_follows_rule('command-name', example, errors) - - -@pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_file_contents(example_file): +def test_example_style(command_table, example_file): + # All the style rules are checked in one test per file, so each file is + # read and parsed once, and the test count stays at one per file. example = Example(example_file) - errors = check_file_contents(example) - _assert_follows_rule('file-contents', example, errors) - - -@pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_output(command_table, example_file): - example = Example(example_file) - errors = check_output(example, command_table) - _assert_follows_rule('output', example, errors) - - -@pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_link(example_file): - example = Example(example_file) - errors = check_link(example) - _assert_follows_rule('link', example, errors) - - -@pytest.mark.parametrize('example_file', STYLE_EXAMPLE_FILES) -def test_example_sanitization(example_file): - example = Example(example_file) - errors = check_sanitization(example) - _assert_follows_rule('sanitization', example, errors) + errors_by_rule = { + 'whitespace': check_whitespace(example), + 'location': check_location(example, command_table), + 'title': check_title(example), + 'description': check_description(example), + 'command': check_command(example), + 'command-name': check_command_name(example), + 'file-contents': check_file_contents(example), + 'output': check_output(example, command_table), + 'link': check_link(example), + 'sanitization': check_sanitization(example), + } + failures = [] + for rule, errors in errors_by_rule.items(): + try: + _assert_follows_rule(rule, example, errors) + except AssertionError as e: + failures.append(str(e)) + if failures: + raise AssertionError('\n'.join(failures)) def test_example_style_exceptions_exist():