Make the unit suite runnable and gate it in CI - #73
Open
jules-paystack wants to merge 2 commits into
Open
Conversation
The 98 tests under Test/Unit had never executed anywhere: composer.json has an empty require block and no require-dev, there is no vendor/, and the only workflow was codeql-analysis.yml. That is why SetupTest had been failing since March 2026 unnoticed. magento/framework alone is not enough -- the tests mock Magento\Sales\... and Magento\Store\... classes. Six packages plus phpunit are needed, and they resolve cleanly from the Mage-OS mirror with no Adobe auth. The dependencies deliberately do NOT go in the shipped root composer.json: composer.lock is committed and already locks yabacon/paystack-php, a package not in require, so its content-hash is stale and adding require-dev would make composer install refuse until the lock is regenerated -- and the regenerated lock would ship in the Marketplace zip. "minimum-stability": "dev" with no prefer-stable would also resolve dev branches. 3.0.10 passed Adobe review with an empty require block, so that surface stays untouched. Instead the deps live in a CI-only Test/Unit/composer.json. - Fixed the 7 remaining errors: the observer tests stubbed getPaystackOrder() and getOrder(), which are not declared methods on Magento\Framework\Event\Observer or ...\Event -- they resolve via __call, and PHPUnit 10 refuses to configure undeclared methods. Construct real Observer/Event objects with data instead. - Strengthened testNullOrderDoesNotCrash and testNullPaymentDoesNotCrash to assert the observable consequence (no email sent / no email suppression applied) rather than only "did not throw". - phpunit.xml repointed at Test/Unit/vendor/autoload.php and now excludes Test/Unit/vendor from discovery, so CI and a local run share one config. magento/framework ships its own *Test.php files which fatal when loaded. - Workflow pinned to PHP 8.5 only, and the comment records why: 8.2 cannot install at all (magento/framework 103.0.9 needs ~8.3||~8.4||~8.5, which also contradicts the "PHP 8.2+" claim in README/CLAUDE.md), and 8.3/8.4 are untested because the lock is not committed so each runner resolves its own set. An untested matrix leg is a red check waiting to happen. - permissions: contents: read, and pull_request rather than pull_request_target, so fork code never gets a writable token or secrets. - build-adobe-zip.sh excludes phpunit.xml, composer.lock and Test/Unit/*. Test/Mftf/ still ships (verified: 10 entries present, 0 test-infra entries). Verified with the exact CI invocation: OK (98 tests, 137 assertions), exit 0. Gate 2 (adversarial diff review) has NOT run for this item -- both critics died on an API session limit. No production code is touched here, but the gate is outstanding and recorded as such in the plan file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adversarial diff review of 165e6a1 (security + architecture critics, 24 findings). Actioned every high-severity item. Packaging leak (high, both critics, confirmed by replaying the exact zip invocation): phpunit.xml's cacheDirectory=".phpunit.cache" created a new repo-root directory whose -x line was not added in the same commit, so .phpunit.cache/ and its test-results shipped into the Marketplace package -- the precise rule CLAUDE.md records after the dev-ee/ incident, broken by the commit that cited it. Fixed by relocating the cache to Test/Unit/.phpunit.cache, which the existing Test/Unit/* exclusion already covers; no fourth -x needed. Non-reproducible gate (high, both critics): Test/Unit/composer.lock was gitignored while every magento/module-* was constrained "*", so the check meant to guard the money path re-resolved 184 floating packages every run. The reasons for not committing the *root* lock do not transfer -- Test/Unit/* is excluded from the zip, so this lock ships nothing and constrains no merchant. The commit was also internally inconsistent: it refused 8.3/8.4 legs *because* unpinned resolution makes them untested, then left 8.5 unpinned over time. Lock now committed, all 184 packages pinned, every constraint exact, cache rekeyed on the lock. dev/ coupling (high): the whole repo is bind-mounted to app/code/Pstk/Paystack, so `cd Test/Unit && composer install` plants a second full magento/framework inside a Magento module path that setup:di:compile scans. Masked with an anonymous volume. dev-repro/ needs the same line but is untracked, so that is noted in CLAUDE.md. Shipped-artifact change reverted: -x "composer.lock" dropped the root lock from the package. It is inert, but 3.0.10 shipped it and passed Adobe review, and changing the artifact's composition in a test-infra commit without validating against Adobe's package checks is not a trade worth making. Removing the stale root lock outright is filed separately. Also dropped the redundant -x "Test/Unit/composer.json" (Test/Unit/* already matches). Test honesty: the two "strengthened" negative tests were cosmetic. Remove the guard they nominally cover and production throws before the never() can be evaluated, so the test fails on the Error, not the assertion. Kept the never() as a smoke check but documented what it does and does not prove, and pointed at the case that actually distinguishes. Also unified the mixed idiom (the outer Observer was still a mock while the inner Event was real) and flagged that testNonPendingOrderIsNotUpdated encodes the status-string gate the verification work will change, so that edit reads as planned rather than as bending a green test. Supply chain: --no-plugins/--no-scripts (verified the suite passes without them), config.allow-plugins removed entirely, actions pinned to commit SHAs, composer pinned to 2.9, persist-credentials: false, concurrency + timeout-minutes, and the cache now stores Composer's content-addressed download cache rather than vendor/, which was restored-and-executed trusted code under an immutable key. CI scope: pull_request no longer filters on base branch -- the money-path work lands as a stack of PRs onto feature branches, which the filter skipped entirely. Docs: CLAUDE.md said "there are no unit tests" and "the only CI is codeql-analysis.yml", listed a stale exclusion list and the wrong MFTF page-object name, and repeated an unverified "PHP 8.2+" claim -- magento/framework 103.0.9 requires ~8.3||~8.4||~8.5. All corrected, with the install prerequisite documented. Also recorded that CodeQL scans JavaScript only, so the PHP money path currently gets no static analysis. Verified from a clean state via the documented commands: OK (98 tests, 137 assertions), exit 0. Zip composition re-checked: 0 phpunit-cache, 0 Test/Unit, 0 phpunit.xml, 0 auth.json entries; composer.lock present; 10 Test/Mftf entries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Infrastructure only — no production code changes. This unblocks the verification-gate work: until now there was no way to run a test for it.
The problem
The 98 tests in
Test/Unit/had never executed anywhere.composer.jsonhas an emptyrequireblock and norequire-dev, there's novendor/, and the only workflow was CodeQL. That's whySetupTesthad been failing since March 2026 without anyone knowing — and why thegetCurrency()bug fixed in #72 survived years of review: the test that would have caught it stubbed a method that doesn't exist, and the stub only errors when the suite actually runs.What this does
Verified from a clean checkout using the exact commands now documented in CLAUDE.md.
Test/Unit/composer.json+ committed lock — 184 packages pinned to exact versions, resolved from the Mage-OS mirror with no Adobe auth. The shipped rootcomposer.jsonis untouched: itscomposer.lockalready has a stale content-hash (it locksyabacon/paystack-php, absent fromrequire), so addingrequire-devthere would breakcomposer installand the regenerated lock would ship to Marketplace. 3.0.10 passed Adobe review with an emptyrequire; that surface stays as-is.getPaystackOrder()/getOrder(), which aren't declared methods onMagento\Framework\Event\Observer— they resolve via__call, and PHPUnit 10 refuses to configure undeclared methods. RealObserver/Eventobjects instead.phpunit.xmlis now the single config shared by CI and local runs, bootstrappingTest/Unit/vendor/autoload.phpand excludingTest/Unit/vendorfrom discovery (magento/frameworkships its own*Test.phpfiles that fatal when loaded).What adversarial review caught
Two findings I'd have shipped otherwise:
phpunit.xml's cache was shipping into the Marketplace zip.cacheDirectory=".phpunit.cache"created a new repo-root directory without adding its exclusion — the exact rule CLAUDE.md records after thedev-ee/incident, broken by the commit citing it. Confirmed by replaying the zip command. Cache relocated underTest/Unit/.magento/module-*was"*", so 184 packages floated on every run. My own workflow comment refused extra PHP legs because unpinned resolution makes them untested, while leaving 8.5 unpinned over time — same file, opposite conclusions. Lock now committed.Also: masked
Test/Unit/vendorout ofdev/'s bind mount (the repo mounts as a Magento module, so a secondmagento/frameworkinside it would be scanned bysetup:di:compile); reverted an unvalidated change to the shipped artifact; SHA-pinned the actions;--no-plugins --no-scripts; dropped thepull_requestbase-branch filter so stacked PRs are gated; and documented honestly that two negative tests only prove "does not throw".Two things worth your attention, not fixed here
actions/checkout@v2andgithub/codeql-action/*@v1(CodeQL v1 is EOL, so the scan may be silently degraded).paystack_payment_verify_afteris registered only inetc/frontend/events.xml, while the inline verify endpoint runs inwebapi_rest. If the REST leg's observer never fires, guarantees placed behind that event would hold only for the registered area — and the unit suite can't detect it, because the tests mock the dispatcher on one side and the observer on the other. Needs an empirical check indev/before the verification gate is built on it.Suggested merge note
The check is intentionally non-required for now. Flip it to required once it's been green on a few PRs — a red required check on protected
masterblocks everything.🤖 Generated with Claude Code