fix(0.28.0): TestE2EStorageSwapTables must skip without E2E_API_TOKEN (CI fix for PR #249) - #250
Merged
Conversation
CI `pytest -m 'not integration'` does not deselect the `e2e` marker, so the class's setup fixture ran in the runner without credentials and crashed with KeyError: 'E2E_API_TOKEN'. Other E2E classes apply both `@skip_without_credentials` (skipif decorator on the class) AND `@pytest.mark.e2e` (marker for explicit `-m e2e` runs). PR #247 shipped only the marker, hence the rollup PR #249 broke CI. Verified locally: `pytest -m 'not integration'` now skips the 3 swap tests cleanly (2480 passed, 69 skipped, 0 errors).
7 tasks
padak
added a commit
that referenced
this pull request
May 6, 2026
…bles (#249) * fix(0.28.0): config update auto-normalizes script[] string -> array (#245) (#246) * fix(0.27.1): config update auto-normalizes parameters.blocks[].codes[].script string -> array (#245) The Storage API silently accepts a string for parameters.blocks[].codes[].script while the Keboola runtime validator requires an array (`Invalid type for path "root.parameters.blocks.0.codes.X.script". Expected "array", but got "string"`). The broken push lands as 200 OK, the version increments, the UI looks fine -- the crash happens only at job-run time, often hours later via the scheduler, with no attribution back to the offending write. Issue #245 was reported after a programmatic refactor crashed three production Snowflake transformations. kbagent config update now closes the gap on the write side. Defense-in-depth in our own command surface; bypassing kbagent (raw Storage API REST, keboola-mcp-server's update_sql_transformation / create_sql_transformation as of v1.59.x) does NOT inherit the normalization -- a separate upstream fix is recommended for the MCP server. ## Surfaces (all v0.27.1) - Helper: normalize_blocks_codes_script(component_id, config) -> (config, normalizations) in src/keboola_agent_cli/sync/code_extraction.py. SQL transformations get statement-level split via the existing split_statements() state machine (respects '...' / "..." / $$..$$ / -- / # / // / /* ... */); Python / R / kds-team.app-custom-python and any other component sharing the parameters.blocks[].codes[].script shape get a single-element [script] wrap. Already-array script values pass through unchanged. - Service: ConfigService.update_config calls the helper immediately after _resolve_configuration, before client.update_config. Both the real-write path and the --dry-run preview path normalize, so what the operator sees in dry-run matches what would actually land on Storage. The result envelope gains a normalizations: [{path, action: "sql_split"|"wrap_array", before_type, after_type, after_length}] field per write -- empty list when input was already valid. - CLI: human mode prints a yellow `Auto-normalized N script field(s) to array (string -> list). See --json for details.` warning followed by a per-element trace, so the silent fix is observable. ## Bonus silent-gap fix SQL_TRANSFORMATION_COMPONENTS in code_extraction.py was missing keboola.google-bigquery-transformation and keboola.duckdb-transformation, so kbagent sync push previously did NOT split semicolons in BigQuery / DuckDB transformations -- it joined every statement into one script element. Same failure shape as #119 (closed for Snowflake / Synapse / Oracle / Redshift), just on different backends. The new fragment-based is_sql_transformation_component() helper also matches future variants (*-bigquery-transformation, *-duckdb-transformation, *-exasol-transformation, *-teradata-transformation, *-transformation-v2) so newer SQL backends do not require an edit to the exact set. ## Tests - 35 new unit tests in tests/test_normalize_script.py: registry detection (exact + fragment fallback), splitter edge cases (semicolons inside block comments and string literals), per-component dispatch (SQL split vs Python wrap vs already-array passthrough), ConfigService integration (write path, dry-run path, --set path), CLI surfacing in both JSON and human modes. - New E2E test class TestE2EConfigUpdateNormalization in tests/test_e2e.py exercising the full path against a real Snowflake transformation in a dev branch: dry-run preview -> string-script push -> Storage API state assert (script[] is list, not str; literal with embedded ; preserved) -> --set path with nested script= value -> job run on the normalized config (status success/warning, NOT 'Expected array, got string' runtime crash). Live-validated against project 901 (padak) during development. ## Sync-map walk Every silent-drift surface from CONTRIBUTING.md "Plugin synchronization map" updated: - pyproject.toml 0.27.0 -> 0.27.1; plugin.json + marketplace.json synced via make version-sync. - changelog.py -- five entries for 0.27.1 (fix description, observability, silent gap, plumbing, plugin updates). - commands/context.py AGENT_CONTEXT -- config update bullet annotated. - commands-reference.md -- config update bullet annotated. - keboola-expert.md -- Rule 6 VERSION GATE notes 0.27.1+; tool selection matrix gains "Update SQL transformation body (script[])" row; inline gotchas gain "script[] string-vs-array runtime crash" entry. - gotchas.md -- new "config update auto-normalizes script[] from string to array (since v0.27.1)" section with full failure mode + bypass caveat. - sql-migration-workflow.md -- new error-table row, new "Auto-normalization of script[]" section, new anti-pattern. - SKILL.md regenerated via make skill-gen (no command-name diff; surface is signature-stable, behaviour-only change). ## Verification make check clean (2467 tests pass, lint + format + skill freshness + version consistency + changelog + error-codes all green). Live e2e against project 901 confirmed the full path: string-script push -> array on Storage API -> Snowflake job run to success. * chore: renumber 0.27.1 -> 0.28.0 (collection branch retarget) PR base retargeted from main to feat-0.28.0; this fix lands as part of the 0.28.0 release alongside other v0.28.0-tagged work, not as a standalone 0.27.1 patch. Mechanical rename across every silent-drift surface: - pyproject.toml 0.27.1 -> 0.28.0; plugin.json + marketplace.json re-synced via make version-sync. - changelog.py block key "0.27.1" -> "0.28.0"; the "(since v0.27.1)" inside the plugin entry rewritten to "(since v0.28.0)" for consistency. - gotchas.md, commands-reference.md, sql-migration-workflow.md, keboola-expert.md -- all "(since v0.27.1)" / "0.27.1+" markers rewritten to 0.28.0. - commands/context.py AGENT_CONTEXT auto-normalize bullet retagged. - tests/test_e2e.py docstring + assertion message reference v0.28.0. No behaviour change. make check clean (2467 tests pass). * feat(0.28.0): storage swap-tables -- promote typed table rebuild via Storage API swap (#247) * feat(0.28.0): storage swap-tables -- promote typed table rebuild via Storage API swap * fix(swap-tables): poll async storage job to completion (PHP claim of sync was wrong) * fix(swap-tables): register CommandHint so --hint client/service works (PR #247 review B-1) * fix(0.28.0): VERSION GATE + test client.close() assertion (PR #247 review NB-1, NB-2) (#248) - keboola-expert.md Rule 6: enumerate `storage swap-tables needs 0.28.0+` so an agent on an older install refuses the task before attempting it (carry-over NB-1 from PR #247 self-review). - tests/test_storage_swap.py: add `mock_client.close.assert_called_once()` to TestSwapTablesService.test_success and test_api_error_propagates per CONTRIBUTING.md Testing Guidelines line 371. Locks the try/finally contract that releases the HTTPX client on both happy + error paths. Documentation/test gaps only -- no functional code changes. * fix(0.28.0): TestE2EStorageSwapTables must skip without E2E_API_TOKEN (#250) CI `pytest -m 'not integration'` does not deselect the `e2e` marker, so the class's setup fixture ran in the runner without credentials and crashed with KeyError: 'E2E_API_TOKEN'. Other E2E classes apply both `@skip_without_credentials` (skipif decorator on the class) AND `@pytest.mark.e2e` (marker for explicit `-m e2e` runs). PR #247 shipped only the marker, hence the rollup PR #249 broke CI. Verified locally: `pytest -m 'not integration'` now skips the 3 swap tests cleanly (2480 passed, 69 skipped, 0 errors). * docs(0.28.0): add typify-table-workflow.md -- typeless to native-types end-to-end (#251) The PR #247 storage-types-workflow.md gained a small subsection on 'Promoting a typed rebuild back into the original name', but that section only covered the swap-tables mechanics. AI agents asked to retypify a Storage table also need to know HOW to profile the data, HOW to choose types, and HOW to hand off to the user -- those were missing. New 8-phase workflow: 0. Decide-or-skip rubric (when typify is the wrong answer). 1. Isolate in a dev branch (so production stays on the typeless original). 2. Profile the typeless table in a workspace -- length / cardinality / parse-failure / scale-precision queries with a decision matrix that maps profile signals to Snowflake native types. 3. Build typed sibling via storage create-table + copy data via in-workspace INSERT or SQL transformation, with row-count and NULL-count verification. 4. Validate downstream consumers in the dev branch (config search + representative transformation run against the typeless source). 5. Swap (dry-run + actual + verify column types swapped). 6. Re-run downstream as smoke test against the typed source. 7. Cleanup the sibling after merge. 8. Handoff protocol -- structured summary the AI agent gives the user with phase-by-phase receipts, the merge URL, and rollback / cleanup commands. Plus: SKILL.md workflow-references table gains the new entry; changelog 0.28.0 entry adds a 'Plugin docs' bullet describing the workflow.
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
1 participant
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.
Summary
PR #249 (release rollup) broke CI on the unit-suite job because
TestE2EStorageSwapTables(introduced in #247) was decorated only with@pytest.mark.e2e, missing the@skip_without_credentialsskipif decorator that all other E2E classes use. CI runspytest tests/ -v -m 'not integration', which does NOT deselect thee2emarker -- the class collected, the autousesetupfixture ran, andos.environ[ENV_TOKEN]raisedKeyError. 3 errors, exit 1.CI run: https://github.com/padak/keboola_agent_cli/actions/runs/25440815047
Fix is one line: add
@skip_without_credentials(the skipif decorator already used byTestE2EStorageNativeTypesAndBranchMaterializeand friends) above the existing@pytest.mark.e2emarker.Test plan
Verified locally with the exact CI command:
Before: 3 errors (
KeyError: 'E2E_API_TOKEN'insetup).After: 3 skips (
E2E tests require E2E_API_TOKEN environment variable).The
@pytest.mark.e2emarker is preserved so explicit-m e2eruns (with credentials) still pick the class up. This matches the pattern already established byTestE2EStorageNativeTypesAndBranchMaterialize.