Skip to content

test(tests): re-enable the Python edge integration test - #404

Open
jsteinich wants to merge 2 commits into
open-constructs:mainfrom
jsteinich:test/unskip-python-edge
Open

test(tests): re-enable the Python edge integration test#404
jsteinich wants to merge 2 commits into
open-constructs:mainfrom
jsteinich:test/unskip-python-edge

Conversation

@jsteinich

@jsteinich jsteinich commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Related issue

Follow-up from the review on #400, point 2. #400 is closed; this is the durable half of its regression coverage.

Description

test/python/edge/test.ts has been describe.skip'd since 3acb935 (1 December 2022), waiting on aws/jsii#3866, with the stated intent to "re-add them when updating JSII". That fix shipped shortly after, and the JSII update has since landed (#395: jsii 6.0, jsii-pacmak 1.140.0) — but the skip was never lifted.

Python is the only language whose edge test does not run. TypeScript, Go, Java and C# all do.

That gap has a concrete cost: it is why #311's cross-language compile coverage never saw the broken Python provider import, which only surfaced once CI could reach Terraform >= 1.8 (#398) — and then via the python-documentation example rather than a test aimed at it.

main.py calls edge.provider.EdgeProvider(...), so beforeAll's synth imports the generated edge.provider module — exactly the module whose __init__.py carried the unresolvable import. A regression there fails the suite loudly, at the import.

Two stale assertions, and why they were wrong

Re-enabling produced 18 passed / 2 failed. The suite is substantially intact after three years dormant; both failures were expectations that had never once executed.

1. A typo. The numList expectation read ...reqnum)} — a stray closing paren — where synth produces ...reqnum}.

2. A wrong assumption about Fn.lookup. The map expectations assumed every Fn.lookup(map, key, default) renders as a Terraform lookup(). It does not. Fn.lookup (packages/cdktn/src/terraform-functions.ts:41) emits lookup() only when the default is truthy:

static lookup(inputMap: any, key: string, defaultValue?: any) {
  if (defaultValue) return Fn._lookup(inputMap, key, [defaultValue]);
  return asAny(propertyAccess(inputMap, [key])); // -> renders inputMap[key]
}

The fixture passes the same Fn.lookup(...) call for all six attributes, with different defaults:

attribute default truthy renders as
reqMap false no ${map_resource.map.reqMap.key1}
optMap "missing" yes ${lookup(map_resource.map.optMap, "key1", "missing")}
computedMap 0 no ${map_resource.map.computedMap.key1}

So the deciding factor is the truthiness of the default, not whether the attribute is required, optional or computed. The corrected expectations match, and are now identical to test/typescript/edge/test.ts.

Only the first failing assertion in each block is reported by jest, so the rest of the map block was corrected at the same time rather than surfacing them one CI run at a time.

These pin current behaviour, not necessarily correct behaviour (#416)

Worth flagging rather than burying: that truthiness guard looks like a bug. Fn.lookup(map, "key", false) reads as "return false if the key is missing", but the falsy default is discarded and the emitted expression becomes a bare property access — which errors on a missing key instead of returning the default. The guard should almost certainly be defaultValue !== undefined.

Filed as #416. I have deliberately not changed it here. This PR is test coverage; altering Fn.lookup is a behavioural change to the construct library with its own blast radius, and it would be wrong to smuggle it in. If it is fixed, these expectations move with it — and at that point the edge tests will be the thing that proves the fix, which is rather the point of un-skipping them.

Note this also means the equivalent TypeScript assertions were changed to match this behaviour in 09ac17b ("chore: fix assertions", Aug 2023) with no rationale recorded.

Testing

CI on this branch: 257 pass, 0 fail, 1 skip (windows_integration, if: false). Both python/edge jobs pass, on Terraform 1.5.7 and 1.16.1.

🤖 Generated with Claude Code

Skipped in 3acb935 (Dec 2022) waiting on aws/jsii#3866, with the intent to
"re-add them when updating JSII". That fix shipped long ago and the JSII
update has now landed (open-constructs#395: jsii 6.0, jsii-pacmak 1.140.0), but the skip was
never lifted. Python is the only language whose edge test does not run -
typescript, go, java and csharp all do.

That gap has cost us: it is why open-constructs#311's cross-language compile coverage never
saw the broken Python provider import that only surfaced once CI could reach
Terraform >= 1.8 (open-constructs#398), via the python-documentation example rather than a
test aimed at it.

The test is a real guard for that class of bug rather than a hopeful one.
`main.py` calls `edge.provider.EdgeProvider(...)`, so `beforeAll`'s synth
imports the generated `edge.provider` module - exactly the module whose
`__init__.py` carried the unresolvable import. A regression there fails the
whole suite loudly.

The fixture is not stale: `main.py` defines the same reference/provider/
iterator stacks as the TypeScript edge fixture, and the helpers it imports
(`QueryableStack`, `TestDriver`, `onlyJson`) are the ones the other language
edge tests use.

Verified on main that the underlying defect is gone: the regenerated
edge-provider bindings now emit `_LazyImport("edge.provider_functions")` in
`edge/provider/__init__.py`, and all 21 relative imports in the generated
Python resolve.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI on the re-enabled suite came back 18 passed / 2 failed. Both failures were
stale expectations in the test, not product bugs - these assertions were
written before Dec 2022 and have never executed since.

1. The numList expectation carried a stray closing paren: `...reqnum)}` where
   the synthesized value is `...reqnum}`.

2. The map expectations assumed every `Fn.lookup(map, key, default)` renders
   as a Terraform `lookup()`. It does not. `Fn.lookup`
   (packages/cdktn/src/terraform-functions.ts:41) emits `lookup()` only when
   the default value is *truthy*:

       if (defaultValue) return Fn._lookup(inputMap, key, [defaultValue]);
       return asAny(propertyAccess(inputMap, [key]));

   The fixture passes `false` for reqMap and `0` for computedMap - both falsy
   - so those render as property access (`map_resource.map.reqMap.key1`),
   while optMap's `"missing"` default keeps its `lookup()`. That rule, not
   anything about required vs optional vs computed attributes, is what
   decides the form.

The corrected expectations are identical to test/typescript/edge/test.ts,
which has been running throughout. Only the first failing assertion in each
block was reported, so the rest of the map block was corrected at the same
time rather than surfacing them one CI run at a time.

These pin current behaviour, which is not obviously desirable behaviour: the
truthiness guard silently discards a legitimate `false` or `0` default, so
the emitted expression loses its fallback and errors on a missing key instead
of returning the default. Raised separately; if that is fixed, these
expectations move with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant