Strengthen tests: MSI 74% → 80%, fix quoted-pair bug#54
Merged
Conversation
Targeted test-strengthening pass to kill surviving mutants identified by Infection, plus a real bug fix uncovered along the way. All additions are pure test work except for the one-line parser fix in validateQuotedContent. Metrics: - Infection MSI: 74% → 80% (+6 pp) - Covered Code MSI: 79% → 85% (+6 pp) - Surviving mutants: 219 → 159 (−60) - Overall line coverage: 89.6% → 91.2% (+1.6 pp) - Parse.php line cov: 86.69% → 87.98% (+1.3 pp) - Tests: 65 → 75 (+10) - Assertions: 489 → 589 (+100) Bug fix (Parse.php): - validateQuotedContent: restored a `continue` after handling a backslash-escape quoted-pair. Without it, the outer qtextSMTP byte check at line 1230 runs on the backslash itself and incorrectly reports "Invalid character in quoted string: byte 92" for any quoted local-part containing a valid backslash escape under validateQuotedContent=true. Regression introduced in commit 5066d2b during a Scrutinizer cleanup that mistakenly flagged the continue as redundant. New test testQuotedStringContentValidation::boundary-asserts covers the fix. Test strengthening: - testFactoryPresetsHaveExpectedRuleValues — single source-of-truth table asserting all 17 rule properties of each of four factory presets (rfc5321, rfc6531, rfc5322, rfc2822). Kills ~60 boolean-flip mutants across ParseOptions in one pass. - testLengthLimitsDefaultsMatchRfc — exact defaults for createDefault() and createRelaxed() plus the no-arg constructor. Kills 7 integer boundary mutants. - testEveryErrorCodeHasASeverity (strengthened) — now asserts each code maps to its specific Warning or Critical severity rather than just asserting a severity exists. Kills 13 MatchArmRemoval mutants on the severity() match arm. - testDomainLabelHyphenRejection — leading/trailing hyphen labels rejected with the exact ParseErrorCode::DomainLabelStartsOrEndsWithHyphen. Exercises mb_substr / mb_strlen mutants on that line. - testFqdnGateRejectsBadDotPositions — covers all three branches of the dotPos check (false, 0, strlen-1) for RFC 5321 FQDN. - testDomainLabelInvalidCharactersRejected — empty-label case from a leading-dot domain. - testLocalPartLengthBoundary / testTotalLengthBoundary / testQuotedLocalPartLengthIncludesWireDquotes — exact-boundary tests (N accepted, N+1 rejected) for the three RFC 5321 §4.5.3.1 limits; cover the quoted wire-form DQUOTE accounting. - testMultipleInvalidAddressesReasonIsPlural — covers the second-error branch in parseMultiple that flips the top-level reason to plural. - testLegacyAsciiOnlyPatternAcceptsTrailingDot — exercises the legacy/non-strict regex branch (allowUtf8LocalPart=false, allowObsLocalPart=false, rejectC0Controls=false). - testC1ControlInQuotedStringRejectedUnderRfc6531 — hits the C1 control check inside quoted-string validation. - testQuotedStringContentValidation — extended with four boundary cases: byte 0x1F (US, end of C0 range), byte 0x7F (DEL, start of DEL+ range), byte 0x20 (SPACE, lower valid bound), byte 0x7E (~, upper valid bound). Plus backslash+0x7F (upper escape bound) rejection. - Small doc note: ParseErrorCode::TrailingBackslashInQuotedString is unreachable in current code because STATE_QUOTE's backslash-counting logic always closes the quote before a lone trailing backslash can land in local_part_parsed. Kept as defensive code with a test comment explaining the unreachability. Tooling: - infection.json5 thresholds raised to minMsi=80, minCoveredMsi=85 to lock in the new baseline so future regressions fail. ROADMAP updates: - Infection entry reflects new thresholds and progress toward 85% goal. - Parse.php line-coverage entry updated with current number and a note that ≥95% remains aspirational; remaining gaps are defensive / unreachable code paths.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #54 +/- ##
============================================
+ Coverage 89.95% 90.88% +0.92%
Complexity 382 382
============================================
Files 6 6
Lines 986 987 +1
============================================
+ Hits 887 897 +10
+ Misses 99 90 -9
🚀 New features to boost your workflow:
|
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
1 participant
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
Targeted test-strengthening pass that kills ~60 surviving Infection mutants and surfaces one real parser bug introduced during an earlier Scrutinizer cleanup.
Bug fix
Parse::validateLocalPart()— missingcontinueafter quoted-pair handling.In commit
5066d2b(the v3.0 "Scrutinizer cleanup" PR), acontinuestatement was removed from the quoted-pair handler on the mistaken belief that it was redundant after$i++in aforloop. It was not redundant — the subsequent qtextSMTP byte-check on line 1230 then runs against the backslash byte itself (92), and rejects any valid backslash-escape with "Invalid character in quoted string: byte 92" whenevervalidateQuotedContentis enabled.The escape target is correctly vetted by the existing
quoted-pairSMTP %d32-126check just above; restoring thecontinueskips the redundant qtextSMTP rejection. Added tests for"a\\ b"@example.com(valid escape of SPACE) and"a\\~b"@example.com(valid escape of~) would have caught this — they now exist.Metrics
Infection thresholds in
infection.json5raised tominMsi=80/minCoveredMsi=85to lock in the new baseline.New/strengthened tests (10)
High-leverage first:
testFactoryPresetsHaveExpectedRuleValuestestEveryErrorCodeHasASeverity(strengthened)testLengthLimitsDefaultsMatchRfctestQuotedStringContentValidation(extended)testToJsonEmitsUnescapedUnicode(via v3.3 work, unchanged here)Plus smaller targeted fixes:
testDomainLabelHyphenRejection— leading/trailing hyphen labels with specific error codetestFqdnGateRejectsBadDotPositions— all three branches of FQDN dotPos checktestDomainLabelInvalidCharactersRejected— empty-label (leading dot)testLocalPartLengthBoundary/testTotalLengthBoundary/testQuotedLocalPartLengthIncludesWireDquotes— exact-boundary N vs N+1 for RFC 5321 §4.5.3.1 limits, including quoted-wire-form DQUOTE accountingtestMultipleInvalidAddressesReasonIsPlural— second-error branch that flips top-level reason to pluraltestLegacyAsciiOnlyPatternAcceptsTrailingDot— exercises legacy/non-strict regex pathtestC1ControlInQuotedStringRejectedUnderRfc6531— C1 control check inside quoted-stringDocumentation
One doc note added:
ParseErrorCode::TrailingBackslashInQuotedStringis unreachable in current code becauseSTATE_QUOTE's backslash-counting logic always closes the quote before an unescaped lone trailing backslash can land inlocal_part_parsed. Kept as defensive code; a comment points this out so future readers know the assertion can't be reached via the public API.ROADMAP updated: both test-strengthening and Parse.php-coverage items flipped to partial
[~]with current numbers.Test plan
composer cipasses (cs:check, PHPStan level 8, 75 tests / 589 assertions)composer infectpasses at new thresholds (80% MSI / 85% covered MSI)continuechange is only observable whenvalidateQuotedContent=true(not a v3.x preset default outsiderfc5321()/rfc6531()strict presets); boundary tests confirm correct acceptance of valid escapes and rejection of boundary-violating onesWhat's next
Remaining from the "Quality and Infrastructure (ongoing)" ROADMAP section: