Further improve test coverage - #331
Conversation
There was a problem hiding this comment.
I think we should remove MigrateFromJiraEconomicsCommand instead of adding tests for it. The project has moved on from this.
And fix failing GitHub Actions.
AI Review:
Fix before merge — CLAUDE.md now contradicts the code, and one half of it will drift again
CLAUDE.md:26 still reads (threshold 62) and :56 still reads coverage-check coverage/clover.xml 62. More importantly, the ## Tests section still says "There is no DAMADoctrineTestBundle.
Tests are not wrapped in transactions and are not isolated from each other, so a test that writes must clean up after itself." For the three new base shapes the opposite now holds — a writing
test must not clean up after itself, it must let the rollback do it — and that paragraph is the instruction the next contributor or agent will follow, which makes it the one genuinely blocking
item here. Note the numbers will drift again on the next ratchet: task test:coverage:set-threshold (Taskfile.yml:84) rewrites only Taskfile.yml, composer.json and .github/workflows/pr.yml, so
either add CLAUDE.md to that foreach or drop the literal number from it and point at task test:coverage:check.
Fix before merge — one findOne() call where the assertion depends on which row came back
findOneBy([]) compiles to SELECT … LIMIT 1 with no ORDER BY — whatever row the storage engine hands back first. There are around thirty criteria-less findOne(X::class) calls in the PR and most
are harmless, but InvoiceExportFlowTest:24 takes an arbitrary Project and :192-193 then expects Test Testesen / test@economics.local.itkdev.dk as its lead, i.e. it assumes the arbitrary project
is the one fixture project that has one. That is one line of explicit criteria. The rest — ProductFlowTest:17 and IssueProductTypeTest:17 deriving the working project from an arbitrary Product,
ServiceAgreementFlowTest:22-24 and ServiceAgreementTypeTest:135-137,190-192 taking an arbitrary Project, Client and Worker — are stylistic and can go in a later sweep; they are stable in
practice because InnoDB scans in clustered-PK order.
Fix before merge — no new test calls parent::setUp()
All fifteen flow tests are setUp() { $this->bootTransactionalClient('ROLE_X'); … }, and no new file anywhere in the PR calls parent::setUp(). That is harmless only because
AbstractTransactionalFlowTestCase happens not to define setUp() — the moment anything is added there (a clock freeze, a mailer reset, the transaction start itself) fifteen classes silently skip
it. The smallest fix is to add the parent::setUp() calls now. The better one — give the base a setUp() that calls bootTransactionalClient($this->role()) behind an abstract protected function
role(): string, so most of those overrides disappear — can ride along with the follow-up below, which touches the same files.
Fix before merge — use_savepoints: true is now load-bearing and nothing says so
A hand-rolled connection-level transaction nests with the ones flush() opens, and without savepoints a nested rollBack() marks the outer transaction rollback-only, so every later operation in
that test dies with "the transaction has been marked for rollback only" rather than failing on what it asserts. That doesn't bite because config/packages/doctrine.yaml:40 already sets
use_savepoints: true under when@test — a pre-existing recipe line this PR now depends on. If someone prunes it as unused config, thirty-odd tests start failing in a way that points nowhere near
the cause. A one-line comment on it, or a sentence in CLAUDE.md alongside the rewrite above, costs nothing and prevents a bad afternoon.
Decide before merge — six assertions pin "guard violation → HTTP 500" as the contract
InvoiceExportFlowTest asserts 500 for a stale delete form on a recorded invoice (:77) and on one added to a project billing (:90), for exporting an unrecorded invoice (:101) and a project
billing invoice (:110), and twice more on export-selection (:170, :179). The assertions are accurate about today and the guards are genuinely covered, but they also turn the obvious improvement
— catch it, flash a message, redirect, or return 409 — into a test-breaking change. Either note in the class docblock that the 500s describe current rather than desired behaviour, or open a
follow-up and reference it from there. Not a code change necessarily, but a decision worth recording now rather than rediscovering later.
Decide before merge — where the +26pp comes from, before the gate becomes permanent
Roughly 1,850 of the added lines are entity accessor tests — SmallEntitiesTest 412, ProjectTest 299, IssueTest 214, InvoiceTest 201, ServiceAgreementTest 187, ClientTest 167, InvoiceEntryTest
164, WorklogTest 112, VersionTest 95 — setter/getter round-trips plus assertSame($e, $e->setX(…)) fluency checks. Cheap coverage on code that cannot realistically be wrong, and they make an
entity rename a two-file edit. Not worth blocking, but the 90% is not 90% of behaviour, and 85 as a permanent gate is being set partly on the back of it.
means an untested new feature area of a few hundred lines trips the gate, which is the intent — just confirm the team wants that ratchet now rather than 88 or 80, because relaxing it later reads
as a regression. Related: MigrateFromJiraEconomicsCommandTest (102 lines) covers what CLAUDE.md calls the last Jira remnant, so if that command is on its way out these tests protect nothing and
removing it will move the coverage number under the new gate.
Follow-up PR — disableReboot() buys the isolation by giving up request fidelity
Introducing transactional isolation is the right call — cleanup-by-convention is unenforceable and its failure mode is a test that passes for the wrong reason — but the way it is bought here has
a cost worth naming. To make one transaction span several requests the kernel must not reboot, hence AbstractTransactionalFlowTestCase:29. That means ServicesResetter never runs between
requests, and the test and the controller share one EntityManager, identity map included. No real request runs in that configuration. It is also why every flow test has to sprinkle
$this->entityManager->clear() by hand (ProductFlowTest:67,82,110,124,136 and so on): forget one and the assertion reads the in-memory object instead of the database and passes. A false pass is
worse than the pollution the transaction was added to prevent, and the guard against it is again convention. Fine to ship as-is; the fix is the DAMA paragraph below.
Follow-up PR — the rollback pattern exists in three copies, and nothing enforces it
Identical beginTransaction() in setup and isTransactionActive() + rollBack() in teardown appear in AbstractTransactionalFlowTestCase:35,38-48, AbstractFormTestCase:30,33-43 and
WorklogRepositoryFilterTest:47,52-62, and findOne()/requireEntity()/requireId() are duplicated verbatim between the first two. Worse than the duplication is that a test extending the base but
never calling bootTransactionalClient(), or overriding tearDown() without parent::, writes for real and says nothing about it — tearDown()'s isset($this->entityManager) guard makes the omission
invisible. Not worth extracting a trait now, because DAMA deletes the need for one.
Follow-up PR — the suite now has two isolation regimes
Fifteen flow tests are transactional; InvoiceEntryFlowTest, InvoiceFlowTest, InvoiceFullFlowTest, ProjectBillingFlowTest and ProjectBillingFullFlowTest still extend AbstractControllerTestCase,
write for real and clean up by hand. So "the database is untouched" is a per-class guarantee, not a per-suite one, and the transactional tests' reads can still pick up whatever a
non-transactional test left behind. Two regimes in one suite is harder to reason about than either applied consistently: when an ordering bug shows up, the first question becomes which regime
the offending test was under.
Follow-up PR — replace the hand-rolled transactions with dama/doctrine-test-bundle
DAMA does what this PR does by hand, through a StaticDriver that holds one connection across kernel boots, and it fixes the three paragraphs above at once: no disableReboot() is needed, because
the transaction survives a reboot, so requests get a fresh container and a fresh EntityManager and most of the manual clear() calls go away; it is a listener rather than a base class, so it
cannot be forgotten; the three copies of begin/rollback collapse into one config block; and enabling it for when@test gives the whole suite one isolation regime instead of two. I checked the
three things that usually make this migration painful and none of them applies here: tests/bootstrap.php runs drop, create, migrate and fixtures through passthru in separate processes (lines
14-46), so fixture data commits outside any static connection; there is no @Depends anywhere in tests/ and both InvoiceFullFlowTest:22 and ProjectBillingFullFlowTest:14 are a single test method,
so no existing test depends on data surviving between methods; and use_savepoints: true is already set. The one piece of real work is that AbstractTransactionalFlowTestCase stores the
EntityManager as a property and uses it after requests — with reboots re-enabled that property points into a container that has since been shut down, so it needs to become a getEntityManager()
accessor resolving from static::getContainer(), with the existing helpers routed through it. Everything else is deletion. Worth a version check against PHPUnit 10.2 and DBAL 3.10.5 first.
Deliberately not in this PR: a new dev dependency plus a global change to how every test isolates does not belong in a 7,700-line coverage PR, where it would make both halves harder to review
and harder to revert.
Optional — smaller things
AbstractFormTestCase::assertHasFields():131-135 compares array_keys($form->all()) in order, so reordering fields in a form type — a cosmetic change — fails with a confusing message; compare as
sets unless order is meaningful. ErrorControllerTest:57-62 asserts the controller returns 200 and the docblock reason ("so the template renders") isn't right: render() works fine with a non-200
status, and HttpKernel::handleThrowable() overwrites the 200 from the exception anyway, which is why the sibling tests correctly see 403/404 — the test pins an internal detail the other four
already cover. ProductFlowTest:128 and InvoiceExportFlowTest:38 pin "invalid CSRF → silent redirect, no error", which is stock maker-CRUD behaviour and fine, just now pinned. CHANGELOG.md:11-12
uses a lazy continuation line where every neighbouring entry uses a nested bullet.
Verify before merge — none of this has been run
I reviewed this statically and ran nothing, since tests/bootstrap.php drops and rebuilds the test database. Still unconfirmed: the 90.16% figure against the new 85 gate (task
test:coverage:check), PHPStan level 8 over the new files with no baseline additions (task code-analysis), the suite wall time — every one of the ~446 new tests boots a kernel in setUp(), and if
task test has grown past a few minutes that is the real cost of this PR and deserves a number in the thread — and order independence (vendor/bin/phpunit --testsuite integration
--order-by=random), which is what would expose the findOne() premise above and any interaction between the transactional and non-transactional tests.
What this gets right
Worth saying, because it isn't visible from the diffstat. LeantimeApiServiceDispatchTest pins exactly the invariants CLAUDE.md warns against breaking: one message per enabled Leantime provider,
sync vs async via TransportNamesStamp, which entity types get scoped to the provider's tracker ids and which deliberately do not, the LIMIT = 100 first page, and updateAll()'s type order.
WorklogRepositoryFilterTest seeds its own project so expectations are exact lists rather than counts, covering every filter branch including the onlyAvailable default and isBilled = false
catching NULL flags. The flow tests submit rendered forms, so CSRF tokens and field names stay honest, and they assert post-state after entityManager->clear() rather than trusting the identity
map — 422 on validation failure, redirect targets and the delete guards are all asserted rather than smoke-tested. tests/Unit stays kernel-free, so composer tests-unit and bootstrap_unit.php
still work. And the follow-up commits fixed the two things that usually rot first: the leaked temp file in ProductsImportCommandTest and a teardown that assumed setup had succeeded.
2 participants
Link to ticket
#8120
Description
Improved test coverage from 64.42% → 90.16%.
Changed test coverage gate threshold from 62% to 85% - left a bit of headroom.
Screenshot of the result
N/A
Checklist