Run the 746 integration tests CI never executed

[jgruberf5]

Fixes #130.

The problem

"P3 · Integration Tests · Backend" ran 206 of 952 integration tests. The other 746 never executed in CI on any branch.

The job's only integration command is make test-integration-full, which passes -m full. backend/pyproject.toml sets addopts = "-v --tb=short -m 'not full'". The two selectors are exact complements, so the CI set and the default set are disjoint — and nothing ran the larger half. test-backend-legacy explicitly passes --ignore=tests/integration, so it wasn't covering them either.

A green "P3 · Integration Tests" meant "the 22% carrying the full marker pass".

What that hid

Five tests had been failing unnoticed, all with ResponseValidationError on a missing is_default:

test_routes_blueprint_catalog.py::test_list_blueprint_sources
test_routes_blueprint_catalog.py::test_create_blueprint_source
test_routes_module_sources.py::TestListModuleSources::test_list_sources
test_routes_module_sources.py::TestGetModuleSource::test_get_source
test_routes_module_sources.py::TestCreateModuleSource::test_create_source

These are stale fixtures, not a product defect. Both files @patch their service class outright and hand-build the response dict; those dicts were never updated when is_default was added to the response models. The real serializers do emit it — module_source_service.py:240 computes it from the configured default URL/ref, blueprint_catalog_service.py:503 delegates to is_default_blueprint_source() — so the live endpoints were always fine.

Changes

• make test-integration — new target for the default marker set. The P3 job now runs both targets, covering tests/integration/ completely.
• test-integration-full: SUITE = integration-full — artifact filenames derive from SUITE, and both targets now run in one job, so sharing the value would have silently overwritten the first run's JUnit/coverage XML. Both pairs are uploaded.
• Five fixtures repaired, with the computed fields mirrored from the real serializers.
• Drift guard: each fixture helper now validates against its response model, so the next required field added to a schema fails at the fixture naming the field, instead of surfacing as a 500 buried in a FastAPI ExceptionGroup.
• Docs updated (TESTING.md, BRANCH_PROTECTION.md).

Verification

Check	Result
Newly-enabled set (make test-integration)	742 passed, 0 failed
full set (unchanged)	187 passed, 19 skipped
ruff 0.15.2 on changed files	clean
ci.yml	parses as valid YAML

The drift guard was break-tested: removing is_default from a fixture now fails at the helper with ValidationError ... is_default Field required, rather than the previous opaque response-validation 500.

Left for a follow-up decision

The full marker's declared meaning is "requires running Docker Compose app" (pyproject.toml:114), but the P3 job starts no Compose stack and those tests pass anyway — 187 passed, 19 skipped with nothing running. The marker looks vestigial, which is how the selector inversion survived. Whether to retire it is a separate call from restoring coverage, so this PR doesn't touch it.

[jgruberf5]

Self-review of #131

Reviewed the change against the failure mode it exists to prevent: a selector pair that can silently stop covering the suite. One defect found in my own fix, plus the checks that came back clean.

---

1. The new target inherited its selector instead of stating it (fixed in dbb0f4b)

test-integration was written with no marker expression at all, relying on pyproject.toml's addopts = "-v --tb=short -m 'not full'" to make it the complement of test-integration-full.

That reproduces the exact condition that caused #130. The pairing existed only as an implicit relationship between a Makefile target and a config file two directories away; an edit to addopts alone could have made the two targets overlap or leave a gap, and the only symptom would have been a quietly smaller test run behind a green check — which is precisely how 746 tests went unexecuted in the first place.

Both targets now spell out their selector, so the partition is visible in one place:

test-integration:      ... tests/integration/ -m 'not full' ...
test-integration-full: ... tests/integration/ -m full ...

Verified the selected set is byte-for-byte the same work: 742 collected explicitly, 206 with -m full, 948 total in tests/integration/ — an exact partition with no overlap and no remainder.

---

Checked and found correct

• Artifact collision avoided. PYTEST_COV_REPORT and PYTEST_JUNIT derive their filenames from SUITE (Makefile:47-48). Both targets now run in one job, so leaving test-integration-full at SUITE = integration would have had the second run silently overwrite the first's junit-integration.xml — losing exactly the results this PR exists to surface. It is now integration-full, and the upload step lists both pairs.
• Nothing else consumes the renamed artifacts. junit-integration.xml / coverage-integration.xml appear only in ci.yml's upload step, which this PR updates. No other workflow invokes either make target.
• No job timeout to breach. ci.yml declares no timeout-minutes anywhere, so the added ~9 minutes of runtime has nothing to trip.
• No other job had the same inversion. test-backend-unit, test-backend-component, test-contracts pass no marker override. test-backend-legacy runs tests/ but explicitly --ignore=tests/integration, which is why nothing else was covering those 746.
• The drift guard is safe on every call site. All eight callers of the two fixture helpers pass only benign overrides (id, name, url); none deliberately supplies invalid data that the new model_validate() would now reject.
• .PHONY updated — test-integration is registered, so it can't be shadowed by a file of that name.
• Fixture repairs are faithful, not merely silencing. is_default is mirrored from what the real serializers compute (module_source_service.py:240, blueprint_catalog_service.py:503) rather than invented, and credential_recommendation was added alongside it for the same reason.

Verification

Check	Result
-m 'not full' set (newly enabled in CI)	742 passed, 0 failed
-m full set (behaviour unchanged)	187 passed, 19 skipped
Repaired files	23 passed
ruff 0.15.2, tests/integration/	clean
ci.yml	parses as valid YAML
make -n test-integration	quoting survives make → pytest tests/integration/ -m 'not full' ...

The drift guard was break-tested: dropping is_default from a fixture now fails at the helper with ValidationError ... is_default Field required naming the model and field, instead of the previous ResponseValidationError buried inside a FastAPI ExceptionGroup.

Deliberately not done

• No meta-test asserting CI runs both targets. It would have to parse ci.yml and the Makefile as text; there is no precedent for that in backend/tests/, and a brittle assertion over CI config is a poor trade for a check the explicit selectors now make legible.
• Fail-fast left as is. If make test-integration fails, the step aborts and test-integration-full does not run. That is standard CI behaviour and the artifact upload already tolerates missing files (if-no-files-found: ignore).
• The full marker itself is untouched. It claims to mean "requires running Docker Compose app" while the job starts no stack and the tests pass anyway. Retiring or redefining it is a separate decision from restoring coverage.

[jgruberf5]

CI found a real bug — fixed in a1f2e9c

The first run of this PR failed on the very job it changes, which is the outcome the PR was arguing for. Reporting it in full rather than just pushing a green build.

What failed

740 passed, 2 failed, 206 deselected in 675.02s
FAILED tests/integration/test_routes_api.py::TestHealthCheck::test_health_check      - assert 500 == 200
FAILED tests/integration/test_routes_api.py::TestHealthCheck::test_api_health_check  - assert 500 == 200
TypeError: the JSON object must be str, bytes or bytearray, not MagicMock

Root cause — not a test artefact

core/maintenance.py:64:

try:
    r = _get_redis()
    raw = r.get(MAINTENANCE_KEY)
    if raw:
        result: dict[str, str] = json.loads(raw)   # <-- unguarded
        return result
except (RuntimeError, redis.ConnectionError, redis.RedisError):
    return None

The except list covers Redis being absent or unreachable, but not the value being unreadable. get_maintenance_status() is called by MaintenanceMiddleware.dispatch on every request, so anything unparseable under bnkforge:maintenance_mode — truncated JSON, a value written by another tool, bad bytes — becomes a 500 for the entire API, not just for maintenance endpoints. The function's own docstring says it "Returns None if Redis is not configured (graceful degradation)"; it just did not extend that promise to the data.

Now catches TypeError/ValueError as well (JSONDecodeError and UnicodeDecodeError are ValueError subclasses, so decode failures are covered) and logs the ignored key. Checked the other two middlewares — this is the only json.loads on the request path.

Why local runs were green and CI was not

settings.REDIS_URL is unset in the test container, so _get_redis() raised RuntimeError and was caught before reaching json.loads. ci.yml:64 sets REDIS_URL globally, so CI got past that line. Reproduced locally by exporting CI's env — same two failures — then confirmed the fix against it.

Worth naming: the environment gap and the coverage gap compounded. Even had these tests been running, a machine without REDIS_URL would have reported them green.

The stub is left alone on purpose

The health tests patch redis.from_url with a bare MagicMock, so r.get() returns a mock. Making that a faithful double would be the easy fix — and would let both tests pass without the product fix, removing the only coverage that reaches this path. The sloppy stub is now harmless because the product handles unparseable input, which is the behaviour worth pinning.

Regression test

test_get_maintenance_status_survives_an_unparseable_value, parametrised over plain text, truncated JSON, invalid bytes, and a non-str object. Break-tested: narrowing the except back to TypeError alone fails 3 of the 4 cases.

Verification, under ci.yml's environment

Check	Result
tests/integration -m 'not full' with CI env	742 passed, 0 failed (was 740 / 2)
maintenance + backup + restore unit suites	43 passed
ruff 0.15.2	clean