Skip to content

test: simplify test ownership and remove implementation assertions - #194

Merged
lucarlig merged 1 commit into
mainfrom
user/luca/simplify-test-architecture
Oct 2, 2026
Merged

lucarlig merged 1 commit into
mainfrom
user/luca/simplify-test-architecture

Conversation

@lucarlig

@lucarlig lucarlig commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

The catalog suite mixed functional tests with assertions about workflow recipes, documentation, tool versions, stubs, and private module layout. That made harmless refactors fail and duplicated checks already performed by builds and artifact jobs.

This change gives each test a behavioral owner and replaces the omnibus catalog suite with focused discovery, selection, release, coverage, Git/CLI, and selection-validator tests. The catalog file shrinks from 4,773 to 455 lines and from 134 test methods to 26. Two useful security policies move to their own suite. Repository tooling now runs 41 tests with no skips, compared with 148 tests and three skips before the change.

Keep

  • Plugin algorithm, detection/redaction/privacy, Redis failure/TLS, hook dispatch, payload isolation, and observability regressions.
  • Packaging invariants, mixed-language CI routing, release/version decisions, coverage aggregation, and wheel compatibility/installation behavior.
  • Action SHA pinning and expiring Cargo advisory exceptions, in tests/test_repository_policy.py.
  • Actual build, lint, type-check, security, coverage, mutation, and isolated artifact jobs.

Remove or consolidate

  • Catalog assertions about exact YAML/Makefile recipes, documentation text, stub/source layout, benchmark scripts, tool versions, and the current plugin inventory.
  • Repeated fixture repositories, Git setup, CLI calls, and equivalent invalid-input variations. Representative input classes use subtests; a few real Git tests retain the I/O boundary.
  • The exporter test that reads a Makefile recipe and the wheel test that freezes argparse help text.
  • Generated empty TODO tests, non-null constructor checks, and generic Pydantic serialization/field tests. Generated Python tests now invoke every selected hook through the wrapper and compiled Rust extension against real CPEX.

TESTING.md documents the keep/remove criteria and ownership. AGENTS.md and development guidance use the same rules. Catalog CI explicitly validates the real repository, then runs behavior and security-policy tests. Scaffold CI keeps actual generated-plugin builds and corrects the invalid-hook check, whose previous && exit 1 || true expression always succeeded.

Validation

  • make plugins-validate: 41 passed, no skips.
  • Generated default and all-hooks plugins, built in an isolated scratch Cargo workspace: 1 + 12 Python hook tests passed with real CPEX and compiled Rust extensions; 4 generated Rust tests passed with nextest.
  • ICA exporter behavior suite in an isolated Python environment: 78 passed.
  • actionlint on both changed workflows; Ruff lint/format on the new catalog/policy code; changed-file pre-commit checks; diff whitespace checks: passed.

The full plugin/platform CI matrix and gateway suites were not run locally. An all-files pre-commit run found pre-existing whitespace issues in five untouched files; those formatting edits were reverted and the changed-file checks pass. Plugin runtime implementations and versions are unchanged.

Signed-off-by: lucarlig <luca.carlig@ibm.com>
@lucarlig
lucarlig marked this pull request as ready for review October 2, 2026 10:57

@msureshkumar88 msureshkumar88 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of PR #194 at commit 6aa794a

No blocking issues found. Approve, subject to required CI checks passing.

Scope and necessity

  • No linked closing issue or issue-number reference was found in the PR description. The stated problem is brittle tests that assert implementation details and duplicate build checks, rather than a production incident.
  • This is a repository test/tooling change. It changes neither UI nor backend plugin runtime behavior. No Alembic migration, schema change, data migration, or plugin version bump is needed because core plugin implementations are unchanged.
  • The cleanup is reasonable and internally consistent. A smaller patch could fix only the invalid-hook shell check, but would not address the broader test-ownership problem described by this PR.
  • Reviewed the 13 PR files against the refreshed main merge base, plus the unchanged catalog, scaffolder, wrapper/engine templates, generated Makefile, and release artifact workflows relevant to their behavior. No unrelated changes identified.

Implementation and surrounding behavior

  • The replacement catalog suite keeps mixed-language discovery and workspace validation, packaging/entry-point/version invariants, changed-path routing, mutation dependency selection, canonical release tags, version-bump and initial-release decisions, coverage aggregation, CLI error handling, and selection-payload validation.
  • Fixture setup is consolidated. Real Git/CLI cases remain where the boundary matters; table-driven cases exercise distinct policies without repeating whole repositories.
  • Catalog CI now validates the actual repository directly, rather than depending on tests of its current inventory. The manifest path filter now matches the manifests' actual package location.
  • The invalid-hook check correctly fails if the scaffolder unexpectedly succeeds. Its former shell expression always succeeded.
  • Generated Python tests now call every selected hook through the wrapper. The wrapper requires the Rust extension; pre-hook failures produce a violation, while post-hook failures return error metadata. The new assertions detect both forms rather than accepting a fail-open result as a successful smoke test.
  • Removing the exporter Makefile-text test and wheel help-wording test is appropriate: they did not prove artifact construction or wheel installation behavior. Actual artifact jobs and wheel selection/install tests remain.

Security, performance, compatibility, and diagnostics

  • No introduced exploitable weakness or concrete CWE finding identified. SHA pinning and dated Cargo advisory exception checks remain in a dedicated policy suite, and workflow/advisory changes trigger that suite. Production security controls are unchanged.
  • Lower fixture duplication and fewer subprocess-heavy tests reduce test maintenance and execution overhead. No production performance change is introduced, and no benchmark speedup is claimed.
  • No public API, config, hook signature, package version, or persistent data shape changes. Expected blast radius is repository validation and newly scaffolded test files; existing plugin consumers are unaffected.
  • No new runtime dead code or logging/exception regression identified. Empty TODO tests and trivial constructor/serialization assertions are removed. CLI malformed-input behavior still checks a clean nonzero exit without a traceback.
  • Documentation accurately distinguishes controlled hook shims, generated real-CPEX smoke tests, isolated artifact installation, and gateway testing ownership.

Testing and nonblocking opportunities

  • Independently ran make plugins-validate: repository validation succeeded; all 41 tests passed with no skips.
  • Independently verified typo_hook rejection: exit status 1 with the expected invalid-hook diagnostic. Diff whitespace check passed.
  • At the final CI snapshot, 28 checks succeeded, 11 remained in progress, and 4 were skipped; no failures were reported. These counts are a snapshot, not a claim that the full matrix passed.
  • Did not locally rebuild generated Rust extensions, rerun every plugin suite, measure numeric changed-line coverage, or run gateway integration/E2E suites. Generated default/all-hooks builds and hook tests are exercised by scaffold CI; existing plugin and artifact checks remain their relevant owners. Gateway E2E additions are unnecessary for this tooling-only change.
  • Optional test improvement: parameterize the new-plugin initial-release Git case over Rust and Python. The current case covers a new Python plugin and version bumps for both languages, but removes the old dedicated new-Rust-plugin case. This is a coverage opportunity, not an observed implementation defect.
  • Optional hardening of the invalid-hook CI check: verify the expected validation diagnostic as well as a nonzero exit, so an unrelated early crash cannot satisfy this negative check. Positive scaffold builds already reduce this risk.

Approval concerns the reviewed commit and code quality. Required CI completion remains the merge gate.

@lucarlig
lucarlig merged commit c105ecd into main Oct 2, 2026
44 checks passed
@lucarlig
lucarlig deleted the user/luca/simplify-test-architecture branch October 2, 2026 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants