test(validator): add negative controls to TestOrganizationNonASFSmoke for invalid orgs and ASF coupling - #1046
Conversation
…atter in TestOrganizationNonASFSmoke
… governance workflow
potiuk
left a comment
There was a problem hiding this comment.
LGTM — the premise is exactly right, and I confirmed it by experiment rather than by reading, because "these assertions cannot fail" is a claim worth testing.
The premise, demonstrated
TestOrganizationNonASFSmoke asserted x == [] throughout. That passes in two very different worlds: the validator ran and correctly found nothing, or the validator never ran at all. I disabled validate_asf_coupling (early return []) and re-ran both sets against the break:
| Result | |
|---|---|
the four pre-existing assert x == [] assertions |
all passed — completely blind to it |
| your three coupling negative controls | all failed |
So the suite was reporting green on a validator that had stopped working. That is the whole argument for this PR, and it holds.
The assertions are also the right shape. Each pins a count, a category, and message content:
assert len(coupling_violations) == 1
assert coupling_violations[0].category == ASF_COUPLING_CATEGORY
assert "announce@apache.org" in coupling_violations[0].messageAsserting only "non-empty" would have been the easy version and would fail to catch a validator that fires for the wrong reason. Naming the offending token in the message assertion means the test also documents what the rule is for.
This is the same class of gap as #984 — where direct unit tests stayed green if the call was deleted from run_validation — and it is the reason I went looking for a mutation here at all.
One thing worth knowing, no action needed
The tools/bitbucket/tests/test_bitbucket.py half of this PR adds test_cloud_get_pull_request_commits_rejects_repeated_next_url, which is byte-identical to the test #1047 landed while this was open.
I checked what that actually does on merge rather than assuming a conflict or a duplicate definition: computing the merge result with git merge-tree against current main gives one copy, not two — git recognises the identical addition and collapses it. So there is no F811 redefinition and nothing to fix. The net effect of merging this PR today is exactly the 95 lines of test_validator.py, and the bitbucket hunk is simply a no-op.
Flagging it only so the diff not matching what lands is not a surprise later.
Verified: 11 tests pass under -k yields_violation, and the suite is green with the change applied.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md § Opening a pull request.
Summary
Adds negative control test cases to
TestOrganizationNonASFSmokeintools/skill-and-tool-validator/tests/test_validator.pyfor each workflow surface (organization frontmatter, security intake, release housekeeping, contributor governance, and organization structure).Previously, assertions in
TestOrganizationNonASFSmokeonly verified that violation lists were empty (assert x == []), making it impossible to distinguish between "the checks ran and correctly found no violations" versus "the checks went silent or were not executed".Changes Made
Added 5 new negative control test cases to
TestOrganizationNonASFSmoke:test_invalid_organization_yields_violation: Asserts an unknown/invalidorganization:frontmatter value produces anORGANIZATION_CATEGORYviolation ("is not a known organization").test_security_intake_asf_coupled_body_yields_violation: Asserts security intake skills containing ASF-coupled email patterns (announce@apache.org) underorganization: independentproduce anASF_COUPLING_CATEGORYviolation ("asf-coupling").test_release_backend_asf_coupled_body_yields_violation: Asserts release housekeeping skills containingsvnrelease commands underorganization: independentproduce anASF_COUPLING_CATEGORYviolation ("asf-coupling"and"svn").test_contributor_governance_asf_coupled_body_yields_violation: Asserts contributor governance skills mentioningPMCvotes underorganization: independentproduce anASF_COUPLING_CATEGORYviolation ("asf-coupling"and"PMC").test_independent_org_structure_missing_file_yields_violation: Asserts incomplete organization adapters missingorganization.mdproduce anORGANIZATION_CATEGORYviolation ("missing required file 'organization.md'").Verification
uv run pytest tools/skill-and-tool-validator/tests/test_validator.py -k TestOrganizationNonASFSmoke: all 9 tests pass.fixes: #1009
Generated using Claude Code