Repository navigation
Clear the Mago lint baseline across the test suite - #196
Merged
simon-mundy merged 3 commits intoSep 29, 2026
Merged
Conversation
Drops 18 entries for issues that no longer exist: the seven per-component ExceptionInterface files deleted in PR php-db#190, four exception classes that are now final, three resolved unhandled-thrown-type and mixed-argument sites, and the unfulfilled-expect and prefer-test-attribute entries settled by the mutation coverage work in PR php-db#188. Suppressed issue totals are unchanged at 3948 lint and 1215 analyzer, so no new findings are masked.
Rewrites the assertion style, strict assertions and test attributes that Mago can fix automatically. Six sites needed manual repair afterwards. Four test methods lost their "test" prefix and collided with final PHPUnit\Framework\TestCase methods, so they are renamed after the behaviour they cover rather than the method they call. assertAttributeSame joins DeprecatedAssertionsTrait, since the rewrite assumed it existed. Like and NotLike assert null rather than the empty string for an unconfigured predicate: getIdentifier() and getLike() return null, and the old loose comparison only passed because '' == null. The uniqid() callback in ResultTest keeps its closure. As a first class callable it receives the three arguments PDOStatement::fetch() passes, which uniqid() does not accept. Source files are left untouched on this branch.
The first fix pass wrote the PHPUnit Test attribute fully qualified, which traded 3592 assertion and attribute issues for 1008 new no-fully-qualified-global-class-like ones. A second pass imports the attribute instead. The lint baseline now holds 348 issues, down from 3948. 249 sit in src and 99 in test, so test is no longer the bulk of the suppression. prefer-self-return-type is deliberately left unapplied: in ArrayObjectResultSetInterface it rewrites an intersection type to use self, which does not parse.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Applies the Mago lint fixes that can be made automatically, repairs the six sites where those fixes were wrong, and regenerates both baselines. Source files are untouched; every change is under
test/or in a baseline.Result
test/src/mago lint,mago analyzeandmago format --checkare clean. The assertion count is unchanged, so no assertion was weakened or dropped.test/fell from 94% of the suppressed issues to 28%.Commits
assertion-style(2070),prefer-test-attribute(1007) andstrict-assertions(515), plus the six manual repairs below.no-fully-qualified-global-class-likeissues.The six repairs
Four test methods lost their
testprefix and becamecount(), which overrides afinalPHPUnit\Framework\TestCase::count()— a fatal error at load. They are renamed after the behaviour they cover:countsStoredParameters,countsRowsInDataSource,countsPopulatedColumns,countsJoinedTables.assertAttributeEqualswas rewritten toassertAttributeSame, whichDeprecatedAssertionsTraitdid not provide. The trait now has it.ResultTestkeeps itsstatic fn() => uniqid()closure. Asuniqid(...)it receives the three argumentsPDOStatement::fetch()passes, anduniqid()takes at most two. The two other first class callable conversions inAbstractSqlFunctionalTestCasestub zero argument methods and are kept.One real bug found
strict-assertionsexposedLikeTestandNotLikeTestasserting that an unconfigured predicate returns''fromgetIdentifier()andgetLike(). Both returnnull; the assertions passed only because'' == null. They now assert null.prefer-self-return-type must not be applied
Mago marks it safe, but in
ArrayObjectResultSetInterfaceit rewritesResultSetInterface&ArrayObjectResultSetInterfacetoResultSetInterface&self, andselfis not permitted inside an intersection type — the file stops parsing. It is still 5 findings insrc, so anyone running--fixthere will hit a fatal. Worth an upstream report or amago.tomlexclusion before the next pass.prefer-first-class-callableneeds the same caution, since it ignores whether the closure was discarding arguments.What remains
348 lint (249
src, 99test) and 1215 analyzer findings, all still baselined. The analyzer set is where the real defects are, headed byAbstractResultSetcallingrewind(),current()andvalid()on a property typed to allowIteratorAggregate, which has none of them.