From 0793e2a75f6293f725e91b1b303bc858c9dc5c60 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:19:03 +0200 Subject: [PATCH 1/2] Give every assertion something it can fail on --- tests/README.md | 30 +++++++++-- tests/_support/Spy_Writer.php | 24 +++------ tests/_support/Traits/WithIncorrectUsage.php | 35 +++++++++++-- tests/unit/Conflict/RedirectorTest.php | 52 ++++++++++++++++++++ tests/unit/Notices/StoreTest.php | 25 +++++++++- tests/unit/Notices/WriterTest.php | 25 +++++++++- tests/unit/ProviderTest.php | 36 +++++++++++--- 7 files changed, 194 insertions(+), 33 deletions(-) diff --git a/tests/README.md b/tests/README.md index 6c0dff5..e977b5d 100644 --- a/tests/README.md +++ b/tests/README.md @@ -267,6 +267,26 @@ The trait needs `UopzFunctions` on the same class, for the stub. a stub really can stop a code path before it reaches `exit`, and that the shared helper reports it when one does not. +## An absence needs a control + +An assertion that something is *not* there passes just as happily when the place it looked was +empty, and it goes on passing after the behaviour it was written for stops running. So the same +read has to be shown holding something first: + +```php +add_option( self::AUTOLOADED_CONTROL, 'Carried on every request.', '', 'yes' ); + +$autoloaded = array_keys( wp_load_alloptions() ); + +$this->assertContains( self::AUTOLOADED_CONTROL, $autoloaded ); +$this->assertNotContains( self::OPTION, $autoloaded ); +``` + +That is the same rule as the recorder one — record calls, assert the list is empty, then invoke the +stub once and assert it caught it — and it is why `Spy_Writer` records only the notice branches +something reads back. A recorder no test reads is an invitation to assert it is empty and be told +nothing, so a counter lands in the same change as the assertion that reads it. + ## Expecting `_doing_it_wrong()` `setExpectedIncorrectUsage()` matches the first argument exactly, which for a @@ -286,9 +306,13 @@ $loader->load_all(); $this->assert_the_library_reported_incorrect_usage(); ``` -An unexpected report still fails the test, because everything the listener sees -is recorded and asserted to belong to this library. Call -`stop_expecting_incorrect_usage()` from tearDown. +An unexpected report still fails the test. `expect_incorrect_usage()` declares +that *this library* may report a developer mistake and nothing else, so that is +all the listener registers with WPTestCase: a report named for anything else — +WordPress's own, another plugin's, a translation loaded before `init` — stays +unexpected and fails the test as it always did. Registering whatever arrived +would have given that protection away on the first report a test did expect, +which is most of them. Call `stop_expecting_incorrect_usage()` from tearDown. ## The scenario suite diff --git a/tests/_support/Spy_Writer.php b/tests/_support/Spy_Writer.php index 85a06fc..94dea6a 100644 --- a/tests/_support/Spy_Writer.php +++ b/tests/_support/Spy_Writer.php @@ -40,13 +40,6 @@ class Spy_Writer implements Writer_Interface { */ public $merge_notices = []; - /** - * Slugs handed to queue_conflict_notice(), in order. - * - * @var string[] - */ - public $conflict_notices = []; - /** * Slugs handed to queue_dependency_notice(), in order. * @@ -54,13 +47,6 @@ class Spy_Writer implements Writer_Interface { */ public $dependency_notices = []; - /** - * Slugs handed to queue_stranding_notice(), in order. - * - * @var string[] - */ - public $stranding_notices = []; - /** * @param Sub_Plugin $sub_plugin Sub-plugin concerned. * @@ -71,12 +57,16 @@ public function queue_merge_notice( Sub_Plugin $sub_plugin ): void { } /** + * Recorded nowhere, because nothing reads it: the conflict branch is asserted against the real + * queue's own `slug:conflict` key, in `Conflict\ResolverTest`. A recorder with no reader is a + * standing invitation to assert it is empty and be told nothing at all — so if a test here ever + * needs this call counted, the counter lands in the same change as the assertion that reads it. + * * @param Sub_Plugin $sub_plugin Sub-plugin concerned. * * @return void */ public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void { - $this->conflict_notices[] = $sub_plugin->get_slug(); } /** @@ -89,12 +79,14 @@ public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { } /** + * Recorded nowhere, for the reason given on queue_conflict_notice(): the stranding branch is + * asserted against the real queue's own `slug:stranding` key. + * * @param Sub_Plugin $sub_plugin Sub-plugin concerned. * * @return void */ public function queue_stranding_notice( Sub_Plugin $sub_plugin ): void { - $this->stranding_notices[] = $sub_plugin->get_slug(); } /** diff --git a/tests/_support/Traits/WithIncorrectUsage.php b/tests/_support/Traits/WithIncorrectUsage.php index 3bdaa58..b21ab99 100644 --- a/tests/_support/Traits/WithIncorrectUsage.php +++ b/tests/_support/Traits/WithIncorrectUsage.php @@ -19,8 +19,11 @@ * observe — that the mistake is reported at all, and reported against this library rather than * swallowed or blamed on WordPress — is what these assertions pin. * - * The expectation is registered from the report itself, so an unexpected report still fails the test: - * anything the library reports is recorded here and asserted over. + * The expectation is registered from the report itself, but only for a report this library made. + * `expect_incorrect_usage()` declares that this library may report a developer mistake and nothing + * more, so that is the only thing registered with WPTestCase — a report from anywhere else is left + * unexpected and still fails the test. Registering whatever arrived would have handed WordPress's own + * protection away on the first report a test did expect, which is most of them. * * The *message* is recorded alongside the name, because "something was reported" is a weak thing to * assert on its own: a failed registry read, a missing hook prefix and the gate a test is actually @@ -73,8 +76,9 @@ protected function expect_incorrect_usage(): void { $reports = &$this->incorrect_usage_reports; $messages = &$this->incorrect_usage_messages; + $declared = self::declared_report_prefix(); - $listener = function ( $function_name, $message = '' ) use ( &$reports, &$messages ): void { + $listener = function ( $function_name, $message = '' ) use ( &$reports, &$messages, $declared ): void { if ( ! is_string( $function_name ) ) { return; } @@ -82,6 +86,14 @@ protected function expect_incorrect_usage(): void { $reports[] = $function_name; $messages[] = is_string( $message ) ? $message : ''; + // Only what this library reports is what a caller of this trait declared it expects, so + // only that is registered with WPTestCase. A report from anywhere else — WordPress's + // own, another plugin's, a translation loaded before `init` — stays unexpected and + // still fails the test, which is the protection a blanket registration gave away. + if ( strpos( $function_name, $declared ) !== 0 ) { + return; + } + $this->setExpectedIncorrectUsage( $function_name ); }; @@ -104,7 +116,7 @@ protected function assert_the_library_reported_incorrect_usage(): void { foreach ( $this->incorrect_usage_reports as $report ) { $this->assertStringStartsWith( - 'Nexcess\\PluginAbsorber\\', + self::declared_report_prefix(), $report, 'The report has to name this library, or the host goes looking in WordPress.' ); @@ -153,6 +165,21 @@ protected function stop_expecting_incorrect_usage(): void { $this->incorrect_usage_messages = []; } + /** + * What a report has to be named for this trait to count it as the one a caller expects. + * + * Every report this library makes carries `__METHOD__`, so its namespace is the whole of the + * declaration `expect_incorrect_usage()` stands for. A method rather than a constant: PHP 7.4 + * has no trait constants. + * + * @since 1.0.0 + * + * @return string + */ + private static function declared_report_prefix(): string { + return 'Nexcess\\PluginAbsorber\\'; + } + /** * Unhook whatever this trait last installed, if anything. * diff --git a/tests/unit/Conflict/RedirectorTest.php b/tests/unit/Conflict/RedirectorTest.php index 68b6f65..3456dea 100644 --- a/tests/unit/Conflict/RedirectorTest.php +++ b/tests/unit/Conflict/RedirectorTest.php @@ -84,6 +84,15 @@ public static function request_uris(): Generator { yield 'a query value carrying a line break' => [ '/wp-admin/edit.php?s=a%0D%0Ab', admin_url( 'edit.php?s=ab' ) ]; yield 'a query value carrying a null byte' => [ '/wp-admin/edit.php?s=a%00b', admin_url( 'edit.php?s=ab' ) ]; + // A query value is not always a scalar: `s[]=` is the shape a list of checked boxes comes + // back in, and wp_parse_str() hands it over as a nested array. The strip has to walk into + // one, because a pass that only looked at strings would step over the array whole and put + // the line break in the Location header inside a parameter of its own. + yield 'a line break inside an array query value' => [ + '/wp-admin/edit.php?s[]=a%0D%0Ab&s[]=c', + admin_url( 'edit.php?s%5B0%5D=ab&s%5B1%5D=c' ), + ]; + // An admin root names the dashboard by leaving it out, exactly as core's own /wp-admin/ link // does. Sending an admin who asked for the dashboard to the plugins list instead is the // failure this whole class is about, in its most common form. @@ -108,6 +117,49 @@ public static function request_uris(): Generator { yield 'a uri naming another host' => [ '//evil.test/wp-admin/plugins.php', admin_url( 'plugins.php' ) ]; } + /** + * The screen-name pattern is anchored with `\z` and not with `$`, and the whole of the + * difference is a trailing newline: in PCRE `$` also matches immediately before one, so + * "edit.php\n" would satisfy it and a line break would leave this class inside the one value it + * promises is validated — on its way into a Location header. + * + * No case in the provider above can pin that, and not for want of trying: PHP's own parse_url() + * rewrites every non-printable byte of a path to an underscore, so a request URI carrying a real + * line break reaches the pattern as "edit.php_" and is refused for a different reason entirely. + * That rewrite is an implementation detail of the parser rather than a promise this class is + * entitled to lean on, and the anchor is what holds if it ever changes — so the parse is stubbed + * to hand the path over verbatim, which is the only way to put the question to the pattern. + * + * The clean path is asserted first, under the same stub. Without it the fallback below would be + * satisfied just as well by a stub that broke every destination, which is the wrong reason to + * pass. + */ + public function test_it_refuses_a_screen_name_with_a_line_break_after_it(): void { + $this->setFunctionReturn( 'is_network_admin', false ); + $this->setFunctionReturn( 'is_user_admin', false ); + + $this->setFunctionReturn( + 'wp_parse_url', + static function ( $url, $component = -1 ) { + return $component === PHP_URL_QUERY ? null : $url; + }, + true + ); + + $redirector = new Redirector(); + + $this->assertSame( + admin_url( 'edit.php' ), + $redirector->after_deactivation( '/wp-admin/edit.php' ), + 'A path naming a screen still resolves to it, so the parse is not what refuses below.' + ); + + $this->assertSame( + admin_url( 'plugins.php' ), + $redirector->after_deactivation( "/wp-admin/edit.php\n" ) + ); + } + /** * $_SERVER carries whatever the SAPI put there, and a host may filter it besides, so the guard * is a runtime one rather than a promise the signature can keep. diff --git a/tests/unit/Notices/StoreTest.php b/tests/unit/Notices/StoreTest.php index ec5cc57..d493d8b 100644 --- a/tests/unit/Notices/StoreTest.php +++ b/tests/unit/Notices/StoreTest.php @@ -33,6 +33,12 @@ class StoreTest extends WPTestCase { private const OPTION_NORMALISED = 'give_core_plugin_absorber_notices'; + /** + * An option deliberately written with autoload on, so that "the queue is not in the bundle" is + * read off a bundle that demonstrably holds something. + */ + private const AUTOLOADED_CONTROL = 'plugin_absorber_autoload_control'; + /** * The second site the network-scope test reads the queue from, once it has one. * @@ -53,6 +59,7 @@ public function tearDown(): void { delete_site_option( self::OPTION ); delete_site_option( self::OPTION_WOO ); delete_site_option( self::OPTION_NORMALISED ); + delete_option( self::AUTOLOADED_CONTROL ); Config_State::reset(); parent::tearDown(); } @@ -186,6 +193,11 @@ public function test_it_needs_a_hook_prefix(): void { /** * The queue is empty on nearly every request and only ever read in the admin, so it must not * ride along in the autoloaded bundle on every front-end request. + * + * The control option is what makes the absence mean anything. An `assertNotContains()` over an + * empty bundle passes without the store having done a thing, and goes on passing for ever — so + * an option written with autoload on is looked for first, out of a bundle re-read from the + * database rather than off the cache entry the write above just touched. */ public function test_the_queue_is_not_autoloaded(): void { if ( is_multisite() ) { @@ -194,7 +206,18 @@ public function test_the_queue_is_not_autoloaded(): void { ( new Store() )->put( 'give-recurring:merge', 'Bundled now.' ); - $this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) ); + add_option( self::AUTOLOADED_CONTROL, 'Carried on every request.', '', 'yes' ); + + wp_cache_delete( 'alloptions', 'options' ); + + $autoloaded = array_keys( wp_load_alloptions() ); + + $this->assertContains( + self::AUTOLOADED_CONTROL, + $autoloaded, + 'The bundle has to carry an autoloaded option, or the queue not being in it says nothing.' + ); + $this->assertNotContains( self::OPTION, $autoloaded ); } /** diff --git a/tests/unit/Notices/WriterTest.php b/tests/unit/Notices/WriterTest.php index f7e99d3..2f4cc7e 100644 --- a/tests/unit/Notices/WriterTest.php +++ b/tests/unit/Notices/WriterTest.php @@ -35,6 +35,12 @@ class WriterTest extends WPTestCase { // derived from the prefix rather than fixed. private const OPTION_FOR_OTHER_PREFIX = 'woo_plugin_absorber_notices'; + /** + * An option deliberately written with autoload on, so that "the queue is not in the bundle" is + * read off a bundle that demonstrably holds something. + */ + private const AUTOLOADED_CONTROL = 'plugin_absorber_writer_autoload_control'; + public function setUp(): void { parent::setUp(); @@ -46,6 +52,7 @@ public function setUp(): void { public function tearDown(): void { $this->clear_queue(); delete_site_option( self::OPTION_FOR_OTHER_PREFIX ); + delete_option( self::AUTOLOADED_CONTROL ); Config_State::reset(); parent::tearDown(); } @@ -361,6 +368,11 @@ public function test_queueing_needs_a_hook_prefix(): void { /** * The queue is empty on nearly every request and only ever read in the admin, so it must not * ride along in the autoloaded bundle on every front-end request. + * + * The control option is what makes the absence mean anything. An `assertNotContains()` over an + * empty bundle passes without the writer having done a thing, and goes on passing for ever — so + * an option written with autoload on is looked for first, out of a bundle re-read from the + * database rather than off the cache entry the write above just touched. */ public function test_the_queue_is_not_autoloaded(): void { if ( is_multisite() ) { @@ -369,7 +381,18 @@ public function test_the_queue_is_not_autoloaded(): void { $this->make_writer()->queue_merge_notice( $this->make_sub_plugin() ); - $this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) ); + add_option( self::AUTOLOADED_CONTROL, 'Carried on every request.', '', 'yes' ); + + wp_cache_delete( 'alloptions', 'options' ); + + $autoloaded = array_keys( wp_load_alloptions() ); + + $this->assertContains( + self::AUTOLOADED_CONTROL, + $autoloaded, + 'The bundle has to carry an autoloaded option, or the queue not being in it says nothing.' + ); + $this->assertNotContains( self::OPTION, $autoloaded ); } /** diff --git a/tests/unit/ProviderTest.php b/tests/unit/ProviderTest.php index 506055b..47063d3 100644 --- a/tests/unit/ProviderTest.php +++ b/tests/unit/ProviderTest.php @@ -16,6 +16,7 @@ use Nexcess\PluginAbsorber\Conflict\Gatekeeper; use Nexcess\PluginAbsorber\Conflict\Redirector; use Nexcess\PluginAbsorber\Conflict\Resolver; +use Nexcess\PluginAbsorber\Conflict\Rewriter; use Nexcess\PluginAbsorber\Contracts\Activator_Interface; use Nexcess\PluginAbsorber\Contracts\Provider_Interface; use Nexcess\PluginAbsorber\Loader; @@ -84,6 +85,11 @@ public function test_it_binds_every_default( string $id, string $expected ): voi * @return Generator */ public static function default_bindings(): Generator { + // The container's own contract is bound first and by the provider itself, so that a + // container which builds unbound classes reflectively can still satisfy a collaborator that + // takes one. + yield 'the container itself' => [ ContainerInterface::class, Test_Container::class ]; + yield 'the registrar' => [ Registrar_Interface::class, Registrar::class ]; yield 'the registry reader' => [ Reader::class, Reader::class ]; yield 'the notice writer' => [ Writer_Interface::class, Writer::class ]; @@ -97,10 +103,24 @@ public static function default_bindings(): Generator { yield 'the conflict detector' => [ Detector::class, Detector::class ]; yield 'the redirector' => [ Redirector::class, Redirector::class ]; yield 'the conflict gate' => [ Gatekeeper::class, Gatekeeper::class ]; + yield 'the error rewriter' => [ Rewriter::class, Rewriter::class ]; yield 'the load runner' => [ Loader::class, Loader::class ]; yield 'the boot scheduler' => [ Scheduler::class, Scheduler::class ]; } + /** + * `instanceof` is not enough for this one id. Every other binding is answered by a class the + * library names, so building the wrong object means building the wrong class — but a second + * container of the same class is exactly what a self-binding gets wrong, and it satisfies the + * type. What the collaborators taking one need is *this* container: another instance holds none + * of the bindings the provider just made, and every id resolved through it fails. + */ + public function test_it_binds_the_container_under_its_own_contract(): void { + $container = $this->registered_container(); + + $this->assertSame( $container, $container->get( ContainerInterface::class ) ); + } + /** * The registrar holds the registrations and the queue holds its store, so a binding rebuilt per * call would hand the load loop a registry the flush never reached. The rest are stateless, and @@ -131,6 +151,7 @@ public static function single_instance_bindings(): Generator { yield 'the conflict detector' => [ Detector::class ]; yield 'the redirector' => [ Redirector::class ]; yield 'the conflict gate' => [ Gatekeeper::class ]; + yield 'the error rewriter' => [ Rewriter::class ]; yield 'the load runner' => [ Loader::class ]; yield 'the boot scheduler' => [ Scheduler::class ]; } @@ -168,6 +189,7 @@ public static function class_id_bindings(): Generator { yield 'the conflict detector' => [ Detector::class ]; yield 'the redirector' => [ Redirector::class ]; yield 'the conflict gate' => [ Gatekeeper::class ]; + yield 'the error rewriter' => [ Rewriter::class ]; yield 'the load runner' => [ Loader::class ]; yield 'the boot scheduler' => [ Scheduler::class ]; } @@ -229,19 +251,17 @@ public function test_registering_twice_keeps_the_first_instances(): void { } /** - * A container with the defaults registered into it, plus the self-binding a host container - * ordinarily offers. + * A container with the defaults registered into it, and nothing else. + * + * It used to arrive carrying the self-binding a host container ordinarily offers, which meant + * the one binding the provider makes first was the one binding nothing here ever exercised: + * `bind_once()` sees an id already answered and stands down, so a provider that had dropped its + * own would have resolved out of the helper and passed. * * @return Test_Container */ private function registered_container(): Test_Container { $container = new Test_Container(); - $container->singleton( - ContainerInterface::class, - static function () use ( $container ): ContainerInterface { - return $container; - } - ); ( new Provider( $container ) )->register(); From 54fae11863c189a326866ec8da3d2eaba29e9d24 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:19:08 +0200 Subject: [PATCH 2/2] Run both environments behind composer test:unit --- AGENTS.md | 2 +- composer.json | 3 ++- tests/README.md | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 59aa15f..47b1af6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -49,7 +49,7 @@ slic run unit --env multisite # multisite slic run unit tests/unit/ConfigTest.php slic run unit "tests/unit/ConfigTest.php:test_it_rejects_invalid_hook_prefixes" -composer test:unit # wraps `slic run unit` +composer test:unit # both legs: `slic run unit` under singlesite, then multisite composer test:analysis # PHPStan level 9 — must stay green on every PR ``` diff --git a/composer.json b/composer.json index 5c28716..954e784 100644 --- a/composer.json +++ b/composer.json @@ -51,7 +51,8 @@ "phpstan analyse -c phpstan.neon.dist --memory-limit=1G" ], "test:unit": [ - "slic run unit" + "slic run unit --env singlesite", + "slic run unit --env multisite" ] }, "config": { diff --git a/tests/README.md b/tests/README.md index e977b5d..11e80e1 100644 --- a/tests/README.md +++ b/tests/README.md @@ -11,6 +11,8 @@ slic cc build slic run unit # singlesite slic run unit --env multisite # multisite + +composer test:unit # both legs, singlesite then multisite ``` CI runs both envs on PHP 7.4 and 8.5 — the ends of the supported range — against WordPress