Add BridgeFake testing support and isAvailable() check#3
Conversation
Plugin authors currently cannot develop or test plugin behavior without a compiled mobile app, forcing every author to hand-write an anonymous class bridge stub. BridgeFake gives Http::fake()-style faking with stubbed responses and call assertions, and isAvailable() lets app code gracefully no-op when the bridge isn't bound (web/desktop). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe template adds ChangesBridge testing and availability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PluginFacade
participant BridgeFake
participant LaravelContainer
participant Plugin
PluginFacade->>BridgeFake: construct with responses
PluginFacade->>LaravelContainer: bind fake as nativephp.mobile.bridge
Plugin->>LaravelContainer: check bridge binding
LaravelContainer-->>Plugin: return availability
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
What problem this solvesPlugin talks to native code (Android/iOS) through "bridge." Bridge only exists inside compiled mobile app. On your laptop running composer test, no bridge — code crashes with RuntimeException. So every plugin author write same fake bridge by hand, every time, in every test. Real example — battery plugin Contract: Old way (before this PR) — hand-roll fake bridge in every test file: Works, but boilerplate. No way to assert what got called, no way to say "fail if code hits an unstubbed function." New way: One line to fake, one line to assert. Same pattern devs already know from Laravel's Http::fake(). App code that degrades gracefullySay a controller shows battery widget only on mobile: No isAvailable() before this PR — that check had to be a try/catch around RuntimeException, ugly. Steps to use the changed codeIn tests, fake bridge before calling plugin methods: Dynamic response — use closure instead of array, gets payload: Assert calls happened: No setup step needed beyond calling fake() — it swaps the container binding for you, real bridge stays untouched outside tests. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Plugin.php`:
- Around line 39-49: Replace method_exists($bridge, 'call') with
is_callable([$bridge, 'call']) in isAvailable(), and update the corresponding
guard in callBridge() to use the same callability predicate so availability
matches the actual invocation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39172b0c-57cb-47fe-94b0-1ea375ce0fa7
📒 Files selected for processing (9)
docs/testing.mdsrc/Contracts/{{ plugin }}Contract.phpsrc/Facades/{{ plugin }}.phpsrc/Plugin.phpsrc/Testing/BridgeFake.phpstubs/contract.stubstubs/facade.stubstubs/plugin.stubtests/BridgeFakeTest.php
method_exists() returns true for methods that exist but aren't invokable from the calling scope (e.g. private/protected). Both isAvailable() and callBridge() now use is_callable([$bridge, 'call']) so the availability check matches the actual invocation path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Problem
Plugin authors can't develop or test plugin behavior without a compiled mobile app.
Plugin::callBridge()resolvesnativephp.mobile.bridgefrom the container and throwsRuntimeExceptionwhen it's absent — so every author ends up hand-rolling the same anonymous-class bridge stub in their tests (see the oldtests/PluginTest.php).Before / after
Before, every plugin's test suite duplicated this:
After:
What's added
Testing\BridgeFake—Http::fake()-style canned responses (array orClosure) and call assertions:assertCalled(),assertNotCalled(),assertCalledTimes(),assertNothingCalled(), plusstub(),recorded(),preventStrayCalls().{{ plugin }}::fake(array $responses = [])— binds aBridgeFakeinto the container in place of the real bridge.isAvailable(): boolon the contract/Plugin/facade — lets app code check whether the bridge is bound before calling into it, so it can no-op gracefully on web/desktop.plugin.stub,contract.stub,facade.stub) anddocs/testing.mdupdated to match, per CONTRIBUTING.tests/BridgeFakeTest.phpcovering all of the above (stubbing, closures, stray-call prevention, every assertion's pass and fail path,isAvailable()).Design decision — flagging for maintainer input
Unstubbed calls return
[]by default; strict mode is opt-in viapreventStrayCalls(). This mirrorsHttp::fake()/Http::preventStrayRequests()semantics rather than throwing by default. Happy to flip the default if you'd rather fail loud out of the box.Scope
Left
tests/PluginTest.php's inline anonymous-class bridge untouched — refactoring it to useBridgeFakefelt like a separate, code-review-worthy change on its own, not bundled into this diff.Verification
Replicated CI exactly: configured the template (
php configure.php --vendor=acme --package=mobile-battery --plugin=Battery ...), then rancomposer install,composer test(17/17 pass), PHPStan level 8 (clean), Rector dry-run (no diff on any touched file), and confirmed placeholders survive in the unconfigured tree and resolve cleanly in the configured one. Full notes in the PR discussion if useful — one pre-existing note:composer lint's Pint step currently fails onmainitself (7 files) becausecomposer.lockisn't committed and a newerlaravel/pintresolves with stricter rules than the repo was authored against; my new file matches that same existing style drift rather than introducing a new failure class. Didn't fix it here since it's out of this PR's scope.Summary by CodeRabbit
isAvailable()checks to detect whether the mobile bridge is usable.isAvailable()semantics.callsupport.