test: take the suite from 187 failures to 25 - #17
Merged
Conversation
dappcore/mcp could not be installed or tested by anyone. composer.json
declared only php and dappcore/php while src/ imports Laravel\Mcp in 14
files, Livewire in 46 and Core\Tenant in 19 — none of them declared. There
was no phpunit config, no test script, no vendor tree, and no /vendor rule
in .gitignore, because composer had never been run here at all. Its eleven
test files have therefore never executed.
Declared what the code actually uses: laravel/mcp as a hard requirement (the
tools extend Laravel\Mcp\Server\Tool), and dappcore/php-tenant, livewire,
mockery, testbench, pest and pint as dev requirements, matching how
dappcore/agent already carries its sibling modules.
The harness had the same three faults core/agent had. Pest.php bound its
TestCase to bare 'Feature' and 'Unit', which Pest resolves against its
default ./tests while this suite lives at php/tests, so nothing matched.
php/tests/TestCase.php declared namespace Tests with no PSR-4 entry for it,
so `use Tests\TestCase` could not resolve. And it extended
Illuminate\Foundation\Testing\TestCase, which expects a host application to
have booted — a package suite needs Testbench, and the package's own
provider registering, since Testbench does not run package auto-discovery.
Then the suite immediately proved why the imports matter: it died at
collection on Trait "Mod\Mcp\Tools\Concerns\RequiresWorkspaceContext" not
found. Fifteen dangling imports are fixed here, each verified against the
symbol it resolves to rather than assumed:
Mod\Mcp\* -> Core\Mcp\* (the package's own)
Mod\Tenant\... -> Core\Tenant\... (dappcore/php-tenant)
Mod\Uptelligence\ -> Core\Mod\Uptelligence\ (dappcore/php-uptelligence)
A bare Mod\ root is mapped nowhere in any dappcore package — only in the
host application — so every one of these was unreachable. That includes
CircuitBreaker importing Mod\Mcp\Exceptions\CircuitOpenException while its
own exception sits at Core\Mcp\Exceptions\CircuitOpenException, which made
call() fatal the moment a circuit opened.
Five imports are deliberately left alone rather than guessed at:
Mod\Agentic\Models\{AgentPlan,AgentSession} and
Mod\Mcp\Tools\Agent\Contracts\AgentToolInterface all resolve into
dappcore/agent, which would make this package depend on a package that is
meant to depend on it; Mod\Mcp\Services\McpMonitoringService exists only in
an RFC and was never implemented; Mod\Api\Models\ApiKey has no counterpart
on disk. Each needs a decision, not a rewrite.
Result: the suite runs for the first time — 187 failed, 127 passed, 270
assertions. Not green, and not claimed to be: those 187 are the honest
backlog this package has been carrying unseen, and can now be worked with a
runner that reports them.
Co-Authored-By: Virgil <virgil@lethean.io>
187 of 315 tests failed. 180 of those failures were a missing database and
nothing else: 113 on workspaces, 27 on mcp_tool_metrics, 24 on
mcp_tool_versions, 15 on users, 1 on mcp_tool_combinations. Boot.php
publishes the package's migrations for a host application to run rather than
loading them itself, and php-tenant does the same, so under Testbench there
was no schema at all. Naming both directories in defineDatabaseMigrations
takes the suite from 187 failed / 127 passed to 62 / 253 on its own.
The next cluster was reaching the code at all. WorkspaceContextSecurityTest
builds a stand-in class over the RequiresWorkspaceContext trait, whose
accessors are protected — correct for a real tool, and unreachable from a
test, so nineteen assertions died on "Call to protected method ... from
scope" without ever exercising the behaviour. The stand-in now aliases them
to public via `use RequiresWorkspaceContext { getWorkspaceId as public; ... }`,
which exposes them for assertion without widening the trait itself.
Last, four class-style tests under src/Mcp/Tests were migrating and rolling
back for real and failing the rollback on "no such table: migrations". Pest's
uses(RefreshDatabase::class)->in() only reaches Pest-style files, so those
four never picked the trait up while every other test in the suite ran inside
a transaction. They now declare it themselves.
315 tests: 29 failed, 285 passed, 1 risky — from 187 failed, 127 passed. The
29 that remain are a long tail of eighteen distinct causes, each needing its
own diagnosis rather than a shared fix.
Co-Authored-By: Virgil <virgil@lethean.io>
…r counter
Three real defects, all of which only became visible once the suite could run.
Laravel\Mcp\Request has no input(). Its accessor is get(), and nineteen calls
across five tools — QueryDatabase, DescribeTable, CreateCoupon, UpgradePlan
and ListInvoices — used input() and would have thrown BadMethodCallException
on first use. Every one of those tools was broken. The two middleware that
also call input() are left alone: they take Illuminate\Http\Request, where
input() is correct, so this is not a blanket rename but a per-file check of
which Request each one imports. None of the nineteen used dot-notation keys,
so get() is an exact swap.
Laravel\Mcp\Response has no getContent() either — that is the Illuminate
response API. It exposes content(), returning a Content that implements
__toString(). DescribeTableTest asserted through the wrong one.
ToolAnalyticsService::flushToolCombinations() could never record a pair for
the first time. It called updateOrInsert() with
DB::raw('occurrence_count + 1') as the value, which is right on the update
path and invalid on the insert path, where it becomes `insert into ...
(occurrence_count) values (occurrence_count + 1)` — a column reference inside
a VALUES clause. The insert threw, so the follow-up query written to repair
exactly that case never ran. Replaced with an increment that reports how many
rows it touched and an insert only when that is zero. The lookup also now uses
whereNull for a null workspace: `= null` is never true in SQL, so a global
pair matched nothing and would have been re-inserted on every flush.
315 tests: 25 failed, 289 passed, 1 risky — from 187 failed, 127 passed when
the suite first ran. What remains is a long tail of individual causes, the
clearest being McpToolVersion's orderByVersion scope, which sorts with
MySQL-only SUBSTRING_INDEX and so cannot execute on sqlite at all.
Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Replaces #16, which GitHub auto-closed when #13's branch was deleted on merge and refused to reopen. Same work, now based on
main. Receipts already reviewed.187 failed / 127 passed → 25 failed / 289 passed, across 315 tests. Re-verified after merging
mainin: identical numbers.Getting the suite to reach the code
defineDatabaseMigrationsRefreshDatabaseon the four class-style testsMigrations. 180 failures were a missing database and nothing else — 113 on
workspaces, 27 onmcp_tool_metrics, 24 onmcp_tool_versions, 15 onusers, 1 onmcp_tool_combinations.Boot.phppublishes the package's migrations for a host app to run rather than loading them, and php-tenant does the same, so under Testbench there was no schema at all.Protected accessors.
WorkspaceContextSecurityTestbuilds a stand-in over theRequiresWorkspaceContexttrait, whose accessors areprotected— correct for a real tool, unreachable from a test. Nineteen assertions died on "Call to protected method … from scope" without ever exercising the behaviour. The stand-in now aliases them viause RequiresWorkspaceContext { getWorkspaceId as public; … }, which exposes them for assertion without widening the trait itself.RefreshDatabase. Pest's
uses(RefreshDatabase::class)->in()only reaches Pest-style files, so four class-style tests undersrc/Mcp/Testsnever picked it up, migrated and rolled back for real, and failed the rollback on "no such table: migrations".Three production defects the running suite exposed
Laravel\Mcp\Requesthas noinput(). Its accessor isget(). Nineteen calls across five tools —QueryDatabase,DescribeTable,CreateCoupon,UpgradePlan,ListInvoices— would have thrownBadMethodCallExceptionon first use. Every one of those tools was broken. The two middleware that also callinput()are deliberately untouched: they takeIlluminate\Http\Request, where it's correct. This was a per-file check of whichRequesteach imports, not a blanket rename. None used dot-notation keys, soget()is an exact swap.Laravel\Mcp\Responsehas nogetContent()— that's the Illuminate API. It exposescontent(), returning aContentthat implements__toString().ToolAnalyticsService::flushToolCombinations()could never record a pair for the first time. It calledupdateOrInsert()withDB::raw('occurrence_count + 1')as the value — right on the update path, invalid on the insert path where it becomesinsert into … (occurrence_count) values (occurrence_count + 1), a column reference inside aVALUESclause. The insert threw, so the follow-up query written to repair exactly that case never ran. Also now useswhereNullfor a null workspace:= nullis never true in SQL, so a global pair matched nothing and would be re-inserted on every flush.What remains
25 failures, a long tail of individual causes rather than a shared one. The clearest is
McpToolVersion::orderByVersion, which sorts using MySQL-onlySUBSTRING_INDEXand therefore cannot execute on sqlite at all — being fixed separately with a sortable stored version key.Note on history
Contains a merge commit rather than a rebase: the branch was already pushed, and #13 landed as a squash, so this branch's own copy of that work carries a different SHA. One conflict, in
php/tests/TestCase.php, where this branch addsdefineDatabaseMigrationsandmaindoes not — resolved in favour of this branch, then the full suite re-run to confirm the same 25/289.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io