Improve DX/UX, fix Sylius 2 admin icon, harden notify URL, expand tests#39
Merged
Conversation
- Menu: use the Sylius 2 Tabler icon `tabler:heart-handshake` (the old Semantic-UI `handshake outline` name does not render in Sylius 2 admin) - NotifyUrlProvider: URL-encode interpolated values (safer query string, prevents a value reintroducing a placeholder) and simplify with strtr - CookieHandler::get(): fail loudly via Assert when the cookie is absent instead of silently casting null to partner id 0 - Validation: fix `greather_than` -> `greater_than` (key + message) and tidy "Program id" -> "Program ID" - composer.json: drop now-unused ext-mbstring; add a `composer test` umbrella script - Document the order-total currency assumption on the calculator interface - Tests: cover NotifyHandler, ProgramContext, the exceptions, the Client error path, AdminMenuListener, NotifySubscriber no-program branches and the extension load/prepend behaviour
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.x #39 +/- ##
=============================================
+ Coverage 71.38% 86.84% +15.46%
Complexity 80 80
=============================================
Files 21 21
Lines 360 365 +5
=============================================
+ Hits 257 317 +60
+ Misses 103 48 -55 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The test application bootstrap built a non-canonical autoload path
(`dirname(__DIR__) . '../../../vendor/autoload.php'`). It resolved for
plain PHPUnit but, under Infection, loaded the Composer autoloader a
second time via a path `require` could not dedupe, making every mutant
die with a fatal "Cannot redeclare class". Infection counted those as
killed and reported a false 100% MSI, so the gate never actually ran.
Use a canonical `require_once dirname(__DIR__, 3) . '/vendor/autoload.php'`.
With Infection now running for real, close the pre-existing gaps it
exposes so the suite genuinely meets the configured 100% MSI:
- OrderTotalCalculator: format with sprintf('%.2f') instead of round(.., 2).
Order totals are integer minor units, so round(x, 2) vs round(x, 3) was
an equivalent (unkillable) mutant; the precision now lives in a format
string, keeping every remaining mutant killable.
- Configuration: make addHttpClientNode() private (internal helper) and
cover the cookie-expiry boundary and the non-empty http_client rule.
- RegisterHttpClientPass: assert the auto-registered Buzz client receives
the response factory argument.
Revert to round($orderTotal / 100, 2). Because order totals are integer minor units, round(x, 2) and round(x, 3) always agree, so the precision mutant is equivalent (unkillable). Mark the method with @infection-ignore-all rather than reshaping the code to satisfy the mutation tester; behaviour stays covered by OrderTotalCalculatorTest and the suite remains at a genuine 100% MSI.
…ments Revert OrderTotalCalculator::get() to a plain round($orderTotal / 100, 2) with no annotation. The equivalent precision mutant is now ignored in infection.json5 (IncrementInteger on OrderTotalCalculator::get) instead of via an in-source @infection-ignore-all. Also stabilise the now-real mutation gate: - Exclude the bundle class (SetonoSyliusPartnerAdsPlugin) from mutation: it is framework wiring exercised by the functional AdminRoutingTest, but its mutants are equivalent (already-lowercase getConfigFilesPath) or only killable through a cached kernel boot that is flaky under parallel runs. - Add deterministic getSubscribedEvents() unit tests for NotifySubscriber and SetCookieSubscriber so those mutants are killed without relying on the flaky functional kernel boot. Infection: 122/122 mutants killed, 100% MSI (verified with a clean cache).
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.
What
A focused round of DX/UX and best-practice fixes from an audit of the plugin, expanded test coverage, and a fix that restores the (previously broken) mutation-testing gate. Each item maps to a finding from the review.
Fixes
handshake outlineis a Sylius 1.x Semantic-UI icon name and does not render in the Sylius 2 admin (which uses Tabler icons). Switched totabler:heart-handshake. —src/Menu/AdminMenuListener.phpstr_replacewithout encoding. They are nowrawurlencoded, which also prevents a value from accidentally reintroducing another placeholder. Reimplemented withstrtrfor clarity. —src/UrlProvider/NotifyUrlProvider.phpCookieHandler::get()silently cast a missing cookie to partner id0. It now asserts the cookie is present (callers already guard withhas()). —src/CookieHandler/CookieHandler.phpgreather_than→greater_than(key + message), and "Program id" → "Program ID". —translations/validators.{en,da}.yaml,config/validation/Program.xmlext-mbstringrequirement (the onlymb_*call was replaced bystr_contains) and added acomposer testumbrella script. Documented the order-total currency assumption. —composer.json,src/Calculator/OrderTotalCalculatorInterface.phpFix: Infection was never actually running
tests/Application/config/bootstrap.phpbuilt a non-canonical autoload path (dirname(__DIR__) . '../../../vendor/autoload.php'). It resolved for plain PHPUnit, but under Infection it loaded the Composer autoloader a second time via a pathrequirecouldn't dedupe → every mutant died with a fatal "Cannot redeclare class". Infection counted those as "killed" and reported a false 100% MSI, so the gate never really ran.Fixed with a canonical
require_once dirname(__DIR__, 3) . '/vendor/autoload.php'. With Infection now running for real, this PR also closes/handles the pre-existing gaps it exposed so the suite genuinely meets the configured 100% MSI:getSubscribedEvents()on both subscribers (previously only killed via the flaky kernel-boot functional test).infection.json5: ignored the equivalentIncrementIntegermutant onOrderTotalCalculator::get()— order totals are integer minor units, soround(x, 2)andround(x, 3)always agree — and excluded the bundle classSetonoSyliusPartnerAdsPlugin(framework wiring, exercised by the functionalAdminRoutingTest, whose remaining mutants are equivalent or only killable through a cache-dependent kernel boot).Configuration:addHttpClientNode()madeprivate; added coverage for the cookie-expiry boundary and the non-emptyhttp_clientrule.RegisterHttpClientPass: assert the auto-registered Buzz client receives the response-factory argument.Tests (closing coverage gaps)
Added/extended unit tests for:
NotifyHandler,ProgramContext, the three exceptions, theClientnon-200 error path, the missing-cookie guard, URL-encoding,AdminMenuListener(both menu branches), both subscribers'getSubscribedEvents(),NotifySubscriberno-program / no-program-id branches, and the extension'sload()+prepend()behaviour.Verification
max, no errorsScope notes
Intentionally not included (per discussion): duplicate-conversion handling on thank-you-page refresh, payment-state gating, and affiliate query-parameter validation.