test: take the suite from 187 failures to 25 - #16
Conversation
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Stacked on #13 (base is
fix/make-package-installable, so the diff shows only this work). Retarget tomainonce #13 merges.187 failed / 127 passed → 25 failed / 289 passed, across 315 tests.
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 — that one needs a portability decision (driver-aware SQL, a sortable stored version key, or ordering in PHP) rather than a patch, so it is left for a follow-up.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io