Achieve 100% test coverage for Adapter/* classes#143
Open
simon-mundy wants to merge 8 commits into
Open
Conversation
…rastructure - Add ~270 new test methods across 60 files, bringing line coverage from 84% (2809/3354) to 99.13% (3289/3318) - Achieve 100% coverage for Sql/*, ResultSet/*, Metadata/*, Container/* - Remove dead code in AbstractSql (unreachable processValuesArgument, defensive throw in processJoin) and AbstractResultSet (unreachable non-Iterator branches in valid/rewind, unreachable throw in initialize) - Remove 9 permanently-skipped tests that were redundant, had contradictory logic, or tested removed functionality - Replace all anonymous classes in tests with proper TestAsset classes for reusability and consistency - Fix invalid #[CoversMethod] attributes causing PHPUnit warnings - Fix #[CoversMethod] parentheses in method name strings - Add REFACTOR.md documenting completed work and remaining Adapter/* gaps Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Profiler::profilerStart(): remove unreachable else-branch. The union
type `string|StatementContainerInterface` guarantees only those two
types can be passed; the if/elseif handles both exhaustively, making
the else+throw dead code. Clean up unused imports (InvalidArgumentException,
is_string).
- AbstractPdo::checkEnvironment(): mark the throw inside
`if (!extension_loaded('PDO'))` with @codeCoverageIgnore. PDO is a
hard requirement — this branch cannot execute in any environment where
the class is loadable.
- AbstractResultSet::initialize(): add @PHPStan-Ignore for Traversable
assignment. After array and IteratorAggregate branches are handled, the
remaining Traversable is necessarily an Iterator, but PHPStan cannot
narrow it.
Add targeted tests and fix coverage attribution to close the remaining 29 uncovered lines across Adapter/*. New test assets: - TestConnection: extends AbstractConnection directly (not via AbstractPdoConnection) to test inherited methods like setConnectionParameters() and getResource() auto-connect - TestPlatform: extends AbstractPlatform without overriding quoteValue(), allowing direct coverage of the base class throw/escape paths Test changes: - AbstractConnectionTest: switch from ConnectionWrapper (which goes through AbstractPdoConnection overrides) to TestConnection so AbstractConnection methods are exercised directly - PdoTest: add missing #[CoversMethod] for formatParameterName, fix phpstan method.resultUnused warning - StatementTest: add double-execute test (bindParametersFromContainer early-return path) and clone-with-container test - Sql92Test: add tests for AbstractPlatform::quoteValue() throw and escape paths via TestPlatform - ProfilerTest: split combined test into separate string/container tests, remove dead TypeError assertion (covered by union type) - ParameterContainerTest: remove stale @PHPStan-Ignore Config: - phpunit.xml.dist: re-enable AdapterAwareTraitTest (the test works, the exclusion was a leftover)
Resolve conflicts and reconcile with upstream API changes (PR php-db#144 phpstan-type Profiler, PR php-db#145 provide-driver-defaults): - Profiler.php: take upstream's phpstan-typed version (supersedes PR's dead-code pass) - AdapterInterfaceDelegatorTest: union of both test sets; keep PR's #[Group]/ #[CoversMethod] plus upstream's testDelegatorWithPluginManager - ConnectionTest: keep upstream's testResource and the PR's real testConstructorWithPdoResourceSetsConnected (drop skipped placeholder) - Remove getDatabasePlatformName from test assets (API method removed upstream) - Rewrite TestPdoWithFeatures constructor to set properties directly (AbstractPdo constructor removed upstream) - Drop obsolete testCloneWithNullParameterContainerDoesNotClone (Statement parameter container is now non-nullable) - Remove invalid #[CoversMethod(...'__construct')] attributes in ConnectionTest and PdoTest that were marking tests risky and discarding coverage Suite: 1444 tests pass, PHPStan clean, PHPCS clean, 99.94% line coverage.
The trait was incorrectly covered (CoversMethod instead of CoversTrait) and its test was accidentally excluded from the suite. Now 1446 tests, 100% coverage.
ConnectionIntegrationTest ran in neither suite (excluded from unit, absent from the empty integration suite) and added no coverage the unit tests don't already provide. Also drop the exclude for AdapterServiceFactoryTest, which no longer exists.
This test checks that values are bound with the correct PDO type when a statement runs, which nothing else covers. It was excluded from the suite before, so it never ran. It is worth keeping, so now it runs.
f39542a to
63efcb8
Compare
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
Closes the remaining 29 uncovered lines across Adapter/* by adding targeted tests, fixing coverage attribution, and introducing two new test assets. Brings Adapter/* from 93% to 100% line coverage.
Root causes of the gaps
The uncovered lines fell into three categories:
Wrong test asset hierarchy —
AbstractConnectionTestusedConnectionWrapperwhich extendsAbstractPdoConnection. SinceAbstractPdoConnectionoverridessetConnectionParameters()and provides its ownisConnected(), the inheritedAbstractConnectionmethods were never exercised.Missing
#[CoversMethod]attribution —PdoTesttestedformatParameterName()extensively via data providers but didn't declare#[CoversMethod(AbstractPdo::class, 'formatParameterName')], so 16 lines of coverage weren't credited.Untested code paths —
Statement::bindParametersFromContainer()early-return when already bound (requires double-execute),Statement::__clone()with a non-null ParameterContainer, andAbstractPlatform::quoteValue()which was always tested throughSql92(which overrides it).New test assets
TestConnection(test/unit/Adapter/Driver/TestAsset/) — ExtendsAbstractConnectiondirectly, not throughAbstractPdoConnection. Provides a resource-basedisConnected()sodisconnect()andgetResource()auto-connect work correctly against the base class.TestPlatform(test/unit/Adapter/Platform/TestAsset/) — ExtendsAbstractPlatformwithout overridingquoteValue(), allowing direct coverage of the base class throw (no driver) and escape (with driver) paths.Test changes
AbstractConnectionTestConnectionWrappertoTestConnectionsoAbstractConnection::setConnectionParameters(),getResource()auto-connect, andgetDriverName()are exercised directlyPdoTest#[CoversMethod]forformatParameterName; added@phpstan-ignoreformethod.resultUnusedon expected-throw testStatementTesttestSecondExecuteSkipsBindingWhenAlreadyBound(double-execute covers theparametersBoundearly return) andtestCloneClonesParameterContainerWhenSetSql92TesttestAbstractPlatformQuoteValueThrowsWithoutDriverandtestAbstractPlatformQuoteValueEscapesWithDriverviaTestPlatformProfilerTesttestProfilerStartintotestProfilerStartWithStringandtestProfilerStartWithStatementContainer; removed deadTypeErrorassertion (union type covers it)ParameterContainerTest@phpstan-ignore argument.type(method acceptsmixed)phpunit.xml.distAdapterAwareTraitTest— the test works correctly, the exclusion was a leftover