Release 5.16.2: BIMI x/y attribute fix; narrower dnssec exceptions#254
Merged
Conversation
Drop tests that asserted on contrived inputs solely to keep coverage
high, and fix the underlying bugs they were papering over.
- checkdmarc/bimi.py: SVG x/y forbidden-attribute capture was reading
the wrong xmltodict keys ("x"/"y" matches child elements, not
attributes), so the check_svg_requirements rejection never fired
on real SVGs. Switch to "@x"/"@y" and fix a typo that wrote the
y value into metadata["x"].
- checkdmarc/dnssec.py: Narrow three broad "except Exception" clauses
to (dns.exception.DNSException, OSError, EOFError) per AGENTS.md.
- tests/test_bimi.py: Delete TestSvgMetadataExtraAttributes; it built
malformed SVGs (with child <x>/<y>/<description>/<overflow> elements)
to exercise unreachable branches and even asserted on a known source
bug. Replace with one honest end-to-end test that proves the fixed
forbidden-attribute path works.
- tests/test_init.py: Delete testKnownGoodMocked — it patched all nine
check_* helpers to return canned valid dicts then asserted the canned
values came back. The network counterpart testKnownGood is the real
test.
- tests/test_dnssec.py: Delete testValidationExceptionContinues — it
existed only to exercise the broad except we've now narrowed away.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
- Coverage 96.39% 96.34% -0.05%
==========================================
Files 24 24
Lines 5715 5695 -20
==========================================
- Hits 5509 5487 -22
- Misses 206 208 +2 ☔ View full report in Codecov by Sentry. |
Trailing blank line at end of file. The previous commit ran ruff check but missed ruff format. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The narrowed exception handler in test_dnssec catches the actual exception types dnspython raises. Exercise it with a real dns.dnssec.ValidationFailure (the exception thrown on a genuine bad signature) and assert on the function's contract: invalid DNSSEC reports as not validated, rather than propagating. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dns.dnssec re-imports it from dns.exception but doesn't re-export it, so pyright flags the dns.dnssec.ValidationFailure reference as a reportPrivateImportUsage error. Use the canonical path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
A self-audit against the principle that tests should assert on observable behavior — not just exercise lines — turned up a handful of tests written purely to chase coverage. Several of them sat on top of unfixed bugs and even labeled themselves as such. This PR removes those tests, fixes the underlying bug they were hiding, and narrows a broad exception handler that one of them depended on.
Bug fix
get_svg_metadatawas readingsvg["x"]/svg["y"], but xmltodict prefixes attributes with@, so those keys only match child elements — which real SVGs don't have. Net effect: the existingcheck_svg_requirementsrejection of x/y attributes on the root<svg>element has been a no-op. Switched to@x/@yand fixed a typo where the y-value was being written tometadata["x"].Test cleanup
TestSvgMetadataExtraAttributes(tests/test_bimi.py). Built malformed SVGs with child<x>/<y>/<description>/<overflow>elements to hit branches that are unreachable from real SVG input. One test even openly stated: "The branch wrote to metadata['x'] (not metadata['y']) per the source bug — the goal here is line coverage, not correctness."x="0" y="0"attributes, asserts they're captured, and assertscheck_svg_requirementsflags them.testKnownGoodMocked(tests/test_init.py). Patched all ninecheck_*helpers to return canned valid dicts, then asserted the canned values came back. The network counterparttestKnownGoodis the real test.testValidationExceptionContinues(tests/test_dnssec.py). Existed only to exercise the broadexcept Exceptionclause we're narrowing here.Convention compliance
except Exceptionclauses incheckdmarc/dnssec.pytoexcept (dns.exception.DNSException, OSError, EOFError)per AGENTS.md's "Don't catchExceptionbroadly" rule.Coverage impact
Total coverage 96% (small honest drop). The lost percentage points represent branches that were either unreachable, buggy, or exercised tautologically — nothing of real value.
Test plan
ruff check --show-fixes— cleanpython -m pytest tests/— 433 passed, 10 skipped (network tests on CI)testRootXYAttributesCapturedproves the BIMI fix works end-to-end🤖 Generated with Claude Code