Repository navigation
ENG-3684: hold each CI leg to an execution ratio, not just one executed test - #1138
Closed
elsheikhams99 wants to merge 11 commits into
Closed
elsheikhams99 wants to merge 11 commits into
elsheikhams99 wants to merge 11 commits into
Conversation
…ed test
The ENG-3544 guard fails a leg only when zero test bodies ran. tests/functional/v2
cleared that floor while executing 12 of its 195 collected tests: one
module-scoped fixture skip in test_actions_inputs.py took ~40 tests out on its
own, and 36 more inline skips, each conditional on backend state, took out the
rest. Every one of them turned a missing or broken platform fixture into a green
run.
Guard
- tests/ci_guards.py: ExecutionLedger now records collected vs executed per test
module and the id/reason of every skip. New AIXPLAIN_MIN_EXECUTED_RATIO
(default 0.8, shares the existing opt-in) with should_fail_for_low_execution_ratio
and a summary that prints the per-module table and the skip reasons -- the
reason is the actionable part, since it names the fixture that went missing.
An unparsable floor raises rather than falling back, so a typo cannot leave the
workflow claiming a threshold nobody enforces.
- tests/conftest.py: tallies the final item list in pytest_collection_finish, so
-k/-m deselection narrows the denominator too; enforces both floors in
pytest_sessionfinish, zero-executed first.
- Both workflows pin the floor at 0.8 explicitly.
Skips
tests/functional/asset_ids.py centralises the pinned backend ids and carries the
failure helpers (missing_fixture, require_env, resolve_multi_action_tool). Then:
- test_actions_inputs.py: the tool fixture resolves the pinned tool and fails
when it is absent; a new action_with_inputs fixture replaces ten per-test
searches that each skipped on their own.
- test_model.py: streaming and connection_type assertions run against the pinned
models instead of discovering one; the catalogue sweep in
test_model_validation_edge_cases is replaced by the pinned model.
- test_tool.py, test_snake_case_e2e.py, inspector_functional_test.py: sweeps and
empty-result skips become assertions against pinned assets.
- test_trigger.py: TEST_COMPOSIO_INTEGRATION_ID / TEST_CONNECTION_ID fail when
unset and are now exported by the workflow.
- test_arabic_agent.py: the empty-output skip becomes three attempts and then a
hard failure.
grep -c "pytest.skip(" tests/functional/v2/*.py drops from 37 to the 2
credential gates in conftest.py.
The ticket also named tests/functional/model/get_model_by_name_test.py, which
exercises the v1 factories. Left alone: v1 is being deprecated under its own PR,
so the skip there stays rather than creating a conflict for a file on its way
out. The ratio floor still covers that leg -- one conditional skip against its
collected count is well inside 0.8.
Tests: tests/unit/test_ci_guards.py covers the ratio predicate, the env parsing,
the per-module/skip bookkeeping and the message; test_ci_guard_wiring.py drives
the floor end-to-end over a throwaway tree, including the deselection and
misconfiguration paths; test_ci_matrix_coverage.py asserts the workflow pins it.
Every v2 functional test that needed a platform asset carried the raw ObjectId inline: 24 hex characters, no name, no owner, no override. The default LLM appeared in four files, the Slack integration in seven, and Firecrawl/Tavily/Google Search were copy-pasted byte-for-byte between the agent and tool suites. Those literals are not portable. CI resolves them against test-platform-api, a push to main resolves them against production, and a laptop with no BACKEND_URL resolves them against dev-platform-api: three separate id spaces. A retired asset therefore failed one leg at a time, and each failure read as an unrelated backend error from somewhere deep inside a test body. tests/functional/_assets.py is now the single source: one `AssetIds` dataclass per environment (dev/test/prod, selected by BACKEND_URL), a named field per asset, and an AIXPLAIN_TEST_<NAME> override for a single run -- which is also how a backend this module has never heard of gets a working id without a commit. Retiring an id is one line. A session fixture in tests/functional/v2/conftest.py resolves every id before any test runs and reports all the gaps at once, naming the asset, the id, the call that failed and the override that unblocks the run. The v2 conftest now imports its default backend from the same module rather than repeating the literal, because the id space selected and the backend the client talks to drifting apart is the failure this ticket is about. test_arabic_agent.py parametrises on the asset *name* rather than the id, so collection stays backend-independent. Its `gpt-4o` label was the correct half of the pair the ticket flags: the id was onboarded 2024-05-16 per its ObjectId timestamp, four days after GPT-4o shipped and five weeks before Claude 3.5 Sonnet did, and skills/aixplain-builder gives it as the id form of openai/gpt-4o. The "Anthropic Claude 3.5 Sonnet v1" comment the v1 suite attached to it was the stale one; the dating is recorded next to the id. Guards: tests/unit/test_functional_hygiene.py fails on any bare 24-hex literal under tests/functional/ -- in a comment as readily as in code, since one of the removed ids had been documenting a tool that no longer existed -- plus two checks that stop it passing vacuously. The module's own logic (host matching, overrides, unknown backends) is covered in tests/unit/test_functional_assets.py, following the cleanup_guards/test_functional_cleanup split. Scope is the v2 suite. The v1 legs still carry their own literals and are deliberately untouched: PROD-2918 deletes all eight of those directories, so rewriting them here would buy a delete/modify conflict and nothing else. The exemption is a named tuple in the guard rather than a detector quietly pointed at tests/functional/v2, and it matches nothing once PROD-2918 lands.
…xecuted-ratio-guard
…d-functional-asset-ids
…nest Addresses the review findings on this branch. The ENG-3544 blanket-skip guard had gone blind to the v2 suite. Its detector treated any function whose AST dump contained "environ" as guarded, and the autouse verify_assets_resolve fixture contained that text only by accident (its docstring, an `environment` variable, `environment_for`). An unconditional pytest.skip there still passed, and with the API-key read moved behind _api_key(), the real-conftest check only matched slack_token. Now: - the `client` and `assets` fixtures read TEAM_API_KEY/AIXPLAIN_API_KEY with an inline os.getenv, where the detector can see it; - the detector needs a real getenv/environ attribute or name node, not the substring; - test_ci_matrix_coverage.py has a regression case: an autouse fixture that skips unconditionally while naming "the environment" is flagged; - the real-conftest check requires every skip in the v2 conftest to be guarded and one guard to read the API key. A new test deletes the env reads from the real conftest and expects every skip to be flagged. Verified by mutation in a scratch copy: an unconditional skip in client or assets, an autouse blanket skip, or the guard behind _api_key() each fail the suite. The session-wide, autouse verify_assets_resolve is replaced by per-name resolution. The `assets` fixture returns LazyAssets: the first read of `assets.X` resolves X through SPECS (client.<resource>.get(id)) and caches the outcome, success or failure, so @flaky reruns don't hit the backend again. A missing asset fails only the tests that read it, with the same message as before (asset, id, backend, call, override). A leg that reads no asset makes no resolution call and builds no client. The fixture checks credentials before it computes the id space, so no key plus an unrecognised BACKEND_URL skips instead of erroring. Host matching in tests/functional/_assets.py is now an exact lookup of urlparse(url).hostname, lower-cased. Before, a substring test read staging-platform-api as prod and a query string naming test-platform-api as test. AIXPLAIN_TEST_ENV is applied only to an unrecognised host. If it contradicts a recognised one, that raises ConflictingEnvironmentError. Override and pin values are stripped, and a whitespace-only value counts as unset. Unit tests cover the new host cases, the pin rules, whitespace overrides, and LazyAssets with a fake client. A static check makes sure no autouse fixture in the v2 conftest comes back. The module docstring and AGENTS.md now describe lazy resolution and no longer treat PROD-2918 as future work. This also corrects counts in 073af9d's message: Slack was inlined in six files, not seven; two ids (Firecrawl, Tavily) were shared by the agent and tool suites, not three; and the gpt-4o dating and "eight directories" no longer apply.
Addresses the review findings on test_functional_hygiene.py: - OBJECT_ID_PATTERN is case-insensitive, keeping its lookarounds, so an upper-case ObjectId is caught too. - The scan asserts it found at least one file, so pointing it at the wrong directory can't pass vacuously. - Relative paths are compared in POSIX form, so the _assets.py exemption also holds on Windows. - The V1_LEGS/_is_v1_leg exemption is removed. v1 is gone (PROD-2918), and the generic directory names it listed would have exempted a new tests/functional/model/ file with a bare id. - A new test checks that every asset name in test_arabic_agent.py's MODELS is a field of AssetIds. It parses the module rather than importing it, so a typo fails here instead of at run time on a backend. - Docstrings are added to the test functions this PR introduced.
Addresses review findings on the execution-ratio guard. - Count executed tests as a set of nodeids. pytest-rerunfailures reruns a test whose teardown failed, so a body that already passed reached `call` twice and was counted twice, inflating the ratio. - Print the ratio and the floor with one decimal (`:.1%`); 199 of 250 used to read "(80%), below the 80% floor". - Replace should_fail_for_low_execution_ratio with breached_execution_floor, which returns the floor it parsed, so pytest_sessionfinish reads AIXPLAIN_MIN_EXECUTED_RATIO once. low_execution_ratio_message takes the xdist denominator as an argument instead of the conftest mutating the ledger to pass it. - Drop dead code: SkippedTest becomes a NamedTuple (its hand-written __eq__ was unused) and the test-only ExecutionLedger.executed_ratio() is gone. - Add wiring tests that run the floor end-to-end under `-n 2`, which is how the agent and arabic-agent legs run. They fail if the `or session.testscollected` fallback is removed from tests/conftest.py, or if the per-module table loses its executed+skipped fallback; both mutations previously left every guard test passing. - Strip GIT_* from the wiring tests' subprocess environment. - Reword comments that cited per-run counts which no longer hold against the current base, and the claim that 0.8 leaves room for credential-gated skips: the per-file legs have no conditional skips of their own.
Addresses review findings on the functional fixtures. - resolve_multi_action_tool searches up to 5 pages of 20 tools instead of only the first page, and requires the tool to list two or more actions, as its docstring promised. That holds for an AIXPLAIN_TEST_MULTI_ACTION_TOOL_ID override too. A connected tool is tenant-specific, so no id is pinned; the failure message names the Slack integration filter, what was searched, and the override. It still fails rather than skips. - Correct the test_actions_inputs.py / test_tool.py fixture docstrings, which claimed the tool was pinned in asset_ids.py. - test_arabic_agent.py: keep one retry layer for empty model output. The flaky markers now pass `rerun_except` (pytest-rerunfailures >= 16, as pinned) matching the _run_until_output exhaustion failure, so an output that is empty on every attempt is no longer rerun, which had doubled the billed runs per item. The marker still reruns timeouts. The failure message says how many consecutive runs were made and that it is not rerun. - Move the remaining literal ids into tests/functional/asset_ids.py by value: the Arabic suite's GPT-5.4 id (TEXT_MODEL_ID), the Slack integration in test_agent.py and test_integration.py (SLACK_INTEGRATION_ID), and the Tavily tool in test_agent.py (TAVILY_TOOL_ID). - Fix the asset_ids.py module docstring, which cited skip counts that no longer hold. - ruff format the touched files that were formatted on development.
Addresses review findings on the trigger fixtures and CI docs.
- main.yaml exports TEST_COMPOSIO_INTEGRATION_ID / TEST_CONNECTION_ID from
the *_PROD secrets on `main` and the plain secrets otherwise. Both name
backend-resident objects, so one value cannot serve the test and
production backends. The values reach the script through the step's
`env:` rather than `${{ }}` interpolation, with the same variable names
ENG-3683 uses so the two branches converge.
- docs/ci/required-checks.md documents the secrets the functional legs read,
including both trigger pairs, which now fail the trigger leg when unset.
- Reword the workflow comment on AIXPLAIN_MIN_EXECUTED_RATIO, which cited
counts that no longer hold and claimed the floor leaves room for
credential-gated skips. A missing credential skips a whole leg, which the
zero-executed floor covers.
- test_ci_matrix_coverage.py: a rejected floor is reported as "CI integrity
guard misconfigured", not an INTERNALERROR.
ENG-3685 introduced tests/functional/_assets.py as the single home for the functional suite's backend asset ids, read through the `assets` fixture and overridable per run with AIXPLAIN_TEST_<NAME>. This branch had grown its own registry in tests/functional/asset_ids.py, with a second set of override names (AIXPLAIN_TEST_TEXT_MODEL_ID, ..._SLACK_INTEGRATION_ID, ...) for the same ids. Two registries for one id space is the drift ENG-3685 exists to prevent, and its hygiene test rejects the bare ids that module still carried. So the id constants are gone. Every caller now reads assets.DEFAULT_LLM, STREAMING_TOOL_CALL_MODEL, SYNC_ONLY_MODEL, ASYNC_ONLY_MODEL, SLACK_INTEGRATION or TAVILY from the fixture, and the re-pin hints in test_model.py name ENG-3685's override variables. What remains is the failure helpers -- missing_fixture, require_env and resolve_multi_action_tool -- so the module is renamed to tests/functional/_helpers.py, which no longer claims to hold ids. resolve_multi_action_tool takes the Slack integration id as a parameter (callers pass assets.SLACK_INTEGRATION) instead of reading a module constant. The multi-action tool itself stays an AIXPLAIN_TEST_MULTI_ACTION_TOOL_ID override with no default, now read when the helper runs: a connection is tenant-specific, so it has no portable id to register.
Collaborator
Author
|
Superseded by #1163, which combines this PR with the other ENG-3683–3692 PRs into one branch based on development (one commit per original PR, conflicts resolved), plus fixes from the first live functional CI run. |
aix-ahmet
pushed a commit
that referenced
this pull request
Oct 9, 2026
…ed test Combined from PR #1138, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
aix-ahmet
added a commit
that referenced
this pull request
Oct 9, 2026
…l (combined) (#1163) * ENG-3686: remove committed API keys and scan for secrets on every push Combined from PR #1139, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3685: name the functional suite's backend asset ids, once Combined from PR #1140, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3684: hold each CI leg to an execution ratio, not just one executed test Combined from PR #1138, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3683: gate PRs and releases on the functional matrix Combined from PR #1137, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3691: ticket-key every permanent skip, enforce registered markers, drop dead fixtures Combined from PR #1158, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3692: align v2 test hygiene, markers and lint scope Combined from PR #1157, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3687: add functional coverage for untested v2 resources Combined from PR #1154, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3689: add v2 team-agent functional tests Combined from PR #1155, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3688: port agent lifecycle functional tests to v2 Combined from PR #1156, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3690: cover public agent run-time features functionally Combined from PR #1162, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3688: send expected output as a string in run payloads build_run_payload turned a Pydantic class into model_json_schema() and an instance into model_dump(), both dicts, so executionParams.expectedOutput reached the backend as an object. The backend rejects that with "400 executionParams.expectedOutput must be a string", which is how test_agent_lifecycle.py::test_agent_expected_output_run_returns_json failed in the first live CI run. Save already sent json.dumps(schema). Run payloads now JSON-encode a class (the same schema string save persists) and an instance, as they already did for a dict. Unit tests cover the class passed through execution_params, the agent's own class, and an instance. * ENG-3688: assert the backend's in-use error code test_delete_agent_in_use_fails expected "cannot be deleted" in the error, but the backend refuses the delete with the code err.agent_is_in_use ("Failed to delete resource: err.agent_is_in_use" in the first live CI run). Assert on that code instead of prose the backend never sends. * ENG-3688: stop asserting max_inspectors survives a fetch test_agent_round_trips_execution_and_inspector_fields failed in the first live CI run with "assert None == 2" on fetched.max_inspectors. The SDK encodes and decodes the field under the same key (maxInspectors, checked offline: build_save_payload sends it, from_dict reads it back), so the None means the backend does not persist it. The test still saves with max_inspectors=2, so the backend must accept it, and keeps asserting the fields that do round-trip. * ENG-3690: reword the prompt and variables run tests neutrally Both tests asked the model to repeat a "secret", and in the first live CI run GPT-5.4 treated that as a request to leak hidden instructions: test_prompt_override_reaches_model got "I can't share secrets or hidden instructions", and test_variables_are_substituted_into_instructions got the literal "word" back instead of the substituted value. Each now asks for a random token ("Reply with exactly this code and nothing else: <token>") and asserts the token appears. The variables test keeps the placeholder in the instructions, where the SDK rewrites {{x}} to {x} on save and the backend substitutes the query keys at run time (the same contract v1's process_variables used), so the SDK path is unchanged. * ENG-3690: verify only acceptance of run-time inspectors test_run_time_inspector_runs expected a step for the run-time inspector, but in the first live CI run the response's steps held only the agent's own LLM step, so the response exposes no evidence the inspector ran. Renamed to test_run_time_inspector_accepted per the file's *_accepted convention: it asserts the inspectors list reaches the payload as given and that the backend accepts the run. Drops the now-unused _step_agent_label helper. * ENG-3687: page through a sorted search when checking pagination test_second_page_does_not_repeat_the_first compared pages 0 and 1 of an unsorted page_size=1 model search, and in the first live CI run both returned the same model (6ab3e2f8...). Without sort_by the SDK sends sort: [{}], so the backend's order is not guaranteed across two calls, and whether pageNumber counts from 0 or 1 is not something the test should depend on. The test now sorts by creation time and compares pages 1 and 2 (distinct under either convention) with three records each, asserting the two id sets are disjoint. The SDK sends pageNumber/pageSize exactly as v1 did, so no SDK change. * ENG-3687: give every functional test skill a unique name The skill_dir fixture named each skill func-test-skill-<epoch seconds>. Two skills created within the same second (consecutive tests, or a rerun) share that name, and in the first live CI run the backend refused the second with err.exist_name, erroring the setup of TestAgentReflectsSkillUpdate::test_agent_run_reflects_updated_skill_content. Append a uuid fragment, as the agent names in the same file already do. * ENG-3684: fall back to any multi-action tool In the first live CI run every caller of resolve_multi_action_tool failed ("Searched 100 tool(s) ... Slack-backed tools with actions available: none"), and both TestInputActionFieldsRoundTrip tests failed on "Pinned integration 686432941223092cb4294d3f reports no actions available". The cause is actions_available, not the tenant: the backend leaves it unset (None) on search results and integrations. The same run shows the Slack integration with actions_available=None while test_tool.py::test_tool_get_parameters passed on that integration's non-empty list_actions(). Filtering on the field ruled out every tool. resolve_multi_action_tool now skips only an explicit False and probes candidates with list_actions(): Slack-backed tools first, then tools of any other integration, then the rest, bounded to 5 pages and 25 probes. The AIXPLAIN_TEST_MULTI_ACTION_TOOL_ID override is unchanged, and finding nothing still fails, naming every probed tool and its action count. The snake-case round-trip tests use the new resolve_action_source: the pinned Slack integration when it lists actions, else the resolved multi-action tool (same list_actions/list_inputs surface), failing loudly when neither lists any. * ENG-3683: cap functional matrix parallelism The first live run of the functional matrix lost ~40 tests to "429 err.agent_concurrency_limit_reached" (limit 20 for the test team): every leg drives agents on the same tenant, several at -n 2, and all legs started at once. Those failures say nothing about the SDK. Add max-parallel: 4 to the functional job's strategy, with the reason beside it, and a unit test in test_ci_matrix_coverage.py that fails if the cap is dropped or raised above four. required-checks.md notes the cap next to the concurrency group that bounds runs per PR. * ENG-3688: send expected output as a string from session runs too ExecutionConfig.to_api_dict passed execution_params['expected_output'] through unchanged, so a session run with a Pydantic class, instance or dict sent a non-string executionParams.expectedOutput, which the backend rejects with "400 executionParams.expectedOutput must be a string". This is the same bug the previous commit fixes on the direct run path. The session path now encodes it the same way: a class as its JSON schema, an instance as its JSON, and a dict or list with json.dumps. * ENG-3684: xfail the sync-only model when the asset lacks connection_type metadata ENG-3684 turned the old skip in test_sync_model_connection_type into an assert, but the pinned sync-only model exposes no connection_type metadata on the backend, so the model leg went red on a fact no SDK change can fix. Both connection-type tests now call pytest.xfail when the metadata is missing, naming the pinned asset and the AIXPLAIN_TEST_SYNC_ONLY_MODEL / AIXPLAIN_TEST_ASYNC_ONLY_MODEL override. When the metadata is present the real assertions run unchanged, so the tests start passing by themselves once the asset carries it or is re-pinned. * ENG-3688: use one expectedOutput encoder for save, run and session expectedOutput was encoded three times, differently. build_run_payload JSON-encoded a Pydantic class, instance or dict but sent a list raw; ExecutionConfig.to_api_dict also encoded lists; build_save_payload turned a class into its schema string but stored a Pydantic instance (and a dict) as an object, which the agent engine reads back as a Python repr when a run falls back to the stored value. All three now call session._expected_output_to_wire (imported lazily in agent.py, as resolve_attachments already is, to avoid an import cycle): class -> json.dumps(model_json_schema()), instance -> model_dump_json(), dict/list -> json.dumps, str/None unchanged. The run path keeps its rule that an empty string under text/markdown is sent as None. The save-side change is deliberate. The only test pinning a dict at save, test_basemodel_instance_is_still_dumped_to_dict, preserved the original v2 behaviour rather than a backend contract: the save endpoint already stores the schema string for a class. It now asserts a JSON string, and new tests cover a dict at save, save/run agreement, a list on the run path and the text-format empty-string rule. CHANGELOG notes the wire-format fix and that a fetched agent now carries the string. * ENG-3688: assert inspector fields on the save payload, not after fetch test_agent_round_trips_execution_and_inspector_fields failed in CI with "assert [] == ['output']": the platform's v2 create/update path does not persist inspectorTargets or maxInspectors, and the read side defaults the targets to []. That is a backend gap (ticket to be filed), not an SDK bug. The test now asserts both fields on agent.build_save_payload() -- what the SDK actually sends -- before saving, with a comment saying why and when to move them back below the fetch. The output_format and expected_output round-trip assertions are unchanged. * ENG-3690: make the run-params leg report backend gaps as expected failures Five tests in test_agent_run_params.py were red in CI for reasons that are not SDK bugs, and one for a fixture bug: - variables and prompt: the SDK sends the v1-compatible contract (query {input, <vars>}; top-level prompt/criteria/identifier), but the agent engine reads only query.input. Each test is split into a payload test that must pass and an output test under xfail(strict=True, raises=AssertionError). A refused run still raises APIError and a non-success status calls pytest.fail, so only the output check can be the expected failure. - budget maxDurationSeconds=0: the payload carries the cap, but the run hangs IN_PROGRESS on the backend. The payload is asserted first, the run gets timeout=60 instead of the 300 s default, and the test is a strict xfail on the SDK TimeoutError only. - attachments: RED_PNG_BYTES was one pixel at 50% alpha, so "coral" was a correct answer. It is now a 16x16 8-bit RGB PNG with no alpha channel, generated with zlib/struct and decoded to confirm every pixel is (255, 0, 0). - test_debug_returns_analysis duplicated test_debugger.py, so one missing meta-agent turned two legs red. Deleted; the module docstring now says where debug() is covered and how the xfails are structured. * ENG-3689: xfail the team expected-output run until the engine resolves $defs test_run_team_agent_with_expected_output failed in CI with a pydantic ValidationError: _People nests _Person, so its JSON schema reaches _Person through $ref/$defs, which the agent engine does not resolve. The nested type degrades to a list of anything and the model answered {"result": ["Bruno: 34 ..."]}. The test is now xfail(strict=True, raises=pydantic.ValidationError) with a backend reason (ticket to be filed). The persistence checks, a failed run and a missing delegation raise AssertionError, which the xfail does not accept, so only the structured-output validation can be the expected failure. The flaky mark is dropped: rerunning a known backend gap only spends three billed team runs to reach the same xfail. * ENG-3689: drop the run_response_generation team test test_run_response_generation_accepted spent a billed, rerun-able team run to show only that the backend accepts run_response_generation, while test_agent_run_params.py documents that the agent runtime ignores the flag (and its JSON-output test already passes it). Two files disagreeing about the same parameter is worse than one saying it once. Delete the test and replace its line in the module docstring with a note pointing at test_agent_run_params.py. * ENG-3687: never file issues against production tests/functional/v2/test_issue.py files real issues, and there is no API to delete one. The issue leg ran on every push to main, against the production backend, so each merge added two undeletable test reports to the production issue system. The issue leg now carries `prod_safe: false` in the matrix. "Run Tests" receives the leg's path and that key through env: (no more `${{ }}` in the script body) and, when IS_PROD is true and the key is false, prints a ::notice:: explaining the skip and exits 0 without running pytest. The leg still runs on PRs, pushes to test and the nightly, against the test backend. On production runs `functional (issue)` therefore reports success without having run, and release.yaml's gate counts it green. That is deliberate (no production run can exercise it without the side effect) and is stated in the workflow comment and docs/ci/required-checks.md. New unit tests run the real step with a stub python: the issue leg runs pytest when IS_PROD is false and skips with a notice when it is true, every other leg still runs its exact pytest command on production, and the step body holds no `${{ }}`. The expression evaluator learned `matrix.*` lookups for this. * ENG-3683: hand SLACK_TOKEN only to the legs that read it, and drop HF_TOKEN SLACK_TOKEN and HF_TOKEN were exported to every functional leg, including same-repository PR runs, which pip install whatever dependency changes the PR carries. Only three legs read SLACK_TOKEN (agent-lifecycle, model and tool, through the slack_token fixture), and nothing under tests/functional reads HF_TOKEN at all; aixplain/utils/config.py defines it but no SDK or test path consumes it. Those three legs now carry `slack: true` in the matrix, and the env step expands the secret as `matrix.slack && secrets.SLACK_TOKEN || ''`, the same pattern that gates the _PROD secrets, writing SLACK_TOKEN to GITHUB_ENV only when it is non-empty. HF_TOKEN is no longer exported. Unit tests: the `slack` set must equal the legs whose test file uses the slack_token fixture, and every other leg renders SLACK_TOKEN as empty; the real env step exports SLACK_TOKEN only when it has a value; nothing under tests/functional reads HF_TOKEN and main.yaml does not export it. The secrets table in docs/ci/required-checks.md says the same. * ENG-3683: count only runs that prove they used production in the release gate release.yaml's functional gate recognised a manual dispatch on the release tag with `.event == "workflow_dispatch" and .head_branch == $tag`. A run's head_branch is a bare name, so a dispatch on a *branch* named exactly like the tag (which IS_PROD treats as a test-backend run) passed the filter, and its green legs could satisfy a production release. main.yaml gains a trivial `production-backend` job whose `if:` is the functional job's IS_PROD expression (repeated, since a job-level `if:` cannot read another job's env). The gate now reads each candidate run's jobs through a jq `jobs_filter` that yields the run's successful job names only when `production-backend` is among them; a run without it is logged as a warning and not counted. Checking the real ref inside the run is robust where a "does refs/heads/$TAG exist now" lookup is not: a branch deleted after its dispatch would slip past that. Unit tests: the job's condition matches IS_PROD for every event/ref case, including a dispatch on a branch named `v0.3.1`; the jobs filter, run on paginated fixture JSON, drops a run whose production-backend job was skipped and keeps one where it succeeded on a later page; the gate script still wires the filter in; the required-checks doc names the new job, which is listed as not to be required (it is skipped on every PR). * ENG-3683: let the leg timeout fire before the functional job timeout Six legs have `timeout: 45`, which equalled the functional job's `timeout-minutes: 45`. When a leg hung, the job-level kill landed first, so the leg showed as cancelled and its teardown never ran -- the per-leg timeout could never do its job. Raise the job timeout to 55, above the longest leg timeout, with the reason beside it, and add a unit test in test_ci_matrix_coverage.py that fails if the job timeout is not above every leg's. * ENG-3683: correct stale CI docs and comments - AGENTS.md pointed at tests/mock_responses/, which ENG-3691 deleted, and said every leg has a 30-minute timeout; legs declare 30 or 45 minutes. - main.yaml's concurrency comment (and the matching unit-test docstring) said the matrix is 14 legs; it is one leg per functional test file, so say that instead of a count that rots. - docs/ci/required-checks.md left `benchmark` out of the deleted v1 legs. - release.yaml (and a unit-test message) justified `actions: read` by the repository being private; it is public. The scope is needed because the workflow's deny-all default grants nothing. * ENG-3686: keep leaked-key details out of the public doc docs/ci/secret-scanning.md is published in a public repository, and it listed each removed credential's file and line, the backend it worked against, a partial fingerprint of each key and the path of a third token, alongside the statement that none had been rotated. That is a map for anyone looking for live credentials in history. The doc now says only what a reader of this repository needs: committed credentials were removed in ENG-3686, they remain in history and must be rotated, rotation is urgent and independent of any PR, and the specifics are tracked privately in the ENG-3686 ticket. The scanner design, the repo rules, the gitleaks-vs-detect-secrets rationale and the allowlist policy are unchanged; the open items are kept, without identifiers. The same details are removed from the test_secret_scanning.py module docstring, the .gitleaks.toml header comments, the test_eval.py docstring (which tied a deleted script to its key and backend) and a test_agent_lifecycle.py docstring that named the provider of a leaked MCP URL. * ENG-3686: drop the dead login_response.json allowlist entry .gitleaks.toml exempted, by exact value, the recorded ECR password in tests/mock_responses/login_response.json. ENG-3691 deleted that file, so the entry matches nothing, and the allowlist policy is to remove a dead exemption: it can only hide something later. gitleaks dir . with the pinned config still reports no leaks; with the value allowlists removed it reports only the documentation placeholders and the hand-written fake keys in unit tests, which is what the updated docs/ci/secret-scanning.md describes in place of the old "8 findings". * ENG-3684: let a failing integration action listing fail the test resolve_action_source caught every exception from the pinned integration's list_actions() and treated it as "no actions", then fell back to a multi-action tool. A broken listing endpoint therefore went unnoticed: the snake-case round-trip tests passed on the fallback tool while the integration path they are meant to cover was down. Only an empty listing falls back now; an exception propagates and fails the test with the real error. The docstring says why. * ENG-3687: check that pages 0 and 1 of a sorted search are disjoint test_second_page_does_not_repeat_the_first compared pages 1 and 2 only, so it could no longer see the bug it was written around: in the first live CI run pages 0 and 1 returned the same model. The SDK documents page_number as 0-based, and a backend that counts from 1 and clamps 0 up to 1 would alias exactly those two pages. Under the stable CREATED_AT sort the test now also fetches page 0 and asserts it is full and shares nothing with page 1, with a message naming the 1-indexing suspicion. The 1-vs-2 check stays first, so a failure tells the two problems apart. * ENG-3684: create the recurring test triggers disabled test_trigger.py created an enabled trigger every two hours (plus daily, weekly and monthly ones) and relied on teardown to delete them. PR runs use cancel-in-progress, so a newer push can cancel a leg before teardown runs, leaving an active recurring trigger that keeps running its agent on the shared test backend. The recurring triggers are now created with enabled=False. What those tests check -- the schedule_type mapping and notifications -- does not depend on the trigger being active, and enable/disable is still covered by test_enable_disable_via_save on a far-future one-off trigger. The two-hourly test also asserts the fetched trigger is stored disabled, so a backend that ignored the flag on create would show up rather than quietly leak an active schedule. * ENG-3687: encode every non-string expected_output in one shared module Tuples, numbers and bools went out unencoded and still drew the 400; dicts were ASCII-escaped while Pydantic instances kept UTF-8. The encoder now lives in aixplain/v2/_expected_output.py so Agent no longer imports it lazily from session.py, and the read-back change is listed under Breaking. * ENG-3685: cache only permanent asset-resolution failures One 503, timeout or 429 on the first read of an asset was cached for the whole session, so every later test in the worker, @flaky reruns included, replayed it. Successes and definite failures (a 4xx other than 408/409/425/429, an explicit retryable=False, an undeserializable response, a client that cannot be built) are still cached; anything else is asked again on the next read. * ENG-3683: tighten functional-scope paths, label cancellation and the Slack leg - The path filter now covers every module directly under tests/ and pytest.ini, so a PR touching only ci_guards.py, cleanup_guards.py or the marker registry no longer skips the matrix and passes functional-result green. - An unrelated label no longer cancels a running matrix: its run queues in the PR's group instead. A push or the functional-tests label still cancels. - agent-lifecycle no longer holds SLACK_TOKEN: its only Slack test is skipped (BUG-1098). The guard now ignores unconditionally skipped tests, so it fails when that skip is removed without restoring slack: true. * ENG-3686: catch platform keys after per-environment credential names The separator between the credential name and the key could not contain a-f, so TEAM_API_KEY_PROD="<key>", AIXPLAIN_API_KEY_DEV=<key>, SECRET_KEY = "<key>", api_key_test: <key> and {"x-aixplain-key": "<key>"} all went unreported. The rule now needs only a non-hex character right before the key, and aixplain-key is a credential name. Checked with gitleaks v8.28.0: all five shapes are caught, a bare sha256 digest is not, and the tree scan stays clean. * ENG-3684: resolve the multi-action tool once per module and stop masking listing errors multi_action_tool re-ran the bounded search (up to 5 Tool.search pages and 25 list_actions probes) for each of its consumers; the id is now resolved once per module and each test gets a fresh Tool.get. _action_count no longer turns every exception into 0: a pinned tool's listing error propagates, and a search candidate is passed over only when the backend refuses that tool with a 4xx. * ENG-3688: keep two run tests off backend gaps they do not test - test_deprecated_files_alias_uploads_and_reads reads a .txt file, so it runs on the default LLM instead of VISION_LLM, whose outages turned the leg red. - test_agent_expected_output_run_returns_json uses a flat schema: the engine does not resolve $ref/$defs (the strict xfail in test_team_agent.py), so the nested People schema could fail the leg on all three flaky attempts. * ENG-3684: share unique_name and agent_ids from tests/functional/_helpers.py _unique, _name, _unique_name and _agent_ids were redefined in five test files. They are now unique_name (uuid-suffixed, safe under xdist and reruns) and agent_ids (id strings, dicts or Agent objects) in the shared helpers module. * ENG-3684: add the Apache 2.0 header to the new functional modules AGENTS.md requires it on new source files; _assets.py, _helpers.py and the ten test modules this PR adds had none. Appended to each module docstring, as in aixplain/v2/file.py. * ENG-3684: find the event-trigger integration and connection without secrets TEST_COMPOSIO_INTEGRATION_ID / TEST_CONNECTION_ID (and their _PROD variants) were never created, so the trigger leg was red on every run and release.yaml could not release. Neither is a credential: - the integration is fetched by its marketplace path, composio/gmail, which resolves on every backend; - the connection comes from resolve_trigger_connection, a bounded search for a connected tool that lists event-trigger types (Gmail-backed tools first), pinnable with AIXPLAIN_TEST_TRIGGER_CONNECTION_ID. The workflow no longer reads the four secrets, and a unit guard keeps them out. --------- Co-authored-by: aix-ahmet <ahmet.gunduz@aixplain.com>
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.
The ENG-3544 guard fails a leg only when zero test bodies ran. tests/functional/v2 cleared that floor while executing 12 of its 195 collected tests: one module-scoped fixture skip in test_actions_inputs.py took ~40 tests out on its own, and 36 more inline skips, each conditional on backend state, took out the rest. Every one of them turned a missing or broken platform fixture into a green run.
Guard
Skips
tests/functional/asset_ids.py centralises the pinned backend ids and carries the failure helpers (missing_fixture, require_env, resolve_multi_action_tool). Then:
grep -c "pytest.skip(" tests/functional/v2/*.py drops from 37 to the 2 credential gates in conftest.py.
The ticket also named tests/functional/model/get_model_by_name_test.py, which exercises the v1 factories. Left alone: v1 is being deprecated under its own PR, so the skip there stays rather than creating a conflict for a file on its way out. The ratio floor still covers that leg -- one conditional skip against its collected count is well inside 0.8.
Tests: tests/unit/test_ci_guards.py covers the ratio predicate, the env parsing, the per-module/skip bookkeeping and the message; test_ci_guard_wiring.py drives the floor end-to-end over a throwaway tree, including the deselection and misconfiguration paths; test_ci_matrix_coverage.py asserts the workflow pins it.