Repository navigation
test: simplify test ownership and remove implementation assertions - #194
Merged
Merged
Conversation
Signed-off-by: lucarlig <luca.carlig@ibm.com>
lucarlig
marked this pull request as ready for review
October 2, 2026 10:57
lucarlig
requested review from
gandhipratik203 and
msureshkumar88
as code owners
October 2, 2026 10:57
msureshkumar88
approved these changes
Oct 2, 2026
msureshkumar88
left a comment
Collaborator
There was a problem hiding this comment.
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.
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
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 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
tests/test_repository_policy.py.Remove or consolidate
TESTING.mddocuments the keep/remove criteria and ownership.AGENTS.mdand 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 || trueexpression always succeeded.Validation
make plugins-validate: 41 passed, no skips.actionlinton 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.