tests: resolve sam init menu positions dynamically in schemas tests - #9151
tests: resolve sam init menu positions dynamically in schemas tests#9151roger-zhangg wants to merge 6 commits into
Conversation
test_init_interactive_with_event_bridge_app_aws_schemas_python has been failing (see run 30657529336). It answered the runtime prompt with "8" and its comment said "8: Python 3.9", but the prompt offers 15 runtimes for the Infrastructure event management use case and position 8 is now java8.al2023. python3.9 has drifted to position 10. The test therefore selected a Java runtime and fed Python-flow answers into a Java prompt sequence: every later answer landed on the wrong question, and it finally asserted on hello_world_function/schema, a Python layout a Java project never produces. The positions moved when java8.al2023/java11.al2023/java17.al2023 were added (#9125). Nothing about the runtime list is stable, so hardcoding an index means any future runtime addition silently repoints these tests at a different runtime -- a failure that looks like a product bug rather than a stale fixture. Adds _get_runtime_position and _get_use_case_position, which look positions up from the same source the prompt uses (InitTemplates(). get_preprocessed_manifest plus get_sorted_runtimes), mirroring the existing _get_registry_position helper. All 8 tests in the file now resolve the template/use-case/runtime answers dynamically; the 4 Java tests and the go test were relying on the same brittle indexing and only passed by luck. Resolved positions are unchanged for Java (4) and go (1), and corrected from 8 to 10 for python3.9. Stale position comments updated to name the runtime rather than a number. Testing: make pr passes (9347 passed, 25 skipped, coverage 94.09%). Verified the helpers resolve to use case 8, java17.al2023 4, python3.9 10, go (provided.al2) 1, and rendered each test's user_input to confirm the answer sequences.
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Results
Reviewed: d8dc699..e386dcd
Files: 1
Comments: 2
Comments on lines outside the diff:
[tests/integration/init/schemas/test_init_with_schemas_command.py:326] [GENERAL] This test's registry answer is still a hardcoded 1, and it contradicts its own comment block, which says # 4: select aws.events as registries. The sibling tests in this file resolve it with _get_registry_position("aws.events"), which sorts the account's registries alphabetically — the same drift problem the new runtime/use-case helpers address, just for registries.
If the intent is to init against aws.events, the answer should be resolved the same way as elsewhere in the file:
aws_registry_pos = getregistry_position("aws.events")
user_input = f"""
1
{_get_use_case_position(EVENT_BRIDGE_USE_CASE)}
{_get_runtime_position("go (provided.al2)")}
...
{aws_registry_pos}
1
"""If instead the test genuinely intends "whichever registry is first," the stale # 4 comment should be corrected so the next person doesn't read it as a bug. The same hardcoded 1 appears in test_init_interactive_with_event_bridge_app_non_default_profile_selection. Note I could not verify the registry contents of the canary account, so I can't say whether this currently selects aws.events — only that the answer is position-dependent and unverified.
Review feedback on #9151. 1. Name the runtimes as constants (valerena) JAVA_RUNTIME_FOR_INIT, PYTHON_RUNTIME_FOR_INIT, GO_RUNTIME_FOR_INIT. None of these tests is about a specific runtime version -- they exercise the schemas flows (registry choice, pagination, profile selection) -- so the version is incidental and was repeated 4/3/1 times. python3.9 is already deprecated, so a one-line change rather than a hunt through the file is not hypothetical. The per-test comments now name the constant instead of the literal version, so changing a constant cannot leave a stale comment behind -- the same drift this PR exists to fix. Scoped to this file on purpose. The runtimes in test_init_command.py are passed as explicit `--runtime` CLI arguments, which are position-independent and fail loudly if removed; each of those tests deliberately pairs a runtime with a dependency manager (java21+maven, nodejs18.x+npm), so hoisting them into shared constants would hide what those tests are actually exercising. 2. Fetch the manifest once per process (review bot) Both helpers called InitTemplates().get_preprocessed_manifest() independently, and InitTemplates._get_manifest does requests.get(MANIFEST_URL, timeout=10) on every call, falling back to cloning the templates repo. Every test resolves both a use case and a runtime, so that was two fetches per test. It was also a correctness trap: resolved independently, one call could succeed against MANIFEST_URL while the other fell back to the bundled local manifest, giving a use-case position and a runtime position from two different snapshots -- the exact answer misalignment these helpers exist to prevent. Now behind lru_cache(maxsize=1). 3. Registry answers (review bot) The go test said "# 4: select aws.events as registries" but answered 1, while every sibling resolves it with _get_registry_position("aws.events"). Fixed to resolve it. Left ..._non_default_profile_selection hardcoded, and said why: it deliberately drives a non-default profile and an explicit us-east-1, so the prompt lists that profile/region's registries, while _get_registry_position resolves against the default Session(). Converting it would look up the wrong account and region. Testing: 8 tests collect. Resolved against the live manifest -- use case 8, java17.al2023 -> 4, python3.9 -> 10, go (provided.al2) -> 1 (unchanged except python3.9, which is the bug this PR fixes). Cache confirmed at 1 miss / 3 hits for four resolutions, i.e. one fetch instead of four. A removed runtime still raises ValueError listing what is available. black clean.
There was a problem hiding this comment.
Code Review Results
Reviewed: e1f4bf6..7035236
Files: 1
Comments: 1
Comments on lines outside the diff:
[tests/integration/init/schemas/test_init_with_schemas_command.py:413] [GENERAL] The input block for this test still contains a stray answer line that the new comment block doesn't account for. Two lines below this hunk, after eb-app-python39, the input is:
eb-app-python39
3
N
2
us-east-1
Walking the prompt sequence for a Python runtime, there is no prompt between the project-name prompt and the schemas profile confirm: _generate_from_use_case goes runtime → (no package-type/dependency-manager prompt, single option) → starter template (2) → tracing/insights/structured-logging (N N N) → click.prompt("\nProject name") → get_schemas_api_caller() → get_aws_configuration_choice(), whose first prompt is click.confirm("Do you want to use the default AWS profile [...] and region [...]?").
So 3 is fed to that confirm, which rejects it (Error: invalid input) and re-prompts, consuming N instead. The test passes only because click's re-prompt loop silently swallows the extra line — the same "answers are one position off but something still absorbs them" fragility this PR is removing. It also makes the input contradict the comment block, which lists N: Use default profile as the answer directly following the name.
Dropping the 3 makes the input match the documented prompt order:
eb-app-python39
N
2
us-east-1
Review feedback on #9151. The input for test_init_interactive_with_event_bridge_app_non_default_profile_selection carried an extra `3` after the project name, giving 14 answers against 13 documented prompts. The sibling test_init_interactive_with_event_bridge_app_aws_schemas_python is identical up to that point -- same runtime, use case, starter template and N/N/N -- and answers `Y` immediately after the project name, so the next prompt there is the AWS-config confirm, not a numbered choice. The `3` was absorbed rather than acted on. Confirmed against click directly: Use default AWS profile? [y/N]: 3 Error: invalid input Use default AWS profile? [y/N]: N <- re-prompt consumes the next line so the confirm rejected `3`, re-prompted, and ate the `N` intended for it. The test passed only because click's re-prompt loop swallowed the extra line -- the same "answers are one position off but something still absorbs them" fragility this PR removes, and it made the input contradict its own comment block. Testing: input now has 13 answers matching the 13 documented prompts. 8 tests collect; black clean.
Review feedback on #9151. Two tests fed one more answer than the schemas flow has prompts, so the registry position this PR resolves was not landing on the registry prompt at all. After the project name the flow asks, in order: 1. get_aws_configuration_choice() -- one click.confirm on the Y path 2. _get_registry_cli_choice -- one click.prompt per page render 3. _get_schema_cli_choice -- one click.prompt per page render do_paginate_cli issues exactly one prompt per render, returning either an item choice or N/P to page. Nothing sits between the confirm and the registry list, so the comment line "# 1: select schema from cli_paginator" did not correspond to a prompt. The other six tests agree on the shape. Four use `Y -> {registry_pos} -> <schema>`, and the pagination test uses `Y -> {registry_pos} -> N -> P -> 2`, both putting the resolved registry position directly after the confirm. Only these two wedged a literal 1 in between, which meant the literal answered the registry prompt and the resolved position answered the *schema* prompt, with the last line never consumed. It is masked today because aws.events sorts first among the registries these tests use (aws.events, discovered-schemas, other-schema, partner-registry, test-pagination), so _get_registry_position returns 1 and both readings pick the same items. Registry sorting is plain ASCII list.sort(), so any registry sorting before aws.events -- an uppercase-named one, for instance -- shifts the position, and the hardcoded 1 would then select the wrong registry while the resolved position was read as a schema index. That is the same drift this PR exists to remove, and the assertions here only check that a schema directory exists, so it would not be caught. Testing: all 8 tests now have an answer count matching their documented prompt list. 8 tests collect; black clean.
2 participants
Which issue(s) does this change fix?
N/A
Why is this change necessary?
test_init_interactive_with_event_bridge_app_aws_schemas_pythonis failing (run 30657529336).The test answered the runtime prompt with
8and its comment said# 8: Python 3.9. But the prompt offers 15 runtimes for the Infrastructure event management use case, and position 8 is nowjava8.al2023—python3.9has drifted to position 10:So the test selected a Java runtime and then fed Python-flow answers (
eb-app-python39, schema paginator choices) into a Java prompt sequence. Every later answer landed on the wrong question, and it finally asserted onhello_world_function/schema— a Python layout a Java project never produces.The positions moved when
java8.al2023/java11.al2023/java17.al2023were added (#9125).How does it address the issue?
Adds
_get_runtime_positionand_get_use_case_position, which resolve positions from the same source the prompt uses (InitTemplates().get_preprocessed_manifest(...)+get_sorted_runtimes) rather than hardcoding an index. This mirrors the existing_get_registry_positionhelper already in the file.All 8 tests now resolve template/use-case/runtime answers dynamically. The 4 Java tests and the go test were relying on the same brittle indexing and only passed by luck — a future runtime addition would have broken them the same way.
Resolved positions are unchanged for Java (4) and go (1), and corrected from 8 → 10 for
python3.9. Stale comments now name the runtime instead of a number.Changes from review feedback
Runtimes are now named constants (
JAVA_RUNTIME_FOR_INIT,PYTHON_RUNTIME_FOR_INIT,GO_RUNTIME_FOR_INIT). None of these tests is about a specific runtime version — they exercise the schemas flows — so the version was incidental and repeated 4/3/1 times.python3.9is already deprecated, so a one-line change rather than a hunt through the file is not hypothetical. The per-test comments now name the constant rather than the literal version, so changing a constant cannot leave a stale comment behind — the same drift this PR exists to fix.Scoped to this file deliberately: the runtimes in
test_init_command.pyare passed as explicit--runtimeCLI arguments, which are position-independent and fail loudly if removed, and each of those tests deliberately pairs a runtime with a dependency manager (java21+maven,nodejs18.x+npm). Hoisting those into shared constants would obscure what they exercise.The manifest is now fetched once per process (
lru_cache(maxsize=1)). Both helpers calledget_preprocessed_manifest()independently, andInitTemplates._get_manifestdoesrequests.get(MANIFEST_URL, timeout=10)on every call with a clone fallback — two fetches per test. It was also a correctness trap: resolved independently, one call could succeed againstMANIFEST_URLwhile the other fell back to the bundled local manifest, giving a use-case position and a runtime position from two different snapshots, which is the exact misalignment these helpers exist to prevent. Measured 1 miss / 3 hits for four resolutions.Registry answers. The go test's comment said
# 4: select aws.events as registriesbut it answered1, while every sibling resolves it via_get_registry_position("aws.events"); it now resolves it too...._non_default_profile_selectionstays hardcoded on purpose, and now says why: it drives a non-default profile and an explicitus-east-1, so the prompt lists that profile/region's registries, while_get_registry_positionresolves against the defaultSession()— converting it would query the wrong account and region.Removed a stray answer. That same test carried an extra
3after the project name — 14 answers against 13 documented prompts. The sibling..._aws_schemas_pythonis identical up to that point and answersYimmediately after the name, so the next prompt is the AWS-config confirm, not a numbered choice. Confirmed against click directly:The confirm rejected
3, re-prompted, and ate theNintended for it, so the test passed only because click's re-prompt loop swallowed the extra line — the same off-by-one fragility this PR removes, and it made the input contradict its own comment block. Now 13 answers matching 13 documented prompts.What side effects does this change have?
The helpers call
get_preprocessed_manifest, which these tests already exercise viasam inititself, so no new external dependency. If a runtime is ever removed from the manifest the helper raises with the available list, which fails loudly instead of silently selecting the wrong runtime.Note the same job also hit a separate pytest-xdist crash (
struct.error: 'i' format requires ... <= 2147483647— a worker tried to send a >2 GiB test report, killing the session and taking..._non_default_profile_selectionwith it). That is very likely a consequence of the misaligned answers producing runaway prompt output, so this fix should resolve it, but the 2 GiB report limit is a separate robustness issue not addressed here.Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.