Conversation
This reverts commit 0e3b7b7.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical regressions affect generated suite preservation, fixture handling, test coverage, and output validation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (7)
What changed in this PR
Reverts the functional Python SDK emitter implementation to scaffold-only output.
Changes:
- Restores placeholder Python test generation.
- Removes executable emitter tests, fixture materialization, and runtime configuration.
- Reverts operation-map validation and related invariants.
| File | Summary |
|---|---|
tests/codegen/python-sdk-emitter.test.ts |
Regression coverage now masks missing executable output. |
materializer/src/python-sdk/sdk-mapping.ts |
has() may report unusable mappings as present. |
materializer/src/python-sdk/README.md |
Documentation claims validation that is not implemented. |
materializer/src/python-sdk/materialize-support.ts |
Removes fixture support and environment-driven configuration. |
materializer/src/python-sdk/emitter.ts |
Mode-independent filenames can overwrite generated suites. |
materializer/src/index.ts |
Stops materializing Python fixtures. |
configs/camunda-oca/regression-invariants.test.ts |
Invariant incorrectly rejects ${RANDOM} literals. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // `jobType_${RANDOM}` literals) — it is not a missing-binding bug. | ||
| const placeholders = src.match(/\$\{[^}]+\}/g) ?? []; | ||
| if (placeholders.some((p) => p !== '${RANDOM}')) { | ||
| if (/\$\{[^}]+\}/.test(src)) { |
Comment on lines
+14
to
+20
| export function pythonSuiteFileName(collection: EndpointScenarioCollection): string { | ||
| const operationId = collection.endpoint.operationId; | ||
| const snakeCase = toSnakeCase(operationId); | ||
| // The mode suffix is omitted for the default `feature` mode to preserve | ||
| // existing file names. Without it, a `variant` collection for the same | ||
| // operationId (see materializer/src/index.ts's feature + variant | ||
| // emission passes) silently overwrote the feature suite -- both resolved | ||
| // to the identical `test_<op>.py` path in the same output directory. | ||
| // Mirrors the JS/C# emitters' mode-suffix handling. | ||
| const modeSuffix = mode !== 'feature' ? `_${toSnakeCase(mode)}` : ''; | ||
| return `test_${snakeCase}${modeSuffix}.py`; | ||
| const snakeCase = operationId | ||
| .replace(/([A-Z])/g, '_$1') | ||
| .toLowerCase() | ||
| .replace(/^_/, ''); | ||
| return `test_${snakeCase}.py`; |
Comment on lines
+8
to
10
| import { promises as fs } from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import type { EmittedFile } from '@camunda8/emitter-sdk'; |
Comment on lines
162
to
+165
| test('test functions include operation steps', () => { | ||
| const output = renderPythonSuite(SAMPLE_COLLECTION); | ||
| expect(output).toContain('# Step 1: createWidget'); | ||
| }); | ||
|
|
||
| test('uses requestPlan for executable step emission (no TODO placeholders)', () => { | ||
| const collection: EndpointScenarioCollection = { | ||
| ...SAMPLE_COLLECTION, | ||
| scenarios: [ | ||
| { | ||
| ...SAMPLE_COLLECTION.scenarios[0], | ||
| operations: [{ operationId: 'placeholderOp', method: 'GET', path: '/placeholder' }], | ||
| requestPlan: [ | ||
| { | ||
| operationId: 'createWidget', | ||
| method: 'POST', | ||
| pathTemplate: '/widgets/{widgetKey}', | ||
| pathParams: [{ name: 'widgetKey', var: 'widgetKeyVar' }], | ||
| bodyKind: 'json', | ||
| bodyTemplate: { | ||
| enabled: true, | ||
| archived: false, | ||
| owner: null, | ||
| }, | ||
| expect: { status: 201 }, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| const output = renderPythonSuite(collection); | ||
|
|
||
| expect(output).toContain('# Step 1: createWidget'); | ||
| // #354: ctx key must be the planner's original binding name (widgetKeyVar), | ||
| // matching whatever ctx.set(...) would use for the same binding. | ||
| // Leading '/' is stripped so the URL resolves as relative against the | ||
| // httpx client's base_url path segment (e.g. '/v2') instead of | ||
| // replacing it -- see conftest.py's client fixture. | ||
| expect(output).toContain( | ||
| 'url_1 = f\'widgets/{ctx.get("widgetKeyVar") if ctx.get("widgetKeyVar") is not None else "widgetKey"}\'', | ||
| ); | ||
| expect(output).toContain("body_1 = {'enabled': True, 'archived': False, 'owner': None}"); | ||
| expect(output).toContain('response_1 = await client.post('); | ||
| expect(output).toContain('assert response_1.status_code == 201'); | ||
| expect(output).not.toContain('placeholderOp'); | ||
| expect(output).not.toContain('pass # TODO: implement'); | ||
| expect(output).toContain('pass # TODO: implement'); |
Comment on lines
230
to
+232
| async with httpx.AsyncClient( | ||
| base_url=base_url, | ||
| timeout=timeout_seconds, | ||
| headers=headers, | ||
| base_url="http://localhost:8080/v2", | ||
| timeout=30.0, |
Comment on lines
131
to
+132
| has(operationId: string): boolean { | ||
| return this.lookup(operationId) !== undefined; | ||
| return operationId in this.map; |
Comment on lines
+202
to
+205
| When present, the emitter can validate: | ||
| - SDK method coverage (which operations have SDK bindings?) | ||
| - Expected method signatures | ||
| - Breaking changes in new SDK versions |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts #574