refactor(tests): retire script enumeration; pytest discovers tests/integration/

[danielmeppiel]

refactor(tests): retire script enumeration; pytest discovers tests/integration/

TL;DR

PR2 of #1166 collapses scripts/test-integration.sh from 778 → 402 lines by deleting the 28 hand-maintained pytest tests/integration/test_X.py invocations and replacing them with a single pytest tests/integration/ call. The marker registry shipped in PR1 (#1167) handles per-test gating, so new test files dropped into the directory are picked up automatically. 11 test files get the requires_* markers they were previously running on the honor system.

Note

Builds on the marker registry merged in #1167 (PR1). Closes the original wiring bug from #1166 — the script previously enumerated only 28 of 73 integration files, leaving ~45 files dependent on contributors remembering to wire them in.

Problem (WHY)

• Per-file enumeration drifts silently. The pre-PR2 script invoked pytest 28 times against named files; the repo had 73 integration test files. New tests landed in tests/integration/ and never ran in CI unless someone remembered to add a pytest …test_X.py block. Acceptance criterion 2 of Refactor test-integration.sh to fixture-driven pytest discovery (eliminate manual test wiring) #1166: "A new test file dropped into tests/integration/ with the right markers automatically runs without script edits."
• Bash branches re-encoded the gating logic that PR1 centralised. Each block had its own if/then/log_error/exit 1. PR1 (refactor(tests): marker-driven integration discovery (PR1 of #1166) #1167) moved precondition logic into _MARKER_CHECKS in tests/integration/conftest.py; the script's per-file scaffolding is now duplicate state.
• [!] test_skill_bundle_live.py (pytestmark = pytest.mark.live, 9 tests) hits real GitHub repos and was never gated by _MARKER_CHECKS. It was hidden from CI today only because the script never enumerated it; once pytest discovers the directory it would run by default.
• [!] APM_RUN_INTEGRATION_TESTS=1 was set per-invocation in the old script for test_transport_selection_integration.py. Without lifting it to job env, the marker-gated test would silently skip in CI post-refactor.
• [!] requires_apm_binary was missing on 11 files that shell out to the built apm binary. Hermetic on a contributor laptop with apm on PATH; hard failure in CI without the binary or token.

Why these matter: the script-as-source-of-truth regime contradicts the marker-driven design intent of #1166 ("script no longer enumerates individual pytest test_*.py calls; it invokes pytest tests/integration/ with marker selection"). Acceptance criterion 4 also requires: "Conditional gating (ADO PAT, GitHub PAT, runtime presence) preserved via markers + skipif fixtures, not bash branches."

Approach (WHAT)

#	Fix
1	Replace run_e2e_tests() body (28 enumerated blocks, ~415 lines) with a single pytest tests/integration/ invocation.
2	Add requires_apm_binary / requires_github_token / requires_runtime_copilot markers to 11 files that previously ran on the honor system.
3	Add 'live' to addopts deselect list in pyproject.toml so test_skill_bundle_live.py preserves its current "opt-in only" behaviour without inventing a new marker name.
4	Lift APM_RUN_INTEGRATION_TESTS=1 to job env in ci-integration.yml; bump timeout 20 → 30 min to absorb the newly-discovered tests.
5	Reframe docs/.../integration-testing.md: the script is the CI orchestrator, no longer "legacy".
6	Drop the verbose hero-scenario success echo block in main(); pytest reports per-test results.

Implementation (HOW)

• scripts/test-integration.sh — removed lines 300-715 (run_e2e_tests() body) and lines 743-766 (decorative success echoes); replaced with a thin function that exports APM_E2E_TESTS=1, prints the env probe, sources .venv if present, and runs pytest tests/integration/ -v --tb=short. Header banner reframed from "DEPRECATED" to "thin orchestrator". Token resolve / platform detect / binary build / runtime setup / dep install untouched.
• tests/integration/conftest.py — not modified. The _MARKER_CHECKS registry from PR1 is the contract this PR depends on.
• pyproject.toml — one-char change: addopts = "-m 'not benchmark and not live'". Preserves opt-in via pytest -m live.
• .github/workflows/ci-integration.yml — added APM_RUN_INTEGRATION_TESTS: "1" to the integration-tests job env; timeout-minutes: 20 → 30.
• 11 test files — each got pytestmark = pytest.mark.requires_apm_binary (or a list when stacking with requires_github_token / requires_runtime_copilot). Multi-marker files use pytestmark = [pytest.mark.X, pytest.mark.Y] (pytest's documented list form). See test_mcp_env_var_copilot_e2e.py for the canonical multi-marker shape.
• docs/src/content/docs/contributing/integration-testing.md — rewrote the "Legacy: scripts/test-integration.sh" section as "CI orchestrator: scripts/test-integration.sh". No removal of contributor-facing pytest invocations on the same page.
• CHANGELOG.md — one Unreleased line under Changed, ending in (#PR_NUMBER).

Diagrams

Legend: pre-PR2 vs post-PR2 control flow inside scripts/test-integration.sh. The boxed nodes are the new behaviour; everything else was already there.

flowchart TD
    A[main] --> B[check_prerequisites]
    B --> C[detect_platform]
    C --> D[detect_environment]
    D --> E[build_binary or use CI artifact]
    E --> F[setup_binary_for_testing]
    F --> G[setup_runtimes codex copilot llm]
    G --> H[install_test_dependencies]
    H --> I[run_e2e_tests]
    I --> J["pytest tests/integration/ NEW: single invocation"]
    J -->|exit 0| K[log_success Ready for release validation]
    J -->|exit non-zero| L[log_error exit 1]

    style J fill:#dff,stroke:#06c,stroke-width:2px

Loading

Legend: how a single test reaches a skip vs run decision. The _MARKER_CHECKS registry is the gate; the script no longer participates.

sequenceDiagram
    participant Script as scripts/test-integration.sh
    participant Pytest as pytest tests/integration/
    participant Conftest as conftest.py _MARKER_CHECKS
    participant Test as test_marketplace_e2e.py

    Script->>Pytest: invoke once
    Pytest->>Conftest: pytest_collection_modifyitems items
    Conftest->>Test: get_closest_marker requires_apm_binary
    Test-->>Conftest: marker present
    Conftest->>Conftest: _has_apm_binary
    alt binary resolved
        Conftest-->>Pytest: leave item
        Pytest->>Test: run
    else binary missing
        Conftest-->>Pytest: add skip marker apm binary not found
        Pytest->>Test: skip
    end

Loading

Trade-offs

• Chose addopts = "-m 'not benchmark and not live'" over a new requires_live_tests marker. Smaller diff; preserves the existing pytest.mark.live declaration in pyproject.toml. Rejected adding to _MARKER_CHECKS because that would force a marker rename across test_skill_bundle_live.py for no functional gain.
• Dropped the verbose "HERO SCENARIO" echo block. Pytest reports per-test results; the bash echoes were a static list that didn't reflect what actually ran. Per the PROSE principle: "Favor small, chainable primitives over monolithic frameworks."
• Did NOT add a meta-test that asserts every integration file has a marker. Static "is this file hermetic?" classification is unsound (mocked subprocess imports look identical to live ones). The marker registry IS the safety net — if a test fails in CI without preconditions, the signal arrives where it's actionable.
• Bumped CI timeout 20 → 30 min. ~45 previously-unwired files start running; conservative ceiling absorbs the worst case while we learn the real wallclock.

Benefits

1. Script length: 778 → 402 lines (-48%); the deleted block is mechanical scaffolding, not logic.
2. New integration tests run in CI without script edits — the bug class behind acceptance criterion 2 of Refactor test-integration.sh to fixture-driven pytest discovery (eliminate manual test wiring) #1166 is closed by construction.
3. One pytest exit code per CI run (vs 28); failures are reported by pytest's own collection/summary, not bash exit-on-first-fail.
4. _MARKER_CHECKS is now the single source of precondition truth — bash branches no longer race the registry.
5. test_skill_bundle_live.py no longer at risk of accidentally hitting live GitHub in CI when discovery widens.

Validation

uv run --extra dev ruff check src/ tests/:

All checks passed!

uv run --extra dev ruff format --check src/ tests/:

733 files already formatted

uv run pytest tests/unit -q (full unit suite):

8156 passed, 1 warning, 30 subtests passed in 69.78s (0:01:09)

uv run pytest tests/integration --collect-only -q | tail -3:

664/680 tests collected (16 deselected) in 6.99s

The 16 deselected are test_skill_bundle_live.py's 9 live tests + 7 benchmark-marked items — exactly the set we wanted gated out by default.

Script syntax check

$ bash -n scripts/test-integration.sh
$ wc -l scripts/test-integration.sh
402 scripts/test-integration.sh

Scenario Evidence

#	Scenario (user promise)	Principle(s)	Test(s) proving it	Type
1	New integration test file added; CI runs it without bash edits	DevX	tests/integration/conftest.py::pytest_collection_modifyitems (the discovery contract) + the new pytest tests/integration/ invocation in scripts/test-integration.sh	integration
2	Test that needs the apm binary skips cleanly when binary missing	DevX, Portability	tests/integration/conftest.py::_has_apm_binary + 11 newly-marked files	integration
3	test_skill_bundle_live.py does NOT execute in default CI	Secure by default	pyproject.toml addopts = "-m 'not benchmark and not live'"; verified by --collect-only showing 16 deselected	integration
4	test_transport_selection_integration.py (network-integration) RUNS in CI	DevX	ci-integration.yml adds APM_RUN_INTEGRATION_TESTS: "1" to job env	e2e

How to test

• Local collect: uv run pytest tests/integration --collect-only -q | tail -3 → expect 664/680 tests collected (16 deselected).
• Local run (subset, hermetic): APM_E2E_TESTS=1 uv run pytest tests/integration/test_compile_constitution_injection.py -v → all pass without GitHub token or built binary.
• Local script smoke (requires GITHUB_APM_PAT + built binary): uv run ./scripts/test-integration.sh → expect single pytest tests/integration/ invocation in the log; preconditions skip cleanly per marker.
• CI dry-run: enqueue this PR into the merge queue; the integration-tests job in .github/workflows/ci-integration.yml should pass within the 30-min timeout.
• Drift trap: drop a new tests/integration/test_drift_check_pr2.py with a single def test_pass(): assert True; without editing the script, confirm --collect-only picks it up.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

Refactors the integration-test harness to rely on pytest discovery over tests/integration/ (instead of a hand-maintained per-file enumeration), with marker-based gating used to preserve conditional execution in CI/local runs.

Changes:

• Simplifies scripts/test-integration.sh to invoke pytest tests/integration/ once and rely on marker-driven skips.
• Adds/adjusts requires_* markers across integration tests to match required preconditions (binary, tokens, runtimes).
• Updates CI + pytest config to keep live tests opt-in and to ensure network-integration tests still run in CI.

Show a summary per file

File	Description
scripts/test-integration.sh	Removes per-file pytest invocations; runs the full tests/integration/ directory once and prints an env probe.
.github/workflows/ci-integration.yml	Sets APM_RUN_INTEGRATION_TESTS=1 for the integration job and increases timeout.
pyproject.toml	Deselects live tests by default via addopts.
tests/integration/test_uninstall_multi_e2e.py	Adds requires_apm_binary marker alongside GitHub token gating.
tests/integration/test_uninstall_dry_run_e2e.py	Adds requires_apm_binary marker alongside GitHub token gating.
tests/integration/test_skill_integration.py	Adds requires_apm_binary marker alongside GitHub token gating.
tests/integration/test_mcp_env_var_copilot_e2e.py	Adds markers for binary + Copilot runtime preconditions.
tests/integration/test_marketplace_e2e.py	Adds requires_apm_binary marker.
tests/integration/test_local_install.py	Adds requires_apm_binary marker.
tests/integration/test_local_content_audit.py	Adds requires_apm_binary marker.
tests/integration/test_link_rewrite_e2e.py	Adds requires_apm_binary marker.
tests/integration/test_intra_package_cleanup.py	Adds requires_apm_binary marker.
tests/integration/test_install_verbose_redaction_e2e.py	Adds requires_apm_binary marker.
tests/integration/test_install_subdir_dedup_e2e.py	Adds requires_github_token marker for a network+token-dependent test.
tests/integration/test_global_scope_e2e.py	Adds requires_apm_binary marker.
tests/integration/test_apm_dependencies.py	Adds requires_github_token marker for real-repo integration coverage.
docs/src/content/docs/contributing/integration-testing.md	Updates contributor docs to describe the orchestrator script as CI’s entry point.
CHANGELOG.md	Adds an Unreleased entry describing the integration-test harness refactor.

Copilot's findings

• Files reviewed: 18/18 changed files
• Comments generated: 14

[danielmeppiel]

APM Review Panel — advisory recommendation for PR #1247

Note

Eight panelists ran in parallel. Ship recommendation: ship-with-nits-addressed. All recommended findings have been addressed in commit 3eea1242. Remaining items are nits; resolve at author discretion.

Activation

Persona	Active	Why
python-architect	✅	Default
cli-logging-expert	✅	Script output reframed
devx-ux-expert	✅	Default
supply-chain-security-expert	✅	Default
oss-growth-hacker	✅	Default
auth-expert	❌	No auth surface touched (verified: zero changes under src/apm_cli/core/auth.py, token_manager.py, downloaders, host utils)
doc-writer	✅	CHANGELOG.md + docs/src/content/docs/contributing/integration-testing.md modified
test-coverage-expert	✅	Refactor changes test-discovery surface; mandatory unless docs-only

Findings addressed in 3eea1242

#	Severity	Persona	Summary	Fix
1	recommended	supply-chain-security	test_registry_client_integration.py makes live HTTP to api.mcp.github.com with no marker; widened discovery would fire it on every dev pytest tests/integration/	Added pytestmark = pytest.mark.requires_network_integration
2	recommended	test-coverage-expert	test_runtime_smoke.py not in old script, no marker; downloads real codex/llm binaries via setup-codex.sh/setup-llm.sh	Added pytestmark = pytest.mark.requires_e2e_mode
3	recommended	doc-writer	integration-testing.md used pre-fix/post-fix phrasing ("the script no longer enumerates"); violates docs-current-behaviour-only convention	Reframed positively; no history reference
4	recommended	devx-ux-expert	live marker silently deselected by addopts but absent from the documented marker registry table	Added a row to the marker table with the opt-in command (pytest -m live tests/integration -v)
5	nit	cli-logging-expert	Stale banner line "Tests comprehensive runtime scenarios..." conflicts with thin-orchestrator identity	Replaced with one-line description matching the header comment
6	nit	cli-logging-expert	Vestigial inline comment # Integration Testing Coverage! on setup_runtimes call	Removed
7	nit	python-architect	APM_RUN_INTEGRATION_TESTS echoed but not exported, asymmetric with APM_E2E_TESTS	Added export for symmetry
8	nit	python-architect	Timeout bump from 20→30 min not explained in YAML	Added inline comment citing the discovery widening

Findings carried forward as follow-up suggestions (not addressed in this PR)

#	Severity	Persona	Summary	Reason for deferral
A	nit	devx-ux-expert	Pre-existing emoji in log_info/log_success/log_error (lines 48, 52, 56) violates encoding.instructions.md	Pre-existing, out of scope for PR2; would be churn touching every script that sources these helpers. Right venue: a dedicated cleanup PR.
B	recommended	supply-chain-security	Broader audit suggested for ~30 unmarked test files	Audited and verified clean: ran grep -nE "requests\.|httpx\.|urlopen|setup-codex|setup-llm|setup-copilot" tests/integration/test_*.py filtered to files without pytestmark. Only 4 files match: the 2 fixed above (#1, #2), and 2 hermetic files that mock the network (test_credential_fill_disambiguation.py, test_install_local_bundle_e2e.py). No further gaps.
C	nit	oss-growth-hacker	Release-note beat: "adding integration test is now a single file drop"	Surfaced for the next release-note draft, not a PR change.

Diagrams (from python-architect)

Legend: registry-pattern view of where each touched file sits in the marker-driven test discovery system.

classDiagram
    direction LR
    class conftest_py {
        Module -- tests/integration/conftest.py
        +_MARKER_CHECKS dict
        +pytest_collection_modifyitems
        +apm_binary_path fixture
    }
    class pyproject_toml {
        Config -- pyproject.toml
        +addopts str
        +markers list
    }
    class test_integration_sh {
        Script -- scripts/test-integration.sh
        +run_e2e_tests
        +setup_runtimes
    }
    class ci_integration_yml {
        Workflow -- ci-integration.yml
        +env APM_E2E_TESTS
        +env APM_RUN_INTEGRATION_TESTS
    }
    class TestModule {
        Pure
        +pytestmark list
    }
    ci_integration_yml ..> test_integration_sh : invokes
    test_integration_sh ..> conftest_py : pytest discovers
    conftest_py ..> TestModule : skips via markers
    pyproject_toml ..> conftest_py : marker declarations

Loading

Validation re-run after fixes

$ uv run --extra dev ruff check src/ tests/
All checks passed!

$ uv run --extra dev ruff format --check src/ tests/
733 files already formatted

$ uv run pytest tests/integration --collect-only -q | tail -1
664/680 tests collected (16 deselected) in 2.90s

Collection count unchanged — the new module-level markers replace runtime self-skips with declarative gating, no behavioural drift.

cc @sergio-sisternes-epam — the panel is advisory; merge gating remains with you.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com