diff --git a/tests/AutoReview/ComposerFileTest.php b/tests/AutoReview/ComposerFileTest.php index 31e76a2f240..aa862f88fa4 100644 --- a/tests/AutoReview/ComposerFileTest.php +++ b/tests/AutoReview/ComposerFileTest.php @@ -1,5 +1,16 @@ readComposerJson(); @@ -67,6 +85,12 @@ public function testScriptsHaveDescriptions(): void ); } + /** + * The platform PHP version satisfies the PHP requirement. + * + * Expected: Semver::satisfies() accepts config.platform.php from + * composer.json against require.php. + */ public function testPlatformPhpVersionSatisfiesPhpRequirement(): void { $composer_json = $this->readComposerJson(); diff --git a/tests/AutoReview/ForumTest.php b/tests/AutoReview/ForumTest.php index b284304a554..30934ad776b 100644 --- a/tests/AutoReview/ForumTest.php +++ b/tests/AutoReview/ForumTest.php @@ -1,5 +1,16 @@ assertNotEmpty(Config::$modSettings['smfVersion']); $this->assertSame(SMF_VERSION, Config::$modSettings['smfVersion']); } + /** + * The configured database engine is one SMF supports. + * + * Expected: the lower-cased Config::$db_type is mysql or postgresql. + */ public function testTheEngineIsOneSmfSupports(): void { $this->assertContains( @@ -50,8 +71,14 @@ public function testTheEngineIsOneSmfSupports(): void } /** - * Writes a row the next test then looks for. Together with the test below, - * this is what proves the rollback in tearDown() is real. + * A write is visible inside the test that made it. + * + * Writes a row the next test then looks for. Together with + * testThatWriteIsGoneByTheNextTest(), this is what proves the rollback in + * tearDown() is real. + * + * Expected: after replacing a setting, rawSetting() returns the value that + * was written. */ public function testAWriteIsVisibleInsideTheTestThatMadeIt(): void { @@ -66,6 +93,12 @@ public function testAWriteIsVisibleInsideTheTestThatMadeIt(): void $this->assertSame('written', $this->rawSetting(self::LEFTOVER)); } + /** + * The previous test's write is gone by the next test. + * + * Expected: rawSetting() for the variable the previous test wrote returns + * null. + */ #[Depends('testAWriteIsVisibleInsideTheTestThatMadeIt')] public function testThatWriteIsGoneByTheNextTest(): void { @@ -75,21 +108,41 @@ public function testThatWriteIsGoneByTheNextTest(): void ); } + /** + * Config::$modSettings is restored even though it is a static array. + * + * The rollback returns the table, not the copy in memory. tearDown() has to + * put that back by hand, and testModSettingsHasNoLeftoversFromTheLastTest() + * is the check that it does. + * + * Expected: a key written only to Config::$modSettings is present for the + * rest of this test. + */ public function testModSettingsIsRestoredEvenThoughItIsAStaticArray(): void { - // The rollback returns the table, not the copy in memory. tearDown() has - // to put that back by hand, and this is the check that it does. Config::$modSettings['smf_tests_in_memory_only'] = 'x'; $this->assertArrayHasKey('smf_tests_in_memory_only', Config::$modSettings); } + /** + * Config::$modSettings has no leftovers from the last test. + * + * Expected: the key the previous test wrote only to memory is absent from + * Config::$modSettings. + */ #[Depends('testModSettingsIsRestoredEvenThoughItIsAStaticArray')] public function testModSettingsHasNoLeftoversFromTheLastTest(): void { $this->assertArrayNotHasKey('smf_tests_in_memory_only', Config::$modSettings); } + /** + * adminId() finds an administrator that actingAs() can become. + * + * Expected: adminId() returns a positive id, and after actingAs() with it + * User::$me->id is that id and User::$me->is_admin is true. + */ public function testAdminIdFindsAnAdministrator(): void { $id = $this->adminId(); @@ -102,15 +155,25 @@ public function testAdminIdFindsAnAdministrator(): void $this->assertTrue(\SMF\User::$me->is_admin, 'actingAs() did not produce an administrator'); } + /** + * A test that does nothing logs no errors. + * + * Expected: assertNoErrorsLogged() passes in a test with an empty body. + */ public function testNothingIsLoggedByAnEmptyTest(): void { $this->assertNoErrorsLogged(); } /** + * assertNoErrorsLogged() notices an error logged during the test. + * * The assertion is only worth anything if it can fail, and it reads the log * through a watermark taken in setUp() rather than a count, so an empty log * is not what makes it pass. + * + * Expected: after a row is inserted into log_errors, assertNoErrorsLogged() + * throws AssertionFailedError. */ public function testAssertNoErrorsLoggedNoticesALoggedError(): void { diff --git a/tests/Integration/Http/CronTest.php b/tests/Integration/Http/CronTest.php index c29fa09c4b8..d81319d2bda 100644 --- a/tests/Integration/Http/CronTest.php +++ b/tests/Integration/Http/CronTest.php @@ -1,5 +1,16 @@ assertNoErrorsLogged($name . ' (' . $path . ') logged something.' . "\n"); } + /** + * The board index lists at least one board. + * + * Expected: the board index links to at least one board=, and logs nothing. + */ public function testTheBoardIndexListsAtLeastOneBoard(): void { $response = $this->fetch(''); @@ -51,9 +73,14 @@ public function testTheBoardIndexListsAtLeastOneBoard(): void } /** - * The one page here that is not HTML. It is worth its place because the feed - * is built by hand rather than by the template layer, so nothing else in this - * file would notice it breaking. + * The RSS feed is served as XML that parses. + * + * This is the one page here that is not HTML. It is worth its place because + * the feed is built by hand rather than by the template layer, so nothing + * else in this file would notice it breaking. + * + * Expected: ?action=.xml;type=rss2 returns an XML content type and a body + * that simplexml_load_string() parses, and logs nothing. */ public function testTheFeedIsXmlAndParses(): void { @@ -75,8 +102,11 @@ public function testTheFeedIsXmlAndParses(): void } /** - * An action that does not exist should be a 404, not a 200 with an apology - * and not a 500. + * An action that does not exist is a 404. + * + * It should be neither a 200 with an apology nor a 500. + * + * Expected: ?action=smf_tests_no_such_action returns status 404. */ public function testAnUnknownActionIsNotFound(): void { @@ -91,6 +121,10 @@ public function testAnUnknownActionIsNotFound(): void * page here that a cold request is genuinely not allowed to reach, and the * difference between the two halves is worth stating: arriving at the forum * first is what makes it work, and that is what a browser does. + * + * Expected: ?action=signup with no cookies returns 403. After visiting the + * forum, it shows the agreement, and accepting that leads to the + * registration form, with nothing logged. */ public function testRegistrationNeedsASessionFirst(): void { diff --git a/tests/Integration/Http/HttpTestCase.php b/tests/Integration/Http/HttpTestCase.php index c0716b7b262..dabdad77917 100644 --- a/tests/Integration/Http/HttpTestCase.php +++ b/tests/Integration/Http/HttpTestCase.php @@ -1,5 +1,16 @@ signInAsAdmin(); @@ -43,6 +61,12 @@ public function testTheAdministratorCanSignIn(): void $this->assertNoErrorsLogged('signing in logged something.' . "\n"); } + /** + * The wrong password does not sign anyone in. + * + * Expected: submitting the login form with the administrator's name and a + * wrong password leaves the session signed out. + */ public function testTheWrongPasswordDoesNotSignAnyoneIn(): void { $form = $this->fetch('?action=login'); @@ -56,9 +80,13 @@ public function testTheWrongPasswordDoesNotSignAnyoneIn(): void } /** - * A post carrying no session check should be turned away. This is the guard - * that stops another site from posting to the forum on a visitor's behalf, - * and nothing that does not go over HTTP can exercise it. + * A post carrying no session check is turned away. + * + * This is the guard that stops another site from posting to the forum on a + * visitor's behalf, and nothing that does not go over HTTP can exercise it. + * + * Expected: posting the correct credentials to ?action=login2 without the + * form's session fields leaves the session signed out. */ public function testAPostWithoutTheSessionCheckIsRejected(): void { @@ -72,6 +100,12 @@ public function testAPostWithoutTheSessionCheckIsRejected(): void $this->assertSignedIn(false, 'a login with no session check was accepted'); } + /** + * Signing out ends the session. + * + * Expected: following the logout link on a signed in page leaves the + * session signed out, and logs nothing. + */ public function testSigningOutEndsTheSession(): void { // This test should not use the cached authenticated client. diff --git a/tests/Integration/Http/MemberlistSearchTest.php b/tests/Integration/Http/MemberlistSearchTest.php index 72239aedafa..7ee39f9f224 100644 --- a/tests/Integration/Http/MemberlistSearchTest.php +++ b/tests/Integration/Http/MemberlistSearchTest.php @@ -1,5 +1,16 @@ signInAsAdmin(); @@ -87,8 +105,12 @@ public function testStartingATopicAndReplyingToIt(): void } /** - * A guest cannot post on a stock install, and the forum should say so rather - * than accept it. + * A guest cannot start a topic on a stock install. + * + * The forum should say so rather than accept the post. + * + * Expected: posting to ?action=post2;board=1 as a guest leaves the number + * of topics in board 1 unchanged. */ public function testAGuestCannotStartATopic(): void { diff --git a/tests/Integration/Http/ProfileTrackingTest.php b/tests/Integration/Http/ProfileTrackingTest.php index 496ff68ff40..093c859c88f 100644 --- a/tests/Integration/Http/ProfileTrackingTest.php +++ b/tests/Integration/Http/ProfileTrackingTest.php @@ -1,5 +1,16 @@ assertStringNotContainsString(self::ONCE, $body); } + /** + * A member emailed once until they visit is told so. + * + * Expected: the reply notification sent to a member whose msg_notify_pref + * is 2 contains the "until you visit it" wording. + */ public function testAMemberEmailedOnceUntilTheyVisitIsToldSo(): void { $body = $this->notify(2); diff --git a/tests/Integration/Http/StatsStartersTest.php b/tests/Integration/Http/StatsStartersTest.php index 601e1d62571..5cebf302921 100644 --- a/tests/Integration/Http/StatsStartersTest.php +++ b/tests/Integration/Http/StatsStartersTest.php @@ -1,5 +1,16 @@ '41']) makes rawSetting() and + * Config::$modSettings both return '41'. + */ public function testWritesAValue(): void { Config::updateModSettings([self::COUNTER => '41']); @@ -43,6 +60,20 @@ public function testWritesAValue(): void $this->assertSame('41', Config::$modSettings[self::COUNTER]); } + /** + * Passing true as a value adds one to the setting. + * + * Expected: after updateModSettings([$name => true], true) on a setting of + * 41, the setting is 42. + * Guards: the increment was emitted as SET value = value + 1 against + * settings.value, which is a text column. MySQL coerces that; + * PostgreSQL rejects it, and because its API discarded failed + * queries without a word, the statement did nothing. Counters such + * as totalMessages and totalMembers stayed frozen at the last full + * recount. + * + * @link https://github.com/SimpleMachines/SMF/pull/9340 + */ public function testIncrementsACounter(): void { Config::updateModSettings([self::COUNTER => '41']); @@ -56,6 +87,16 @@ public function testIncrementsACounter(): void ); } + /** + * Passing false as a value takes one away from the setting. + * + * Expected: after updateModSettings([$name => false], true) on a setting of + * 41, the setting is 40. + * Guards: the decrement was emitted as SET value = value - 1 against a text + * column, which PostgreSQL rejects, as it did for the increment. + * + * @link https://github.com/SimpleMachines/SMF/pull/9340 + */ public function testDecrementsACounter(): void { Config::updateModSettings([self::COUNTER => '41']); @@ -65,8 +106,12 @@ public function testDecrementsACounter(): void } /** + * An incremented counter stays a plain integer. + * * The value has to stay something the next increment can read back, so a * cast that leaves '42.0000' behind is not good enough. + * + * Expected: after an increment, rawSetting() matches ~^\d+$~. */ public function testAnIncrementedCounterStaysAPlainInteger(): void { @@ -80,10 +125,17 @@ public function testAnIncrementedCounterStaysAPlainInteger(): void ); } + /** + * A counter can be incremented repeatedly. + * + * It starts at 10 rather than 0 on purpose: see + * testDoesNotCreateANewSettingHoldingAFalsyValue() for why a brand new + * setting cannot be created holding a falsy value. + * + * Expected: three increments of a setting of 10 leave it at 13. + */ public function testCountersCanBeIncrementedRepeatedly(): void { - // Starts at 10 rather than 0 on purpose: see the test below for why a - // brand new setting cannot be created holding a falsy value. Config::updateModSettings([self::COUNTER => '10']); for ($i = 0; $i < 3; $i++) { @@ -94,10 +146,15 @@ public function testCountersCanBeIncrementedRepeatedly(): void } /** + * A new setting is not created holding a falsy value. + * * A setting that does not exist yet and would only be set to nothingness is * skipped rather than written. That is deliberate, and it is a sharp edge: * seeding a counter at zero looks like it worked and leaves no row, so the * first increment then has nothing to increment. + * + * Expected: updateModSettings([$name => '0']) leaves rawSetting() null, while + * a setting that already exists can be set to '0'. */ public function testDoesNotCreateANewSettingHoldingAFalsyValue(): void { @@ -112,6 +169,12 @@ public function testDoesNotCreateANewSettingHoldingAFalsyValue(): void $this->assertSame('0', $this->rawSetting(self::COUNTER)); } + /** + * Updating settings logs no errors. + * + * Expected: after a write, an increment and a decrement, + * assertNoErrorsLogged() passes. + */ public function testUpdatingSettingsLogsNoErrors(): void { Config::updateModSettings([self::COUNTER => '1']); diff --git a/tests/Integration/NotifyTest.php b/tests/Integration/NotifyTest.php index b0249937d74..c36220aba97 100644 --- a/tests/Integration/NotifyTest.php +++ b/tests/Integration/NotifyTest.php @@ -1,5 +1,16 @@ assertNotEmpty(Table::getAll('v3_0'), 'the v3_0 schema declares no tables at all'); } + /** + * Every table the schema declares exists in the database. + * + * Expected: no table in Table::getAll('v3_0') is missing from + * Db::$db->list_tables(). + */ public function testEveryDeclaredTableExists(): void { $existing = array_map( @@ -52,6 +75,14 @@ public function testEveryDeclaredTableExists(): void $this->assertSame([], $missing, 'tables the schema declares but the database does not have'); } + /** + * Every column the schema declares exists in the database. + * + * A table that is missing entirely is the business of + * testEveryDeclaredTableExists(). + * + * Expected: no column of a declared table is missing from its list_columns(). + */ public function testEveryDeclaredColumnExists(): void { $missing = []; @@ -78,9 +109,14 @@ public function testEveryDeclaredColumnExists(): void } /** - * The reverse direction, which is the one that catches a migration that - * dropped a column in the schema but not in the database, or a table left - * behind by an older version. + * The database has no columns the schema does not declare. + * + * This is the reverse direction, which is the one that catches a migration + * that dropped a column in the schema but not in the database, or a table + * left behind by an older version. + * + * Expected: every column list_columns() returns for a declared table is + * declared by that table. */ public function testTheDatabaseHasNoColumnsTheSchemaDoesNotDeclare(): void { diff --git a/tests/Support/HttpClient.php b/tests/Support/HttpClient.php index 3674e8496ae..ea4e0a5294c 100644 --- a/tests/Support/HttpClient.php +++ b/tests/Support/HttpClient.php @@ -1,5 +1,16 @@ assertTrue(is_a($class_name, Routable::class, true)); } + /** + * Each class builds a route result of the expected structure. + * + * Expected: for each class and each case's parameters, buildRoute() returns an + * array of exactly two entries, 'route' and 'params', both arrays. + */ #[DataProvider('provideClassAndRouteCases')] public function testStructure(string $class_name, string $route, array $params): void { @@ -56,6 +78,13 @@ public function testStructure(string $class_name, string $route, array $params): $this->assertIsArray($actual['params']); } + /** + * A route builds from its parameters and parses back to them. + * + * Expected: for each case in provideRouteCases(), QueryString::buildRoute() + * returns the route, and QueryString::parseRoute() on that route, + * with its query string split off, returns the parameters. + */ #[DataProvider('provideRouteCases')] public function testRoute(string $route, array $params): void { diff --git a/tests/TestCase/SlugRouteTestTrait.php b/tests/TestCase/SlugRouteTestTrait.php index aa01bbdb06f..603608a63ff 100644 --- a/tests/TestCase/SlugRouteTestTrait.php +++ b/tests/TestCase/SlugRouteTestTrait.php @@ -1,5 +1,16 @@ assertInstanceOf(Login2::class, Login2::load()); $this->assertInstanceOf(Logout::class, Logout::load()); } + /** + * load() returns the right class when the parent was loaded first. + * + * Expected: after Login2::load(), Logout::load() returns a Logout and + * Login::load() returns a Login. + * Guards: $obj is a static property declared in the trait, so it was shared + * with every descendant that does not redeclare it. Loading the + * parent first left the parent's instance in the slot the child + * reads, so Logout::load() returned a Login2 and failed its + * static return type. A banned member got a fatal error instead + * of being logged out. + * + * @link https://github.com/SimpleMachines/SMF/commit/5642a041f Introduced by "Move a bunch of repeat functions to a trait" + * @link https://github.com/SimpleMachines/SMF/pull/9321 + */ public function testLoadIsStillCorrectWhenTheParentWasLoadedFirst(): void { - // $obj is a static property declared in the trait, so it is shared with - // every descendant that does not redeclare it. Loading the parent first - // used to leave the parent's instance in the slot the child reads. Login2::load(); $this->assertInstanceOf(Logout::class, Logout::load()); $this->assertInstanceOf(Login::class, Login::load()); } + /** + * load() returns the right class when the child was loaded first. + * + * Expected: after Logout::load(), Login2::load() returns a Login2. + */ public function testLoadIsStillCorrectWhenTheChildWasLoadedFirst(): void { Logout::load(); @@ -46,12 +79,23 @@ public function testLoadIsStillCorrectWhenTheChildWasLoadedFirst(): void $this->assertInstanceOf(Login2::class, Login2::load()); } + /** + * load() returns the right class in other action hierarchies too. + * + * Expected: after Agreement::load() and Unread::load(), AgreementAccept::load() + * returns an AgreementAccept and UnreadReplies::load() returns an + * UnreadReplies. + * Guards: Eleven action classes extend another action and none + * redeclare $obj, so the shared slot was not specific to the + * login hierarchy. + * Notify is abstract and so cannot be loaded at all; Agreement + * and Unread are the other pairs with a concrete parent. + * + * @link https://github.com/SimpleMachines/SMF/commit/5642a041f Introduced by "Move a bunch of repeat functions to a trait" + * @link https://github.com/SimpleMachines/SMF/pull/9321 + */ public function testTheSameProblemInAnUnrelatedHierarchy(): void { - // Eleven action classes extend another action and none redeclare $obj, - // so this is not specific to the login hierarchy. Notify is abstract and - // so cannot be loaded at all; Agreement and Unread are the other pairs - // with a concrete parent. Agreement::load(); Unread::load(); @@ -59,6 +103,11 @@ public function testTheSameProblemInAnUnrelatedHierarchy(): void $this->assertInstanceOf(UnreadReplies::class, UnreadReplies::load()); } + /** + * load() returns the same instance every time. + * + * Expected: Login2::load() called twice returns the identical object. + */ public function testLoadCachesTheInstanceItReturns(): void { $this->assertSame(Login2::load(), Login2::load()); diff --git a/tests/Unit/AvatarFallbackTest.php b/tests/Unit/AvatarFallbackTest.php index d66465eefeb..3a7440b97ba 100644 --- a/tests/Unit/AvatarFallbackTest.php +++ b/tests/Unit/AvatarFallbackTest.php @@ -1,5 +1,16 @@ assertSame('https://example.com/gallery/cake.png', (string) $avatar->url); } + /** + * A file in a subdirectory of the configured gallery is found as well. + * + * Expected: with avatar_directory pointing at Themes/default/images, new + * Avatar(url: 'icons/bell.png', id_member: 1) has the url + * https://example.com/gallery/icons/bell.png. + * Guards: the file was looked for under the board directory's avatars + * directory whatever the setting said, so a gallery that had been + * moved was never searched, subdirectories included. + * + * @link https://github.com/SimpleMachines/SMF/commit/399ceedb4 Introduced by "Implements SMF\Avatar" + * @link https://github.com/SimpleMachines/SMF/pull/9588 + */ public function testTheSameForAFileInASubdirectoryOfTheGallery(): void { $this->setUpForum(Config::$boarddir . '/Themes/default/images'); @@ -64,8 +101,14 @@ public function testTheSameForAFileInASubdirectoryOfTheGallery(): void } /** + * The shipped gallery is used when nothing says otherwise. + * * A forum whose admin never touched the setting keeps the gallery SMF * ships, so the value the installer would have written is the fallback. + * + * Expected: with no avatar_directory setting, new Avatar(url: + * 'Oxygen/beagle.png', id_member: 1) has the url + * https://example.com/gallery/Oxygen/beagle.png. */ public function testTheShippedGalleryIsUsedWhenNothingSaysOtherwise(): void { diff --git a/tests/Unit/AvatarTest.php b/tests/Unit/AvatarTest.php index 9f8de65816c..95d77960c97 100644 --- a/tests/Unit/AvatarTest.php +++ b/tests/Unit/AvatarTest.php @@ -1,5 +1,16 @@ /avatars/Oxygen/beagle.png, with the filename + * Oxygen/beagle.png. + * Guards: the stored path was read as a URL, and the file was worked back + * to from that URL's path, which lands outside the avatar + * directories and finds nothing. On a forum at the root of its + * domain that path is null, and stripping the board URL off it + * threw a TypeError on every page the member appeared on. + * + * @link https://github.com/SimpleMachines/SMF/commit/399ceedb4 Introduced by "Implements SMF\Avatar" + * @link https://github.com/SimpleMachines/SMF/pull/9442 */ #[DataProvider('boardUrls')] public function testAGalleryAvatarIsFoundUnderTheAvatarsDirectory(string $boardurl): void @@ -70,6 +98,19 @@ public function testAGalleryAvatarIsFoundUnderTheAvatarsDirectory(string $boardu $this->assertSame('Oxygen/beagle.png', $avatar->filename); } + /** + * A gallery avatar in the root of the gallery is found too. + * + * Expected: each case in boardUrls() resolves new Avatar(url: + * 'default.png', id_member: 1) to the url + * /avatars/default.png, with the filename default.png. + * Guards: the stored path was read as a URL and worked back to a file + * from that URL's path, which finds nothing outside a path prefix + * and threw a TypeError on a forum at the root of its domain. + * + * @link https://github.com/SimpleMachines/SMF/commit/399ceedb4 Introduced by "Implements SMF\Avatar" + * @link https://github.com/SimpleMachines/SMF/pull/9442 + */ #[DataProvider('boardUrls')] public function testAGalleryAvatarInTheRootOfTheGalleryIsFoundToo(string $boardurl): void { @@ -82,8 +123,12 @@ public function testAGalleryAvatarInTheRootOfTheGalleryIsFoundToo(string $boardu } /** - * A gallery file that is not there falls through to the default image - * rather than producing a URL pointing at nothing. + * A gallery avatar that is not there falls back to the default image. + * + * It does not produce a URL pointing at nothing. + * + * Expected: new Avatar(url: 'Oxygen/no_such_avatar.png', id_member: 1) has + * the url https://example.com/avatars/default.png. */ public function testAGalleryAvatarThatIsNotThereFallsBackToTheDefault(): void { diff --git a/tests/Unit/BoardsCountPostsBackcompatTest.php b/tests/Unit/BoardsCountPostsBackcompatTest.php index 7d2f96ecbdf..b6d5d4835b5 100644 --- a/tests/Unit/BoardsCountPostsBackcompatTest.php +++ b/tests/Unit/BoardsCountPostsBackcompatTest.php @@ -1,5 +1,16 @@ assertSame($expected, $this->quoteFixes($class, $query)); } + /** + * A legacy insert is still rewritten for a mod that sends one. + * + * Expected: for each engine in engineProvider(), backcompatInsertFixes() + * turns the column count_posts, its value and its key into + * posts_count with the value inverted. + */ #[DataProvider('engineProvider')] public function testALegacyInsertIsStillRewrittenForAModThatSendsOne(string $class): void { @@ -65,6 +103,13 @@ public function testALegacyInsertIsStillRewrittenForAModThatSendsOne(string $cla $this->assertSame(['posts_count'], $keys); } + /** + * Nothing is rewritten when backward compatibility is off. + * + * Expected: for each engine in engineProvider(), backcompatQuoteFixes() + * returns 'SELECT id_board, count_posts FROM smf_boards' + * unchanged. + */ #[DataProvider('engineProvider')] public function testNothingIsRewrittenWithoutBackwardCompatibility(string $class): void { diff --git a/tests/Unit/CacheApiTest.php b/tests/Unit/CacheApiTest.php index 2ee3bd726f2..06f8f65c980 100644 --- a/tests/Unit/CacheApiTest.php +++ b/tests/Unit/CacheApiTest.php @@ -1,5 +1,16 @@ cachedir); } + /** + * An accelerator returns what was put into it. + * + * Expected: after put() of each value in cacheableValues(), + * get() returns an identical value, for every + * accelerator in accelerators(). + */ #[DataProvider('accelerators')] public function testAnAcceleratorReturnsWhatWasPutIntoIt(string $class): void { @@ -103,6 +121,13 @@ public function testAnAcceleratorReturnsWhatWasPutIntoIt(string $class): void } } + /** + * An object comes back as an equal object. + * + * Expected: after put() of a stdClass, get() returns an + * object equal to it, for every accelerator in + * accelerators(). + */ #[DataProvider('accelerators')] public function testAnObjectComesBackAsAnEqualObject(string $class): void { @@ -116,6 +141,13 @@ public function testAnObjectComesBackAsAnEqualObject(string $class): void $this->assertEquals($object, CacheApi::get('an_object', 120)); } + /** + * Putting null removes the entry. + * + * Expected: after put() of a value and then of null under the + * same key, get() returns null, for every + * accelerator in accelerators(). + */ #[DataProvider('accelerators')] public function testPuttingNullRemovesTheEntry(string $class): void { @@ -127,6 +159,12 @@ public function testPuttingNullRemovesTheEntry(string $class): void $this->assertNull(CacheApi::get('to_be_removed', 120)); } + /** + * A key that was never stored is a miss. + * + * Expected: get() of a key nothing was put under returns + * null, for every accelerator in accelerators(). + */ #[DataProvider('accelerators')] public function testAKeyThatWasNeverStoredIsAMiss(string $class): void { @@ -135,6 +173,12 @@ public function testAKeyThatWasNeverStoredIsAMiss(string $class): void $this->assertNull(CacheApi::get('never_stored_' . bin2hex(random_bytes(4)), 120)); } + /** + * Cleaning the cache empties it. + * + * Expected: after put() and then clean(), get() returns null, + * for every accelerator in accelerators(). + */ #[DataProvider('accelerators')] public function testCleaningTheCacheEmptiesIt(string $class): void { @@ -147,8 +191,14 @@ public function testCleaningTheCacheEmptiesIt(string $class): void } /** + * An entry is gone once its time to live has passed. + * * APCu is left out because it reads a time to live of zero or less as * "never expires" rather than as a moment already past. + * + * Expected: after put() with a time to live of -1, get() + * returns null, for every accelerator in + * acceleratorsWithARelativeTtl(). */ #[DataProvider('acceleratorsWithARelativeTtl')] public function testAnEntryIsGoneOnceItsTimeToLiveHasPassed(string $class): void @@ -161,9 +211,18 @@ public function testAnEntryIsGoneOnceItsTimeToLiveHasPassed(string $class): void } /** - * Turning on write ahead logging used to leave the SQLite accelerator with - * no table at all: the pragma writes a header, so the emptiness test that - * decided whether to create one never passed on a fresh database. + * The SQLite cache stores values with write ahead logging on. + * + * Expected: with Config::$cache_sqlite_wal set, put() of ['a' + * => 1] followed by get() returns ['a' => 1]. + * Guards: the pragma writes a header, so the check that + * decided whether to create the table, which + * looked for an empty file, never passed on a + * fresh database. The accelerator had no table, + * every read missed and every write threw. + * + * @link https://github.com/SimpleMachines/SMF/commit/48f3d7bcb Introduced by "[3.0] Add support for WAL in Sqlite3 caching" + * @link https://github.com/SimpleMachines/SMF/pull/9627 */ public function testTheSqliteCacheStoresValuesWithWriteAheadLoggingOn(): void { @@ -177,9 +236,20 @@ public function testTheSqliteCacheStoresValuesWithWriteAheadLoggingOn(): void } /** - * The file cache stores JSON, and json_encode() returns false rather than a - * string for anything that is not valid UTF-8. Handing that on to the writer - * took the request down with a TypeError; a value it cannot store is a miss. + * A value the file cache cannot encode is a miss rather than a fatal + * error. + * + * The file cache stores JSON, and json_encode() returns false rather + * than a string for anything that is not valid UTF-8. + * + * Expected: put() of an array holding invalid UTF-8 does not + * throw, and get() returns null. + * Guards: the false from json_encode() was handed on to the + * writer, which took the request down with a + * TypeError. + * + * @link https://github.com/SimpleMachines/SMF/commit/8b7ef30ce Introduced by "Start adding types and enforcing strict typing" + * @link https://github.com/SimpleMachines/SMF/pull/9627 */ public function testAValueTheFileCacheCannotEncodeIsAMissRatherThanAFatal(): void { @@ -191,9 +261,19 @@ public function testAValueTheFileCacheCannotEncodeIsAMissRatherThanAFatal(): voi } /** - * A truncated entry unserialises to false, which is also what a cached false - * looks like. Telling them apart is the difference between a miss and handing - * the caller a value nobody ever stored. + * An entry that will not unserialise reads as a miss. + * + * A truncated entry unserialises to false, which is also what a cached + * false looks like. Telling them apart is the difference between a miss + * and handing the caller a value nobody ever stored. + * + * Expected: get() of a file cache entry whose value is not + * serialised data returns null. + * Guards: the false from unserialize() was returned as the + * cached value. + * + * @link https://github.com/SimpleMachines/SMF/commit/17fdb6e07 Introduced by "Uses serialization rather than JSON to cache. Needed for objects." + * @link https://github.com/SimpleMachines/SMF/pull/9627 */ public function testAnEntryThatWillNotUnserialiseReadsAsAMiss(): void { @@ -208,6 +288,15 @@ public function testAnEntryThatWillNotUnserialiseReadsAsAMiss(): void $this->assertNull(CacheApi::get('corrupt', 120)); } + /** + * A cached false is still read back as false. + * + * This is the other side of + * testAnEntryThatWillNotUnserialiseReadsAsAMiss(): a miss must not be + * mistaken for a stored false either. + * + * Expected: after put() of false, get() returns false. + */ public function testACachedFalseIsStillReadBackAsFalse(): void { $this->loadApi(FileBased::class); @@ -218,10 +307,23 @@ public function testACachedFalseIsStillReadBackAsFalse(): void } /** - * Adding a server or-ed a bool into a bool, which PHP hands back as an int, - * and the method says it returns a bool. Every page load fatalled the moment - * memcached was the chosen accelerator. No server is needed to see it, since - * addServer() only records where the server is meant to be. + * The memcached accelerator connects. + * + * No server is needed to see this, since addServer() only records where + * the server is meant to be. + * + * Expected: connect() returns true with + * Config::$cache_memcached set to + * '127.0.0.1:11211'; the test is skipped when the + * memcached extension is not installed. + * Guards: adding a server or-ed a bool into a bool, which + * PHP hands back as an int, and the method + * declares a bool. Every page load died with a + * TypeError the moment memcached was the chosen + * accelerator. + * + * @link https://github.com/SimpleMachines/SMF/commit/8b7ef30ce Introduced by "Start adding types and enforcing strict typing" + * @link https://github.com/SimpleMachines/SMF/pull/9627 */ public function testTheMemcachedAcceleratorConnects(): void { diff --git a/tests/Unit/ClassNameSyntaxTest.php b/tests/Unit/ClassNameSyntaxTest.php index 24d80ec2faa..c6522aaf435 100644 --- a/tests/Unit/ClassNameSyntaxTest.php +++ b/tests/Unit/ClassNameSyntaxTest.php @@ -1,5 +1,16 @@ assertSame('2001:db8::1', (string) new IP('2001:DB8::0001')); } + /** + * An IPv4-mapped IPv6 address keeps its dotted tail. + * + * Expected: (string) new IP('::ffff:1.2.3.4') returns '::ffff:1.2.3.4'. + */ public function testItKeepsIPv4MappedAddressesIntact(): void { $this->assertSame('::ffff:1.2.3.4', (string) new IP('::ffff:1.2.3.4')); } + /** + * The flags given to isValid() restrict validation to one address family. + * + * Expected: 1.2.3.4 is valid with FILTER_FLAG_IPV4 but not with + * FILTER_FLAG_IPV6, and 2001:db8::1 is valid with + * FILTER_FLAG_IPV6. + */ public function testFlagsNarrowValidationToOneFamily(): void { $this->assertTrue((new IP('1.2.3.4'))->isValid(FILTER_FLAG_IPV4)); @@ -33,6 +61,12 @@ public function testFlagsNarrowValidationToOneFamily(): void $this->assertTrue((new IP('2001:db8::1'))->isValid(FILTER_FLAG_IPV6)); } + /** + * An address converts to hex and packed binary and back again. + * + * Expected: for 1.2.3.4, toHex() returns '01020304', toBinary() is four + * bytes, and an IP built from that binary casts to '1.2.3.4'. + */ public function testBinaryAndHexRoundTrip(): void { $ip = new IP('1.2.3.4'); @@ -42,6 +76,11 @@ public function testBinaryAndHexRoundTrip(): void $this->assertSame('1.2.3.4', (string) new IP((string) $ip->toBinary())); } + /** + * An empty or unparseable value is not a valid address. + * + * Expected: isValid() is false for '', 'abcde' and '999.999.999.999'. + */ public function testAnEmptyOrUnparseableValueIsNotValid(): void { $this->assertFalse((new IP(''))->isValid()); @@ -49,11 +88,19 @@ public function testAnEmptyOrUnparseableValueIsNotValid(): void $this->assertFalse((new IP('999.999.999.999'))->isValid()); } + /** + * Any four or sixteen byte string is read as a packed address. + * + * The constructor accepts the packed binary form, and it cannot tell that + * apart from a four character string. This is a sharp edge worth pinning + * down: 'nope' is not rejected, it becomes an address. + * + * Expected: (string) new IP('nope') returns '110.111.112.101' and is + * valid, and a sixteen character string is valid as an IPv6 + * address. + */ public function testAnyFourByteStringIsReadAsAPackedAddress(): void { - // The constructor accepts the packed binary form, and it cannot tell that - // apart from a four character string. This is a sharp edge worth pinning - // down: 'nope' is not rejected, it becomes an address. $this->assertSame('110.111.112.101', (string) new IP('nope')); $this->assertTrue((new IP('nope'))->isValid()); @@ -61,28 +108,59 @@ public function testAnyFourByteStringIsReadAsAPackedAddress(): void $this->assertTrue((new IP('not an ip at all'))->isValid()); } + /** + * ip2range() reads a range whose ends are IPv6 addresses. + * + * The two ends of a range are only recognised as ends if they validate as + * addresses of any family. + * + * Expected: IP::ip2range('2001:db8::1-2001:db8::ff') has the low end + * '2001:db8::1' and the high end '2001:db8::ff'. + * Guards: the ends were validated as IPv4 only, so neither end of an + * IPv6 range was one. The range fell through to the "one side is + * a fragment" path, which read the address itself as a list of + * octets to walk, and came back as 255.255.255.255. + * + * @link https://github.com/SimpleMachines/SMF/commit/d530ce5f5 Introduced by "Implements SMF\IP" + * @link https://github.com/SimpleMachines/SMF/pull/9506 + */ public function testIp2RangeReadsARangeOfIPv6Addresses(): void { - // The two ends of a range are only recognised as ends if they validate - // as addresses. Insisting on IPv4 there meant neither end of an IPv6 - // range was one, so this fell through to the "one side is a fragment" - // path and read the address itself as a list of octets to walk. $range = IP::ip2range('2001:db8::1-2001:db8::ff'); $this->assertSame('2001:db8::1', (string) $range['low']); $this->assertSame('2001:db8::ff', (string) $range['high']); } + /** + * ip2range() reads an IPv6 range written with nothing elided. + * + * It is the same range as in testIp2RangeReadsARangeOfIPv6Addresses(), so + * the shortening on the way back out is the only difference between the + * two. + * + * Expected: IP::ip2range('2001:db8:0:0:0:0:0:1-2001:db8:0:0:0:0:0:ff') has + * the low end '2001:db8::1' and the high end '2001:db8::ff'. + * Guards: the ends were validated as IPv4 only, so neither end of an + * IPv6 range was recognised as one. + * + * @link https://github.com/SimpleMachines/SMF/commit/d530ce5f5 Introduced by "Implements SMF\IP" + * @link https://github.com/SimpleMachines/SMF/pull/9506 + */ public function testIp2RangeReadsAFullyWrittenIPv6Range(): void { - // Same range as above with nothing elided, so the shortening on the way - // back out is the only difference between the two. $range = IP::ip2range('2001:db8:0:0:0:0:0:1-2001:db8:0:0:0:0:0:ff'); $this->assertSame('2001:db8::1', (string) $range['low']); $this->assertSame('2001:db8::ff', (string) $range['high']); } + /** + * ip2range() still reads a range whose ends are IPv4 addresses. + * + * Expected: IP::ip2range('1.2.3.4-1.2.3.9') has the low end '1.2.3.4' and + * the high end '1.2.3.9'. + */ public function testIp2RangeStillReadsARangeOfIPv4Addresses(): void { $range = IP::ip2range('1.2.3.4-1.2.3.9'); @@ -91,12 +169,24 @@ public function testIp2RangeStillReadsARangeOfIPv4Addresses(): void $this->assertSame('1.2.3.9', (string) $range['high']); } + /** + * isValid() accepts well-formed addresses and rejects everything else. + * + * Expected: each case in validityProvider() has isValid() return the + * stated boolean. + */ #[DataProvider('validityProvider')] public function testValidity(string $input, bool $expected): void { $this->assertSame($expected, (new IP($input))->isValid()); } + /** + * ip2range() fills a wildcard with the lowest and the highest value. + * + * Expected: each case in ip2RangeWildcardProvider() gives the stated low + * and high ends. + */ #[DataProvider('ip2RangeWildcardProvider')] public function testIp2RangeFillsWildcardsWithTheLowestAndHighestValue( string $input, @@ -109,6 +199,12 @@ public function testIp2RangeFillsWildcardsWithTheLowestAndHighestValue( $this->assertSame($high, (string) $range['high']); } + /** + * matchToCIDR() says whether an address lies inside a network. + * + * Expected: each case in cidrProvider() has matchToCIDR() return the + * stated boolean. + */ #[DataProvider('cidrProvider')] public function testMatchToCIDR(string $ip, string $cidr, bool $expected): void { diff --git a/tests/Unit/Infrastructure/ContainerFactoryTest.php b/tests/Unit/Infrastructure/ContainerFactoryTest.php index 202576832d3..6e6f4c59ee2 100644 --- a/tests/Unit/Infrastructure/ContainerFactoryTest.php +++ b/tests/Unit/Infrastructure/ContainerFactoryTest.php @@ -1,5 +1,16 @@ createMock(ContainerInterface::class); @@ -29,6 +46,12 @@ public function testHasDelegatesToContainer(): void $this->assertTrue($services->has('test')); } + /** + * Services::get() delegates to the container. + * + * Expected: get('test') asks the container's get('test') once and returns the + * service it gave. + */ public function testGetDelegatesToContainer(): void { $container = $this->createMock(ContainerInterface::class); diff --git a/tests/Unit/LangTest.php b/tests/Unit/LangTest.php index 1a86fe14cae..6ed4f494728 100644 --- a/tests/Unit/LangTest.php +++ b/tests/Unit/LangTest.php @@ -1,5 +1,16 @@ assertSame('en_US', Lang::load('General')); @@ -55,18 +73,32 @@ public function testItLoadsLanguageStringsWithNoDatabase(): void $this->assertArrayHasKey('days', Lang::$txt); } + /** + * With no current user, the language defaults to the forum language. + * + * User::$me is a typed static with no default, and reading an + * uninitialized one throws rather than yielding null. load() gets away + * with `User::$me->language ?? Config::$language` only because ?? + * evaluates its left side in an isset() context, which is easy to break + * by "tidying" it into something that reads the property first. + * + * Expected: with User::$me unset, load('General') returns + * Config::$language. + */ public function testItDefaultsToTheForumLanguageWhenThereIsNoUser(): void { - // User::$me is a typed static with no default, and reading an - // uninitialized one throws rather than yielding null. load() gets away - // with `User::$me->language ?? Config::$language` only because ?? - // evaluates its left side in an isset() context, which is easy to - // break by "tidying" it into something that reads the property first. $this->assertFalse(isset(User::$me)); $this->assertSame(Config::$language, Lang::load('General')); } + /** + * With no theme, only the languages directory is searched. + * + * Expected: after addDirs(), the private Lang::$dirs holds + * just the canonical path of + * Config::$languagesdir. + */ public function testItSearchesOnlyTheLanguagesDirectoryWhenThereIsNoTheme(): void { Lang::addDirs(); @@ -83,6 +115,13 @@ public function testItSearchesOnlyTheLanguagesDirectoryWhenThereIsNoTheme(): voi ); } + /** + * A custom directory that does not exist is ignored. + * + * Expected: after addDirs() is given a path that is not + * there, Lang::$dirs holds just the canonical path + * of Config::$languagesdir. + */ public function testItIgnoresACustomDirectoryThatIsNotThere(): void { Lang::addDirs(Config::$boarddir . '/no/such/directory'); @@ -95,6 +134,13 @@ public function testItIgnoresACustomDirectoryThatIsNotThere(): void ); } + /** + * The installed languages are listed with their names and locations. + * + * Expected: get(false) includes 'en_US', named 'English + * (US)', located at the canonical path of + * en_US/General.php in Config::$languagesdir. + */ public function testItListsTheInstalledLanguages(): void { $languages = Lang::get(false); @@ -111,6 +157,12 @@ public function testItListsTheInstalledLanguages(): void ); } + /** + * Naming a file when asking for a string loads that file. + * + * Expected: getTxt('number_of_days', [1], file: 'General') + * returns '1 day' and, with 2, returns '2 days'. + */ public function testItLoadsTheFileAStringWasAskedForFrom(): void { // Nothing has been loaded at this point; naming the file is what makes @@ -119,6 +171,14 @@ public function testItLoadsTheFileAStringWasAskedForFrom(): void $this->assertSame('2 days', Lang::getTxt('number_of_days', [2], file: 'General')); } + /** + * A string that only exists in a file on disk can be found. + * + * Expected: txtExists('actual_theme_dir') is false until + * file: 'Themes' is named, when it is true; + * txtExists('no_such_string_anywhere', file: + * 'Themes') is false. + */ public function testItFindsAStringThatOnlyExistsInAFileOnDisk(): void { $this->assertFalse(Lang::txtExists('actual_theme_dir')); @@ -127,6 +187,21 @@ public function testItFindsAStringThatOnlyExistsInAFileOnDisk(): void $this->assertFalse(Lang::txtExists('no_such_string_anywhere', file: 'Themes')); } + /** + * The language that was asked for is loaded, not the forum default. + * + * Expected: after load('TestStrings', 'de_DE', false) against + * the fixture languages, + * Lang::$txt['test_in_all_three'] is 'German'. + * Guards: load() stopped at the first file it found, which + * after the attempts were reversed was the forum + * default's, so a member whose language was + * anything else read the forum, and received + * email, in the forum default. + * + * @link https://github.com/SimpleMachines/SMF/commit/d33481a33 Introduced by "Uses consistent logic to load both standard and legacy language files" + * @link https://github.com/SimpleMachines/SMF/pull/9612 + */ public function testItLoadsTheLanguageItWasAskedForRatherThanTheForumDefault(): void { Lang::addDirs(self::fixtureLanguages()); @@ -143,12 +218,29 @@ public function testItLoadsTheLanguageItWasAskedForRatherThanTheForumDefault(): $this->assertSame('German', Lang::$txt['test_in_all_three']); } + /** + * A string missing from a translation falls back to the forum default, + * then English. + * + * The forum's default language need not be English, and then there are + * three files in play rather than two. Each string below is defined in + * a different subset of them, so together they say how far down the + * chain each one had to go. + * + * Expected: with the forum default es_ES, load('TestStrings', + * 'de_DE', false) gives 'German' for + * test_in_all_three, 'Spanish' for + * test_not_in_german and 'English' for + * test_english_only. + * Guards: load() stopped at the first file it found, which + * was the forum default's, so the language that + * was asked for was never loaded. + * + * @link https://github.com/SimpleMachines/SMF/commit/d33481a33 Introduced by "Uses consistent logic to load both standard and legacy language files" + * @link https://github.com/SimpleMachines/SMF/pull/9612 + */ public function testItFallsBackThroughTheForumDefaultToEnglishStringByString(): void { - // The forum's default language need not be English, and then there are - // three files in play rather than two. Each string below is defined in - // a different subset of them, so together they say how far down the - // chain each one had to go. Config::$language = 'es_ES'; Lang::addDirs(self::fixtureLanguages()); @@ -171,6 +263,22 @@ public function testItFallsBackThroughTheForumDefaultToEnglishStringByString(): // leaving a partial translation full of gaps. } + /** + * With the English fallback disabled, loading stops at the forum + * default. + * + * Expected: with disable_language_fallback on and es_ES as + * the forum default, load('TestStrings', 'de_DE', + * false) gives 'German' and 'Spanish' for the + * strings they define, and leaves + * test_english_only out of Lang::$txt. + * Guards: load() stopped at the first file it found, which + * was the forum default's, so the language that + * was asked for was never loaded. + * + * @link https://github.com/SimpleMachines/SMF/commit/d33481a33 Introduced by "Uses consistent logic to load both standard and legacy language files" + * @link https://github.com/SimpleMachines/SMF/pull/9612 + */ public function testItStopsAtTheForumDefaultWhenTheEnglishFallbackIsDisabled(): void { Config::$language = 'es_ES'; diff --git a/tests/Unit/MarkReadRouteTest.php b/tests/Unit/MarkReadRouteTest.php index 67401127c83..fbb08524ab6 100644 --- a/tests/Unit/MarkReadRouteTest.php +++ b/tests/Unit/MarkReadRouteTest.php @@ -1,5 +1,16 @@ 'markasread', 'sa' => 'all'], and the same + * with 'unreadreplies'. + * Guards: '??' binds tighter than '?:', so the line that restored the implied + * sub-action read as + * '($params['sa'] ?? isset($params['topic'])) ? ...'. + * A sub-action the route did state is a non-empty string, which is + * truthy, so it was replaced with 'topic'. Marking everything + * read then ran the per-topic branch with no topic loaded, which + * is a database error and a 500. + * + * @link https://github.com/SimpleMachines/SMF/commit/33ce3cf28 Introduced by "Improves routing for SMF\Actions\MarkRead" + * @link https://github.com/SimpleMachines/SMF/pull/9577 */ public function testASubActionInTheRouteSurvives(): void { @@ -37,6 +57,18 @@ public function testASubActionInTheRouteSurvives(): void ); } + /** + * A markasread route with a sub-action parses the same through QueryString. + * + * Expected: QueryString::parseRoute('/markasread/all', []) returns + * ['action' => 'markasread', 'sa' => 'all']. + * Guards: '/markasread/all' was parsed with the sub-action 'topic', so + * "Mark all messages as read" did not work on any forum with + * friendly URLs turned on. + * + * @link https://github.com/SimpleMachines/SMF/commit/33ce3cf28 Introduced by "Improves routing for SMF\Actions\MarkRead" + * @link https://github.com/SimpleMachines/SMF/pull/9577 + */ public function testTheSameThingThroughQueryString(): void { $this->assertSame( @@ -46,8 +78,13 @@ public function testTheSameThingThroughQueryString(): void } /** - * buildRoute() leaves the sub-action out when the topic or the board it is - * hanging off already implies it, so parsing has to put it back. + * A sub-action implied by the topic or board is restored when parsing. + * + * buildRoute() leaves the sub-action out when the topic or the board it + * is hanging off already implies it, so parsing has to put it back. + * + * Expected: MarkRead::parseRoute(['markasread'], ['topic' => '1']) adds 'sa' => + * 'topic', and with ['board' => '1'] it adds 'sa' => 'board'. */ public function testAnImpliedSubActionIsRestored(): void { @@ -63,8 +100,15 @@ public function testAnImpliedSubActionIsRestored(): void } /** - * With nothing to imply one, there is no sub-action to add. It used to be - * set to null, which put a valueless 'sa' into $_GET. + * A route with nothing to imply a sub-action gets none. + * + * Expected: MarkRead::parseRoute(['markasread']) returns + * ['action' => 'markasread'], with no 'sa' key. + * Guards: The sub-action was set to null, which put a valueless 'sa' + * into $_GET. + * + * @link https://github.com/SimpleMachines/SMF/commit/33ce3cf28 Introduced by "Improves routing for SMF\Actions\MarkRead" + * @link https://github.com/SimpleMachines/SMF/pull/9577 */ public function testNothingImpliesNothing(): void { diff --git a/tests/Unit/MessageFormatterTest.php b/tests/Unit/MessageFormatterTest.php index 93b054a9485..09bf4a774a8 100644 --- a/tests/Unit/MessageFormatterTest.php +++ b/tests/Unit/MessageFormatterTest.php @@ -1,5 +1,16 @@ + * ]) returns 'Hello Bob!'. + * Guards: the class skipped any argument that was not + * already a string and handed the intl formatter + * only the scalar ones, so an object reached + * neither and its placeholder was printed to the + * member as written. + * + * @link https://github.com/SimpleMachines/SMF/commit/f500ca99c Introduced by "Implements MessageFormat support" + * @link https://github.com/SimpleMachines/SMF/pull/9409 */ public function testAStringableArgumentIsUsedForItsStringValue(): void { @@ -38,10 +59,22 @@ public function testAStringableArgumentIsUsedForItsStringValue(): void } /** + * MessageFormat syntax in a Stringable value is not interpreted. + * * The braces and apostrophes in an argument are swapped for private use - * characters before the message is formatted and swapped back afterwards, - * so that a value cannot be read as MessageFormat syntax. A \Stringable is - * flattened early enough to go through that too. + * characters before the message is formatted and swapped back + * afterwards, so that a value cannot be read as MessageFormat syntax. A + * Stringable is flattened early enough to go through that too. + * + * Expected: formatMessage('Hello {name}!', ['name' => + * ]) returns "Hello it's + * {here}!". + * Guards: a Stringable argument never reached the formatter + * at all, so its placeholder was printed as + * written. + * + * @link https://github.com/SimpleMachines/SMF/commit/f500ca99c Introduced by "Implements MessageFormat support" + * @link https://github.com/SimpleMachines/SMF/pull/9409 */ public function testMessageFormatSyntaxInAStringableValueIsNotInterpreted(): void { @@ -51,6 +84,18 @@ public function testMessageFormatSyntaxInAStringableValueIsNotInterpreted(): voi ); } + /** + * The same Stringable argument can be used twice in one message. + * + * Expected: formatMessage('{name} and {name}', ['name' => + * ]) returns 'Bob and Bob'. + * Guards: a Stringable argument never reached the formatter + * at all, so both placeholders were printed as + * written. + * + * @link https://github.com/SimpleMachines/SMF/commit/f500ca99c Introduced by "Implements MessageFormat support" + * @link https://github.com/SimpleMachines/SMF/pull/9409 + */ public function testTheSameStringableCanBeUsedTwiceInOneMessage(): void { $this->assertSame( @@ -59,6 +104,12 @@ public function testTheSameStringableCanBeUsedTwiceInOneMessage(): void ); } + /** + * A plain string argument is unaffected. + * + * Expected: formatMessage('Hello {name}!', ['name' => 'Ann']) + * returns 'Hello Ann!'. + */ public function testAPlainStringArgumentIsUnaffected(): void { $this->assertSame( @@ -67,6 +118,12 @@ public function testAPlainStringArgumentIsUnaffected(): void ); } + /** + * A number argument is still formatted as a number. + * + * Expected: formatMessage('{count, plural, one {# post} other + * {# posts}}', ['count' => 2]) returns '2 posts'. + */ public function testANumberArgumentIsStillFormattedAsANumber(): void { $this->assertSame( @@ -75,6 +132,13 @@ public function testANumberArgumentIsStillFormattedAsANumber(): void ); } + /** + * A message with no placeholders comes back unchanged. + * + * Expected: formatMessage('Nothing to substitute', ['name' => + * ]) returns 'Nothing to + * substitute'. + */ public function testAMessageWithNoPlaceholdersComesBackUnchanged(): void { $this->assertSame( diff --git a/tests/Unit/PageIndexTest.php b/tests/Unit/PageIndexTest.php index 4af80dc1441..027200cadad 100644 --- a/tests/Unit/PageIndexTest.php +++ b/tests/Unit/PageIndexTest.php @@ -1,5 +1,16 @@ element. */ #[DataProvider('pageRenderingProvider')] public function testCurrentPageIsRendered(int $start, int $num_items, int $num_per_page, int $expected_page): void @@ -74,6 +96,21 @@ public function testCurrentPageIsRendered(int $start, int $num_items, int $num_p $this->assertStringContainsString(\sprintf('%d', $expected_page), (string) $page_index); } + /** + * A negative start links the first page instead of marking it as current. + * + * Expected: new PageIndex() with a start of -1, 100 items and 20 per page + * clamps the start to 0 and, cast to a string, links page 1 with + * start=0 and contains no current_page. + * Guards: __toString() clamped the start a second time, found nothing + * wrong with the 0 it was now given, and overwrote the record + * that the caller's start had been out of bounds. Page 1 came out + * as plain text rather than a link, so the first page of every + * multi-page topic on the message index could not be clicked. + * + * @link https://github.com/SimpleMachines/SMF/commit/7887c1b28 Introduced by "Implements SMF\PageIndex" + * @link https://github.com/SimpleMachines/SMF/pull/9453 + */ public function testANegativeStartLinksTheFirstPageInsteadOfMarkingIt(): void { $start = -1; @@ -88,6 +125,19 @@ public function testANegativeStartLinksTheFirstPageInsteadOfMarkingIt(): void $this->assertStringNotContainsString('current_page', $page_index); } + /** + * A negative start shows neither a previous nor a next page link. + * + * Expected: new PageIndex() with a start of -1, 100 items and 20 per page + * renders a string containing neither previous_page nor + * next_page. + * Guards: the record that the start was out of bounds was lost when + * __toString() clamped the start again, so page 1 was treated as + * the current page and got a next page link beside it. + * + * @link https://github.com/SimpleMachines/SMF/commit/7887c1b28 Introduced by "Implements SMF\PageIndex" + * @link https://github.com/SimpleMachines/SMF/pull/9453 + */ public function testANegativeStartShowsNeitherPreviousNorNextLinks(): void { $start = -1; @@ -100,9 +150,13 @@ public function testANegativeStartShowsNeitherPreviousNorNextLinks(): void } /** + * Casting to a string twice gives the same string both times. + * * The string is built in __toString() rather than the constructor so that * it reflects any property the caller changed in between, which means it * has to survive being asked more than once. + * + * Expected: (string) $page_index returns the same value on a second call. */ public function testAskingTwiceGivesTheSameAnswer(): void { @@ -112,6 +166,12 @@ public function testAskingTwiceGivesTheSameAnswer(): void $this->assertSame((string) $page_index, (string) $page_index); } + /** + * The start value is clamped and handed back to the caller. + * + * Expected: new PageIndex() with a start of -1 sets the variable passed in + * to 0. + */ public function testTheStartValueIsClampedAndHandedBackToTheCaller(): void { $start = -1; @@ -122,8 +182,15 @@ public function testTheStartValueIsClampedAndHandedBackToTheCaller(): void } /** - * The control. A start that names a real page marks that page as the - * current one and links the pages either side of it. + * A start that names a page marks that page as current. + * + * This is the control for the negative start tests. A start that names a + * real page marks that page as the current one and links the pages either + * side of it. + * + * Expected: new PageIndex() with a start of 40, 100 items and 20 per page + * sets current_page to 2, marks page 3 as current, and renders + * both previous_page and next_page. */ public function testAStartThatNamesAPageMarksThatPageAsCurrent(): void { @@ -139,8 +206,14 @@ public function testAStartThatNamesAPageMarksThatPageAsCurrent(): void } /** + * A start in the middle of a page is moved to the start of that page. + * * A start in the middle of a page belongs to that page, and the caller is * told which page that turned out to be. + * + * Expected: new PageIndex() with a start of 45, 100 items and 20 per page + * sets both the passed variable and PageIndex::$start to 40, + * sets current_page to 2, and marks page 3 as current. */ public function testAStartInTheMiddleOfAPageIsMovedToItsStart(): void { @@ -155,8 +228,15 @@ public function testAStartInTheMiddleOfAPageIsMovedToItsStart(): void } /** + * A start past the end lands on the last page. + * * A start past the end is clamped to the last page, and that is a real * page, so it is marked rather than linked. + * + * Expected: new PageIndex() with a start of 500, 100 items and 20 per page + * sets both the passed variable and PageIndex::$start to 80, + * sets current_page to 4, marks page 5 as current, and renders + * no next_page. */ public function testAStartPastTheEndLandsOnTheLastPage(): void { @@ -173,6 +253,13 @@ public function testAStartPastTheEndLandsOnTheLastPage(): void $this->assertStringNotContainsString('next_page', $page_index); } + /** + * The short format puts the bare offset into the URL. + * + * Expected: new PageIndex('index.php?board=1.%1$d', ..., 100, 20, true) + * links page 1 as index.php?board=1.0 and page 5 as + * index.php?board=1.80. + */ public function testShortFormatUsesOffsetInTheUrl(): void { $start = 20; @@ -184,6 +271,13 @@ public function testShortFormatUsesOffsetInTheUrl(): void $this->assertStringContainsString('href="index.php?board=1.80">5', $page_index); } + /** + * The default format appends a start parameter to the URL. + * + * Expected: new PageIndex('index.php?board=1', ..., 100, 20) links page 1 + * as index.php?board=1;start=0 and page 3 as + * index.php?board=1;start=40. + */ public function testDefaultFormatUsesStartParameterInTheUrl(): void { $start = 20; @@ -196,6 +290,13 @@ public function testDefaultFormatUsesStartParameterInTheUrl(): void $this->assertStringContainsString('href="index.php?board=1;start=40"', $page_index); } + /** + * Previous and next page links can be switched off. + * + * Expected: new PageIndex() with show_prevnext false, on page 3 of 5, + * renders neither previous_page nor next_page and still marks + * page 3 as current. + */ public function testPreviousAndNextLinksCanBeDisabled(): void { $start = 40; @@ -208,6 +309,13 @@ public function testPreviousAndNextLinksCanBeDisabled(): void $this->assertStringContainsString('3', $page_index); } + /** + * Template overrides replace the default templates. + * + * Expected: with overrides for current_page, page, previous_page and + * next_page, the string contains the overridden current page, + * page link, PREVIOUS and NEXT. + */ public function testTemplateOverridesReplaceDefaultTemplates(): void { $start = 20; @@ -227,6 +335,12 @@ public function testTemplateOverridesReplaceDefaultTemplates(): void $this->assertStringContainsString('NEXT', $page_index); } + /** + * An override for a template that does not exist is ignored. + * + * Expected: new PageIndex() given an override named does_not_exist renders + * a string that does not contain that override's text. + */ public function testUnknownTemplateOverrideIsIgnored(): void { $start = 0; @@ -238,6 +352,13 @@ public function testUnknownTemplateOverrideIsIgnored(): void $this->assertStringNotContainsString('unexpected', (string) $page_index); } + /** + * Template overrides can be applied after construction. + * + * Expected: calling setTemplateOverrides() on an existing PageIndex makes + * its string contain the overridden current page, BACK and + * FORWARD. + */ public function testSetTemplateOverridesCanBeAppliedAfterConstruction(): void { $start = 20; @@ -257,6 +378,12 @@ public function testSetTemplateOverridesCanBeAppliedAfterConstruction(): void $this->assertStringContainsString('FORWARD', $page_index); } + /** + * Compact pages can be disabled. + * + * Expected: with compactTopicPagesEnable at 0, a start of 40, 300 items and + * 20 per page renders all of pages 1 to 15 and no expandPages. + */ public function testCompactPagesCanBeDisabled(): void { Config::$modSettings['compactTopicPagesEnable'] = 0; @@ -273,6 +400,13 @@ public function testCompactPagesCanBeDisabled(): void $this->assertStringNotContainsString('expandPages', $page_index); } + /** + * Compact pages can be enabled. + * + * Expected: with compactTopicPagesEnable at 1 and + * compactTopicPagesContiguous at 5, a start of 140, 300 items and + * 20 per page marks page 8 as current and renders expandPages. + */ public function testCompactPagesCanBeEnabled(): void { Config::$modSettings['compactTopicPagesEnable'] = 1; @@ -288,6 +422,12 @@ public function testCompactPagesCanBeEnabled(): void $this->assertStringContainsString('expandPages', $page_index); } + /** + * An odd number of contiguous compact pages is rounded down. + * + * Expected: with compactTopicPagesContiguous at 4, a start of 140, 300 + * items and 20 per page still marks page 8 as current. + */ public function testOddCompactPageCountIsRoundedDown(): void { Config::$modSettings['compactTopicPagesEnable'] = 1; diff --git a/tests/Unit/PunycodeTest.php b/tests/Unit/PunycodeTest.php index f2a44376423..e7399bd4862 100644 --- a/tests/Unit/PunycodeTest.php +++ b/tests/Unit/PunycodeTest.php @@ -1,5 +1,16 @@ assertSame('example.com', (new Punycode())->encode('example.com')); } + /** + * encode() and decode() are inverses for internationalised domains. + * + * Expected: each case in domainProvider() encodes to the expected ASCII + * form and decodes back to the original. + */ #[DataProvider('domainProvider')] public function testEncodeAndDecodeAreInverses(string $unicode, string $ascii): void { diff --git a/tests/Unit/QueryCaseFoldingTest.php b/tests/Unit/QueryCaseFoldingTest.php index 51b95198e8d..921e03bc889 100644 --- a/tests/Unit/QueryCaseFoldingTest.php +++ b/tests/Unit/QueryCaseFoldingTest.php @@ -1,5 +1,16 @@ scan(); @@ -132,6 +148,12 @@ public function testNoNewComparisonSkipsTheCaseFoldingTypes(): void ); } + /** + * No new comparison folds its column without folding the value. + * + * Expected: scanUnfoldedValues() returns the per-file counts listed in + * UNFOLDED_VALUES. + */ public function testNoNewComparisonFoldsOnlyItsColumn(): void { $found = $this->scanUnfoldedValues(); @@ -148,10 +170,16 @@ public function testNoNewComparisonFoldsOnlyItsColumn(): void ); } + /** + * Both scans find the forms they are looking for. + * + * Without this, emptying either scan would leave the tests above passing + * against an empty baseline. + * + * Expected: scan() and scanUnfoldedValues() each return a non-empty array. + */ public function testTheScansFindTheFormsTheyAreLookingFor(): void { - // Without these, emptying either scan would leave the tests above - // passing against an empty baseline. $this->assertNotSame([], $this->scan()); $this->assertNotSame([], $this->scanUnfoldedValues()); } diff --git a/tests/Unit/QueryStringRouteTest.php b/tests/Unit/QueryStringRouteTest.php index c275d95ce43..c29becd8327 100644 --- a/tests/Unit/QueryStringRouteTest.php +++ b/tests/Unit/QueryStringRouteTest.php @@ -1,5 +1,16 @@ 'display', 'topic' => '1']. + */ public function testAPlainTopicRouteIsTheDisplayAction(): void { $this->assertSame( @@ -28,6 +45,12 @@ public function testAPlainTopicRouteIsTheDisplayAction(): void ); } + /** + * A plain board route is parsed as the message index action. + * + * Expected: parseRoute('/boards/2', []) returns + * ['action' => 'messageindex', 'board' => '2']. + */ public function testAPlainBoardRouteIsTheMessageIndexAction(): void { $this->assertSame( @@ -36,6 +59,12 @@ public function testAPlainBoardRouteIsTheMessageIndexAction(): void ); } + /** + * A parameter that already exists wins over the one in the route. + * + * Expected: parseRoute('/topics/42', ['topic' => '99']) returns the display + * action with topic 99. + */ public function testExistingParametersTakePrecedenceOverRoute(): void { $this->assertSame( @@ -49,6 +78,12 @@ public function testExistingParametersTakePrecedenceOverRoute(): void ); } + /** + * A path that is not a route leaves the parameters unchanged. + * + * Expected: parseRoute('not/a/route', ['action' => 'foo']) returns + * ['action' => 'foo']. + */ public function testNonRoutePathLeavesParametersUnchanged(): void { $params = ['action' => 'foo']; @@ -59,6 +94,12 @@ public function testNonRoutePathLeavesParametersUnchanged(): void ); } + /** + * An unknown route is read in the legacy queryless format. + * + * Expected: parseRoute('/action,foo/bar,baz', []) returns + * ['action' => 'foo', 'bar' => 'baz']. + */ public function testUnknownRouteUsesLegacyQuerylessFormat(): void { $this->assertSame( @@ -70,6 +111,12 @@ public function testUnknownRouteUsesLegacyQuerylessFormat(): void ); } + /** + * The legacy .html and .htm route extensions are ignored. + * + * Expected: each path in routeExtensionProvider() parses as the display action + * for topic 42. + */ #[DataProvider('routeExtensionProvider')] public function testLegacyRouteExtensionsAreIgnored( string $path, @@ -81,14 +128,25 @@ public function testLegacyRouteExtensionsAreIgnored( } /** - * Topic::parseRoute() and Board::parseRoute() used to look for the action - * suffix in QueryString::$route_parsers directly. That list only ever holds - * the handful of content parsers it is declared with, because the parser for - * an action is added to it on demand by QueryString::getRouteParser(). So - * the suffix was never recognised and fell through to the branch that treats - * whatever is left as a start value: '/topics/1/post' parsed as the display - * action with a start of 'post', and every posting, voting, poll, print and - * mark-as-read link on a topic or board silently reloaded the page instead. + * An action suffix on a topic or board route is parsed as the action. + * + * The parser for an action is added to QueryString::$route_parsers on + * demand by QueryString::getRouteParser(), so the suffix has to be + * looked up through that method rather than in the list. + * + * Expected: each case in actionSuffixProvider() parses to the action named by + * the suffix, with the topic or board kept. + * Guards: Topic::parseRoute() and Board::parseRoute() looked for the suffix in + * QueryString::$route_parsers directly. That list holds only the + * content parsers, because the parser for an action is added to + * it on demand by QueryString::getRouteParser(). So the suffix + * was never recognised and '/topics/1/post' parsed as the + * display action with a start of 'post'. Every posting, voting, + * poll, print and mark-as-read link on a topic or board + * silently reloaded the page instead. + * + * @link https://github.com/SimpleMachines/SMF/commit/39ca0ee61 Introduced by "Implements QueryString::getRouteParser()" + * @link https://github.com/SimpleMachines/SMF/pull/9575 */ #[DataProvider('actionSuffixProvider')] public function testAnActionSuffixOnATopicOrBoardRoute(string $path, array $expected): void @@ -97,11 +155,20 @@ public function testAnActionSuffixOnATopicOrBoardRoute(string $path, array $expe } /** + * A start value is kept alongside an action suffix. + * * A start value and an action suffix can both be present, because - * Topic::buildRoute() and Board::buildRoute() put the start value into the - * route before the action is appended to it. Parsing only ever consumed one - * of the two, so replying from any page of a topic but the first lost the - * action as well. + * Topic::buildRoute() and Board::buildRoute() put the start value into + * the route before the action is appended to it. + * + * Expected: each case in startValueProvider() parses to the topic or board, the + * start value and, when there is one, the action. + * Guards: Parsing took either the start value or the action suffix and never + * both, so replying from any page of a topic but the first lost the + * action as well. + * + * @link https://github.com/SimpleMachines/SMF/commit/244da60f0 Introduced by "Implements URL routing (i.e. better queryless URLs)" + * @link https://github.com/SimpleMachines/SMF/pull/9575 */ #[DataProvider('startValueProvider')] public function testAStartValueIsKeptAlongsideTheAction(string $path, array $expected): void diff --git a/tests/Unit/Route/ActionRouterTest.php b/tests/Unit/Route/ActionRouterTest.php index a7be34dc541..a003c51afca 100644 --- a/tests/Unit/Route/ActionRouterTest.php +++ b/tests/Unit/Route/ActionRouterTest.php @@ -1,5 +1,16 @@ assertEmpty(TestActionRouter::parseRoute(['unknown-action', 'foo'])); diff --git a/tests/Unit/Route/CalendarRouteTest.php b/tests/Unit/Route/CalendarRouteTest.php index 66c5e5870b9..11968dbc6da 100644 --- a/tests/Unit/Route/CalendarRouteTest.php +++ b/tests/Unit/Route/CalendarRouteTest.php @@ -1,5 +1,16 @@ 'calendar', 'sa' => 'clock']. + */ public function testParsesClockAlias(): void { $this->assertSame( diff --git a/tests/Unit/Route/ProfileRouteTest.php b/tests/Unit/Route/ProfileRouteTest.php index a7f4e35f61d..7db2591c59a 100644 --- a/tests/Unit/Route/ProfileRouteTest.php +++ b/tests/Unit/Route/ProfileRouteTest.php @@ -1,5 +1,16 @@ assertSame('a', Sapi::canonicalPath('a/b/..', false, false)); } + /** + * An absolute path is rooted on the current drive. + * + * Expected: canonicalPath('/a/./b/../c') returns the root, + * then 'a' and 'c'; the root is the separator + * alone on POSIX and the current drive plus the + * separator on Windows. + */ public function testAnAbsolutePathIsRootedOnTheCurrentDrive(): void { // A path that starts at the root names no drive, and on Windows that @@ -50,16 +65,31 @@ public function testAnAbsolutePathIsRootedOnTheCurrentDrive(): void $this->assertSame($drive . $sep . 'a', Sapi::canonicalPath('/a/b/..', false, false)); } + /** + * The unit suite runs under the command line SAPI. + * + * Expected: isCLI() returns true. + */ public function testTheSuiteRunsOnTheCommandLine(): void { $this->assertTrue(Sapi::isCLI()); } + /** + * No memory limit is reported as more than anything will need. + * + * Expected: memoryReturnBytes('-1') returns PHP_INT_MAX. + * Guards: a memory_limit of -1 means unlimited, but it was + * reported as 0, so setMemoryLimit() decided the + * current limit was too small and imposed one. + * Asking for 128M on an unlimited server capped it + * at 128M. + * + * @link https://github.com/SimpleMachines/SMF/commit/a4361e01b Introduced by "Introduce Sapi Class" + * @link https://github.com/SimpleMachines/SMF/pull/9324 + */ public function testNoMemoryLimitIsReportedAsMoreThanAnythingWillNeed(): void { - // A memory_limit of -1 means unlimited. Reporting it as 0 made - // setMemoryLimit() decide the current limit was too small and impose - // one, so asking for 128M on an unlimited server capped it at 128M. $this->assertSame(PHP_INT_MAX, Sapi::memoryReturnBytes('-1')); } @@ -79,11 +109,22 @@ public function testAPlainByteCountKeepsItsLastDigit(): void $this->assertSame(50000000, Sapi::memoryReturnBytes('50000000')); } + /** + * Whitespace around a memory size is ignored. + * + * Expected: memoryReturnBytes(' 64M ') returns 67108864. + */ public function testSurroundingWhitespaceIsIgnored(): void { $this->assertSame(67108864, Sapi::memoryReturnBytes(' 64M ')); } + /** + * A memory size is converted to a number of bytes. + * + * Expected: each case in memorySizeProvider() returns the + * byte count it is paired with. + */ #[DataProvider('memorySizeProvider')] public function testMemoryReturnBytes(string $val, int $expected): void { diff --git a/tests/Unit/SecurityTest.php b/tests/Unit/SecurityTest.php index 9ffbc615c92..a10ef93cdd3 100644 --- a/tests/Unit/SecurityTest.php +++ b/tests/Unit/SecurityTest.php @@ -1,5 +1,16 @@ assertFalse(Security::checkPassword('Tr0ub4dor&3', $member, 0, false, false)); } + /** + * A hash verifies against the password it was made from. + * + * Expected: hashVerifyPassword($password, hashPassword($password)) returns + * true. + */ public function testAHashVerifiesAgainstItsOwnPassword(): void { $hash = Security::hashPassword('correct horse battery staple', self::COST); @@ -46,6 +71,12 @@ public function testAHashVerifiesAgainstItsOwnPassword(): void $this->assertTrue(Security::hashVerifyPassword('correct horse battery staple', $hash)); } + /** + * A hash does not verify against a different password. + * + * Expected: hashVerifyPassword() returns false for the same password with + * different capitalization and for an empty string. + */ public function testAHashDoesNotVerifyAgainstAnythingElse(): void { $hash = Security::hashPassword('correct horse battery staple', self::COST); @@ -54,6 +85,11 @@ public function testAHashDoesNotVerifyAgainstAnythingElse(): void $this->assertFalse(Security::hashVerifyPassword('', $hash)); } + /** + * Hashing the same password twice gives two different hashes. + * + * Expected: two calls to hashPassword('same') return different strings. + */ public function testHashingIsSaltedSoTheSamePasswordHashesDifferently(): void { $this->assertNotSame( @@ -62,17 +98,34 @@ public function testHashingIsSaltedSoTheSamePasswordHashesDifferently(): void ); } + /** + * Password hashes are bcrypt hashes. + * + * Expected: hashPassword() returns a string starting with '$2y$'. + */ public function testHashesAreBcrypt(): void { $this->assertStringStartsWith('$2y$', Security::hashPassword('x', self::COST)); } + /** + * The cost factor given to hashPassword() ends up in the hash. + * + * Expected: hashPassword('x', 4) starts with '$2y$04$' and + * hashPassword('x', 5) starts with '$2y$05$'. + */ public function testTheCostFactorIsHonoured(): void { $this->assertStringStartsWith('$2y$04$', Security::hashPassword('x', 4)); $this->assertStringStartsWith('$2y$05$', Security::hashPassword('x', 5)); } + /** + * Generated passwords are 20 characters long and differ each time. + * + * Expected: generatePassword() returns a string of length 20, and a second + * call returns a different string. + */ public function testGeneratedPasswordsAreDistinctAndNonTrivial(): void { $first = Security::generatePassword(); @@ -81,6 +134,12 @@ public function testGeneratedPasswordsAreDistinctAndNonTrivial(): void $this->assertNotSame($first, Security::generatePassword()); } + /** + * Generated validation codes are 10 characters long and differ each time. + * + * Expected: generateValidationCode() returns a string of length 10, and a + * second call returns a different string. + */ public function testGeneratedValidationCodesAreDistinctAndNonTrivial(): void { $first = Security::generateValidationCode(); diff --git a/tests/Unit/SpoofDetectorTest.php b/tests/Unit/SpoofDetectorTest.php index 360dff2aabf..dc3443331f0 100644 --- a/tests/Unit/SpoofDetectorTest.php +++ b/tests/Unit/SpoofDetectorTest.php @@ -1,5 +1,16 @@ assertTrue(SpoofDetector::checkReservedName('Webmaster')); } + /** + * A name that is not on the list is allowed. + * + * Expected: checkReservedName('Somebody') is false. + */ public function testANameNobodyReservedIsAllowed(): void { Config::$modSettings['reserveNames'] = 'Admin\nWebmaster\nGuest\nroot'; @@ -73,6 +102,11 @@ public function testANameNobodyReservedIsAllowed(): void $this->assertFalse(SpoofDetector::checkReservedName('Somebody')); } + /** + * An empty list reserves no names. + * + * Expected: with an empty list, checkReservedName('Admin') is false. + */ public function testAnEmptyListReservesNothing(): void { Config::$modSettings['reserveNames'] = ''; @@ -81,9 +115,14 @@ public function testAnEmptyListReservesNothing(): void } /** + * A character that merely looks the same as a listed one is still reserved. + * * The point of the class: a name is compared by its skeleton, so a * character that merely looks like the one on the list counts as being on * the list. U+0410 is Cyrillic capital A. + * + * Expected: with 'Admin' on the list, checkReservedName() is true for + * '\u{0410}dmin'. */ public function testACharacterThatMerelyLooksTheSameIsStillReserved(): void { @@ -93,8 +132,13 @@ public function testACharacterThatMerelyLooksTheSameIsStillReserved(): void } /** + * An entity in the name is decoded before it is compared. + * * The admin's list and the name being checked are both decoded first, so * neither side can hide behind an entity. + * + * Expected: with 'Webmaster' on the list, checkReservedName() is true for + * 'Webmaster'. */ public function testAnEntityIsDecodedBeforeComparison(): void { @@ -103,6 +147,12 @@ public function testAnEntityIsDecodedBeforeComparison(): void $this->assertTrue(SpoofDetector::checkReservedName('Webmaster')); } + /** + * The reserveWord setting decides whether part of a name counts. + * + * Expected: with 'Admin' on the list, each case in reserveWordCases() has + * checkReservedName() return the stated boolean. + */ #[DataProvider('reserveWordCases')] public function testReserveWordDecidesWhetherPartOfANameCounts(int $reserve_word, string $name, bool $expected): void { @@ -112,6 +162,12 @@ public function testReserveWordDecidesWhetherPartOfANameCounts(int $reserve_word $this->assertSame($expected, SpoofDetector::checkReservedName($name)); } + /** + * The reserveCase setting decides whether the case has to match. + * + * Expected: with 'Admin' on the list, each case in reserveCaseCases() has + * checkReservedName() return the stated boolean. + */ #[DataProvider('reserveCaseCases')] public function testReserveCaseDecidesWhetherTheCaseHasToMatch(int $reserve_case, string $name, bool $expected): void { diff --git a/tests/Unit/TimeIntervalTest.php b/tests/Unit/TimeIntervalTest.php index 13981cb8322..f3e0f66bd77 100644 --- a/tests/Unit/TimeIntervalTest.php +++ b/tests/Unit/TimeIntervalTest.php @@ -1,5 +1,16 @@ assertSame('P1Y2M3DT4H5M6S', (string) new TimeInterval('P1Y2M3DT4H5M6S')); } + /** + * A time-only duration keeps its time designator. + * + * The point here is the 'T': without it, the 'M' would read as months + * rather than minutes. + * + * Expected: (string) new TimeInterval('PT30M') returns 'PT30M'. + */ public function testTimeOnlyDurationsKeepTheirTimeDesignator(): void { - // The point here is the 'T': without it, the 'M' would read as months - // rather than minutes. $this->assertSame('PT30M', (string) new TimeInterval('PT30M')); } + /** + * A zero unit is dropped when the duration is written back out. + * + * An interval built from a string has no total number of days, and + * stringifying writes only the units that have a value, so a redundant zero + * day count handed in does not come back out. + * + * Expected: (string) new TimeInterval('P0DT30M') returns 'PT30M'. + */ public function testAZeroUnitIsDroppedOnTheWayBackOut(): void { - // This used to be the canonical form: the class populated days for a - // duration naming no years or months, and stringifying wrote the days - // out whether there were any or not. Both ends of that are gone, so a - // duration written the old way now comes back in the short form. $this->assertSame('PT30M', (string) new TimeInterval('P0DT30M')); } + /** + * A TimeInterval can be built from a plain DateInterval. + * + * Expected: createFromDateInterval(new \DateInterval('P1D')) stringifies + * to 'P1D'. + */ public function testItCanBeBuiltFromAPlainDateInterval(): void { $this->assertSame( @@ -54,16 +88,29 @@ public function testItCanBeBuiltFromAPlainDateInterval(): void ); } + /** + * The length of an interval in seconds is measured from a given moment. + * + * Expected: new TimeInterval('PT1H') asked toSeconds() from the epoch + * returns 3600. + */ public function testToSecondsIsMeasuredFromAGivenMoment(): void { $this->assertSame(3600, (new TimeInterval('PT1H'))->toSeconds(new \DateTimeImmutable('@0'))); } + /** + * The seconds in a calendar unit depend on where they are measured from. + * + * A month is not a fixed number of seconds. January is longer than + * February, and asking from a different starting point proves the + * interval is resolved against a real calendar rather than an average. + * + * Expected: new TimeInterval('P1M') asked toSeconds() from 2026-01-01 + * returns 31 * 86400, and from 2026-02-01 returns 28 * 86400. + */ public function testToSecondsDependsOnTheMomentForCalendarUnits(): void { - // A month is not a fixed number of seconds. January is longer than - // February, and asking from a different starting point proves the - // interval is resolved against a real calendar rather than an average. $january = (new TimeInterval('P1M'))->toSeconds(new \DateTimeImmutable('2026-01-01T00:00:00Z')); $february = (new TimeInterval('P1M'))->toSeconds(new \DateTimeImmutable('2026-02-01T00:00:00Z')); @@ -71,23 +118,41 @@ public function testToSecondsDependsOnTheMomentForCalendarUnits(): void $this->assertSame(28 * 86400, $february); } + /** + * toParsable() spells the duration out in words. + * + * Note the singular 'year' against the plural everything else; the + * units are pluralised one at a time based on their own value. + * + * Expected: new TimeInterval('P1Y2M3DT4H5M6S')->toParsable() returns + * '1 year 2 months 3 days 4 hours 5 minutes 6 seconds'. + */ public function testToParsableSpellsTheDurationOut(): void { - // Note the singular 'year' against the plural everything else; the - // units are pluralised one at a time based on their own value. $this->assertSame( '1 year 2 months 3 days 4 hours 5 minutes 6 seconds', (new TimeInterval('P1Y2M3DT4H5M6S'))->toParsable(), ); } + /** + * The interval carries its own values rather than holding another object. + * + * Everything is read from the object itself, as \DateInterval's own + * documented interface does and as every caller passing one to date + * arithmetic does. + * + * Expected: new TimeInterval('P1Y2M3DT4H5M6S') is a \DateInterval whose + * y, m, d, h, i and s properties are 1, 2, 3, 4, 5 and 6. + * Guards: the class kept a \DateInterval of its own and answered for it + * through property hooks, which left the object it actually is + * empty. + * + * @link https://github.com/SimpleMachines/SMF/commit/7b31585f8 Introduced by "Improves SMF\TimeInterval thanks to property hooks" + * @link https://github.com/SimpleMachines/SMF/pull/9499 + */ public function testItCarriesItsOwnValuesRatherThanHoldingThem(): void { - // The class used to keep a \DateInterval of its own and answer for it - // through property hooks, which left the object it actually is empty. - // Everything below reads the object itself, as \DateInterval's own - // documented interface and every caller passing one to date arithmetic - // does. $interval = new TimeInterval('P1Y2M3DT4H5M6S'); $this->assertInstanceOf(\DateInterval::class, $interval); @@ -99,11 +164,21 @@ public function testItCarriesItsOwnValuesRatherThanHoldingThem(): void $this->assertSame(6, $interval->s); } + /** + * Date arithmetic moves the date by the whole interval. + * + * Expected: adding new TimeInterval('P1M2DT3H') to 2026-01-01 00:00 UTC + * gives 2026-02-03T03:00:00+00:00, and subtracting it gives + * 2025-11-28T21:00:00+00:00. + * Guards: \DateTime reads an interval's own properties and cannot see a + * stored one, so adding an interval that said it was a month and + * change moved the date by nothing at all. + * + * @link https://github.com/SimpleMachines/SMF/commit/7b31585f8 Introduced by "Improves SMF\TimeInterval thanks to property hooks" + * @link https://github.com/SimpleMachines/SMF/pull/9499 + */ public function testDateArithmeticMovesTheDateByTheWholeInterval(): void { - // This is what an empty object costs: \DateTime reads the interval's - // own properties and cannot see a stored one, so adding an interval - // that said it was a month and change moved the date by nothing at all. $start = new \DateTimeImmutable('2026-01-01 00:00:00', new \DateTimeZone('UTC')); $this->assertSame( @@ -117,6 +192,12 @@ public function testDateArithmeticMovesTheDateByTheWholeInterval(): void ); } + /** + * A TimeInterval moves a date by the same amount a DateInterval does. + * + * Expected: adding new TimeInterval('P1M2DT3H') to 2026-01-01 00:00 UTC + * formats the same as adding the equivalent \DateInterval. + */ public function testItMovesTheDateByTheSameAmountAPlainDateIntervalDoes(): void { $start = new \DateTimeImmutable('2026-01-01 00:00:00', new \DateTimeZone('UTC')); @@ -127,20 +208,41 @@ public function testItMovesTheDateByTheSameAmountAPlainDateIntervalDoes(): void ); } + /** + * A duration with fractional seconds survives construction. + * + * This is the whole reason the class exists: \DateInterval accepts only the + * integer subset of the ISO 8601 duration spec. + * + * Expected: new TimeInterval('PT1.5S') stringifies to 'PT1.5S' and + * toSeconds() from the epoch returns 1.5. + * Guards: when the fractional unit was the only one given, taking it out + * left 'PT' behind, which \DateInterval rejects, so every + * such duration threw. + * + * @link https://github.com/SimpleMachines/SMF/commit/9627ad8b6 Introduced by "Implements SMF\TimeInterval" + * @link https://github.com/SimpleMachines/SMF/pull/9383 + */ public function testFractionalSecondsSurviveConstruction(): void { - // The whole reason this class exists: \DateInterval accepts only the - // integer subset of the ISO 8601 duration spec. $this->assertSame('PT1.5S', (string) new TimeInterval('PT1.5S')); $this->assertSame(1.5, (new TimeInterval('PT1.5S'))->toSeconds(new \DateTimeImmutable('@0'))); } + /** + * format('%a') falls back to the days field when the total is unknown. + * + * %a is the total number of days, which only an interval produced by diff() + * knows; \DateInterval writes '(unknown)' for any other. When there are no + * years or months in the way, the days field is that total, so it is written + * instead. With a year in it, the days field is not the total and nothing + * can be substituted, so the answer is still the one \DateInterval gives. + * + * Expected: new TimeInterval('P1DT2H')->format('%a') returns '1', and + * new TimeInterval('P1Y')->format('%a') returns '(unknown)'. + */ public function testFormatFallsBackToDaysWhenTheTotalIsUnknown(): void { - // %a is the total number of days, which only an interval produced by - // diff() knows; \DateInterval writes '(unknown)' for any other. When - // there are no years or months in the way, the days field is that - // total, so it is written instead. $this->assertSame('1', (new TimeInterval('P1DT2H'))->format('%a')); // With a year in it, the days field is not the total and nothing can be @@ -156,49 +258,84 @@ public function testFormatFallsBackToDaysWhenTheTotalIsUnknown(): void * now watched here rather than only implied by toParsable() above. */ + /** + * Each unit is localised with its own plural. + * + * Same shape as toParsable(), but through the language file: the units are + * pluralised one at a time, and the result is a sentence rather than a list + * of fields. + * + * Expected: new TimeInterval('P1Y2M3D')->localize() returns + * '1 year, 2 months, and 3 days'. + */ public function testItLocalisesEachUnitWithItsOwnPlural(): void { - // Same shape as toParsable(), but through the language file: the units - // are pluralised one at a time, and the result is a sentence rather - // than a list of fields. $this->assertSame( '1 year, 2 months, and 3 days', (new TimeInterval('P1Y2M3D'))->localize(), ); } + /** + * The order of the units does not depend on how the caller wrote it. + * + * localize() walks its own table of units rather than the array it was + * handed, so the same three units asked for backwards come back in the + * order a reader expects. + * + * Expected: new TimeInterval('P1Y2M3D')->localize(['d', 'y', 'm']) returns + * '1 year, 2 months, and 3 days'. + */ public function testTheUnitOrderDoesNotDependOnHowTheCallerWroteIt(): void { - // localize() walks its own table of units rather than the array it was - // handed, so the same three units asked for backwards come back in the - // order a reader expects. $this->assertSame( '1 year, 2 months, and 3 days', (new TimeInterval('P1Y2M3D'))->localize(['d', 'y', 'm']), ); } + /** + * Asking for the total days falls back when there is no total. + * + * 'a' is the total number of days, which only an interval produced by + * diff() has. On any other one it is substituted with years, months and + * days; without that, nothing would match and the answer would be a flat + * '0 days'. + * + * Expected: new TimeInterval('P1Y')->localize(['a']) returns '1 year', and + * new TimeInterval('P1DT2H')->localize(['a']) returns '1 day'. + */ public function testAskingForTheTotalDaysFallsBackWhenThereIsNoTotal(): void { - // 'a' is the total number of days, which only an interval produced by - // diff() has. On any other one it is substituted with years, months and - // days; without that, nothing would match and the answer would be a - // flat '0 days'. $this->assertSame('1 year', (new TimeInterval('P1Y'))->localize(['a'])); $this->assertSame('1 day', (new TimeInterval('P1DT2H'))->localize(['a'])); } + /** + * An empty result says zero of the smallest unit asked for. + * + * Empty units are dropped so the output is not padded with "0 hours, 0 + * minutes", but dropping all of them would leave nothing to say. + * + * Expected: new TimeInterval('PT0S')->localize(['h', 'i', 's']) returns + * '0 seconds', and localize(['d']) returns '0 days'. + */ public function testItSaysZeroOfTheSmallestUnitItWasAskedFor(): void { - // Empty units are dropped so the output is not padded with "0 hours, - // 0 minutes", but dropping all of them would leave nothing to say. $this->assertSame('0 seconds', (new TimeInterval('PT0S'))->localize(['h', 'i', 's'])); $this->assertSame('0 days', (new TimeInterval('PT0S'))->localize(['d'])); } + /** + * Fractional seconds are folded into the seconds. + * + * 's' and 'f' are one number to a reader, not two. + * + * Expected: new TimeInterval('PT1.5S')->localize(['s', 'f']) returns + * '1.5 seconds'. + */ public function testFractionalSecondsAreFoldedIntoTheSeconds(): void { - // 's' and 'f' are one number to a reader, not two. $this->assertSame('1.5 seconds', (new TimeInterval('PT1.5S'))->localize(['s', 'f'])); } diff --git a/tests/Unit/TimeTest.php b/tests/Unit/TimeTest.php index f4828c53dfc..5f1b33250ce 100644 --- a/tests/Unit/TimeTest.php +++ b/tests/Unit/TimeTest.php @@ -1,5 +1,16 @@ assertFalse(isset(User::$me)); // The constructor only consults User::$me while it has no time zone @@ -57,6 +78,12 @@ public function testItFallsBackToTheForumsDefaultTimeZoneBeforeTheUserIsLoaded() $this->assertSame('Pacific/Auckland', $time->getTimezone()->getName()); } + /** + * An explicitly given time zone is used as it was given. + * + * Expected: new Time('@1785441064', 'Asia/Tokyo') has the time zone + * 'Asia/Tokyo'. + */ public function testAnExplicitTimeZoneIsUsedAsGiven(): void { $this->assertSame( diff --git a/tests/Unit/UrlTest.php b/tests/Unit/UrlTest.php index da226b0cd46..27c70f20595 100644 --- a/tests/Unit/UrlTest.php +++ b/tests/Unit/UrlTest.php @@ -1,5 +1,16 @@ assertSame(8080, $url->port); } + /** + * A component that the URL does not contain is not set. + * + * Expected: new Url('https://example.com') has no query and no fragment, so + * isset() is false for both. + */ public function testMissingComponentsAreNotSet(): void { $url = new Url('https://example.com'); @@ -37,6 +61,12 @@ public function testMissingComponentsAreNotSet(): void $this->assertFalse(isset($url->fragment)); } + /** + * Casting a URL back to a string returns the URL it was built from. + * + * Expected: (string) new Url('https://example.com/a/b?c=d#f') returns that + * same string. + */ public function testCastingBackToStringPreservesTheUrl(): void { $original = 'https://example.com/a/b?c=d#f'; @@ -44,6 +74,12 @@ public function testCastingBackToStringPreservesTheUrl(): void $this->assertSame($original, (string) new Url($original)); } + /** + * toAscii() punycodes an internationalised host. + * + * Expected: toAscii() on 'https://münchen.de/' gives + * 'https://xn--mnchen-3ya.de/'. + */ public function testToAsciiPunycodesAnInternationalisedHost(): void { $this->assertSame( @@ -52,6 +88,12 @@ public function testToAsciiPunycodesAnInternationalisedHost(): void ); } + /** + * toAscii() percent-encodes a non-ASCII path. + * + * Expected: toAscii() on 'https://münchen.de/straße' gives + * 'https://xn--mnchen-3ya.de/stra%C3%9Fe'. + */ public function testToAsciiPercentEncodesANonAsciiPath(): void { $this->assertSame( @@ -60,6 +102,12 @@ public function testToAsciiPercentEncodesANonAsciiPath(): void ); } + /** + * toUtf8() reverses the punycoding of a host. + * + * Expected: toUtf8() on 'https://xn--mnchen-3ya.de/' gives the host + * 'münchen.de'. + */ public function testToUtf8ReversesPunycode(): void { $this->assertSame( @@ -68,13 +116,25 @@ public function testToUtf8ReversesPunycode(): void ); } + /** + * The scheme is reported exactly as it was written. + * + * Schemes are case insensitive, but the class does not normalise them, so a + * caller comparing the property against 'https' must lowercase it first. + * + * Expected: new Url('HTTPS://example.com') has the scheme 'HTTPS'. + */ public function testTheSchemeIsReportedExactlyAsItWasWritten(): void { - // Schemes are case insensitive, but this does not normalise them, so a - // caller comparing against 'https' must lowercase first. $this->assertSame('HTTPS', (new Url('HTTPS://example.com'))->scheme); } + /** + * isScheme() matches the scheme against one name or a list of names. + * + * Expected: https://example.com matches 'https' and ['http', 'https'], and + * does not match 'ftp'. + */ public function testIsSchemeMatchesTheSchemeAsWritten(): void { $this->assertTrue((new Url('https://example.com'))->isScheme('https')); @@ -82,15 +142,40 @@ public function testIsSchemeMatchesTheSchemeAsWritten(): void $this->assertFalse((new Url('https://example.com'))->isScheme('ftp')); } + /** + * isScheme() ignores case on both sides of the comparison. + * + * RFC 3986 section 3.1 makes scheme names case insensitive. The scheme is + * not normalised on parsing, so the comparison has to fold it. + * + * Expected: isScheme('https') is true for HTTPS://example.com, + * isScheme('HTTPS') is true for https://example.com, and + * isScheme(['http', 'https']) is true for HtTp://example.com. + * Guards: isScheme() compared the scheme as written with in_array(), so a + * URL written with an uppercase scheme did not match its own + * name. + * + * @link https://github.com/SimpleMachines/SMF/commit/2029bc6c3 Introduced by "Fix avatar issues" + * @link https://github.com/SimpleMachines/SMF/pull/9325 + */ public function testIsSchemeIgnoresCaseOnBothSides(): void { - // RFC 3986 section 3.1: scheme names are case insensitive. The scheme is - // not normalised on parsing, so the comparison has to fold it. $this->assertTrue((new Url('HTTPS://example.com'))->isScheme('https')); $this->assertTrue((new Url('https://example.com'))->isScheme('HTTPS')); $this->assertTrue((new Url('HtTp://example.com'))->isScheme(['http', 'https'])); } + /** + * A URL with an uppercase http or https scheme is still a website. + * + * Expected: isWebsite() is true for HTTP://example.com and + * HTTPS://example.com, and false for ftp://example.com. + * Guards: isWebsite() relies on isScheme(), so HTTP:// and HTTPS:// URLs + * stopped being recognised as websites. + * + * @link https://github.com/SimpleMachines/SMF/commit/2029bc6c3 Introduced by "Fix avatar issues" + * @link https://github.com/SimpleMachines/SMF/pull/9325 + */ public function testAnUppercaseSchemeIsStillAWebsite(): void { $this->assertTrue((new Url('HTTP://example.com'))->isWebsite()); @@ -98,28 +183,66 @@ public function testAnUppercaseSchemeIsStillAWebsite(): void $this->assertFalse((new Url('ftp://example.com'))->isWebsite()); } + /** + * An uppercase DATA: URI is recognised as a data URI. + * + * User's avatar handling asks isScheme('data') to decide whether the value + * is an inline image or a remote address. + * + * Expected: isScheme('data') is true for 'DATA:image/png;base64,AAAA'. + * Guards: a DATA: URI was not recognised as one, so the avatar handling + * in User treated it as though it were a remote address. + * + * @link https://github.com/SimpleMachines/SMF/commit/2029bc6c3 Introduced by "Fix avatar issues" + * @link https://github.com/SimpleMachines/SMF/pull/9325 + */ public function testAnUppercaseDataUriIsRecognised(): void { - // User's avatar handling asks isScheme('data') to decide whether the - // value is an inline image or a remote address. $this->assertTrue((new Url('DATA:image/png;base64,AAAA'))->isScheme('data')); } + /** + * An IPv6 host is reported with its brackets. + * + * The brackets are part of the authority, not part of the address, so the + * host comes back with them still on it. Anything wanting to treat the host + * as an address has to take them off first, as proxied() does. + * + * Expected: new Url('http://[2001:db8::1]/pic.png') has the host + * '[2001:db8::1]'. + */ public function testAnIPv6HostIsWrittenInBrackets(): void { - // The brackets are part of the authority, not part of the address, so - // the host comes back with them still on it. Anything wanting to treat - // the host as an address has to take them off first, which is what - // proxied() was not doing. $this->assertSame('[2001:db8::1]', (new Url('http://[2001:db8::1]/pic.png'))->host); } + /** + * isValid() accepts absolute URLs and rejects bare words and the empty + * string. + * + * Expected: each case in validityProvider() gives the expected result. + */ #[DataProvider('validityProvider')] public function testValidity(string $input, bool $expected): void { $this->assertSame($expected, (new Url($input))->isValid()); } + /** + * proxied() leaves hosts that are private or reserved addresses alone. + * + * Expected: with the image proxy switched on, each case in + * proxiedProvider() is returned unchanged when its host is a + * private, loopback or documentation address, and is routed + * through https://forum.test-site.com/forum/proxy.php when its + * host is a global address. + * Guards: filter_var() does not accept an address in brackets, so every + * IPv6 literal read as a name rather than an address and went to + * the proxy, private and reserved ranges included. + * + * @link https://github.com/SimpleMachines/SMF/commit/1c9e53bf7 Introduced by "Avoids some unnecessary image proxy usage" + * @link https://github.com/SimpleMachines/SMF/pull/9507 + */ #[DataProvider('proxiedProvider')] public function testProxiedLeavesUnroutableHostsAlone(string $url, bool $expected): void { @@ -137,26 +260,51 @@ public function testProxiedLeavesUnroutableHostsAlone(string $url, bool $expecte }); } + /** + * isFetchSafe() judges an address written out in the URL without a lookup. + * + * Expected: each case in fetchSafeLiteralProvider() is fetch safe only when + * its address is globally routable, in both IPv4 and IPv6. + */ #[DataProvider('fetchSafeLiteralProvider')] public function testIsFetchSafeJudgesLiteralAddresses(string $url, bool $expected): void { $this->assertSame($expected, Url::create($url)->isFetchSafe(['http', 'https'])); } + /** + * isFetchSafe() refuses hosts under reserved top-level domains. + * + * These never reach the resolver: they are refused on the name alone, which + * is the only reason this case can live in a unit test. + * + * Expected: each URL in fetchSafeReservedTldProvider() is not fetch safe. + */ #[DataProvider('fetchSafeReservedTldProvider')] public function testIsFetchSafeRejectsReservedTlds(string $url): void { - // These never reach the resolver: they are refused on the name alone, - // which is the only reason this case can live in a unit test. $this->assertFalse(Url::create($url)->isFetchSafe(['http', 'https'])); } + /** + * isFetchSafe() only accepts the schemes it is given. + * + * Expected: each case in fetchSafeSchemeProvider() is fetch safe only when + * the URL has a scheme in the allowed list and a host. + */ #[DataProvider('fetchSafeSchemeProvider')] public function testIsFetchSafeHonoursTheAllowedSchemes(string $url, array $schemes, bool $expected): void { $this->assertSame($expected, Url::create($url)->isFetchSafe($schemes)); } + /** + * isFetchSafe() with no list of schemes allows any scheme that can be + * fetched. + * + * Expected: isFetchSafe() is true for 'http://93.184.216.34/x' and + * 'ftp://93.184.216.34/x', and false for 'javascript:alert(1)'. + */ public function testIsFetchSafeWithNoSchemesAllowsAnythingWeCanFetch(): void { $this->assertTrue(Url::create('http://93.184.216.34/x')->isFetchSafe()); @@ -164,11 +312,21 @@ public function testIsFetchSafeWithNoSchemesAllowsAnythingWeCanFetch(): void $this->assertFalse(Url::create('javascript:alert(1)')->isFetchSafe()); } + /** + * isFetchSafe() leaves the URL it was asked about untouched. + * + * Expected: after isFetchSafe(['http', 'https']) on + * 'https://93.184.216.34:8443/a/b?c=d#e', the URL still casts to + * that same string and still has the host '93.184.216.34'. + * Guards: the host was swapped for a literal address, so the request went + * out with the wrong SNI name, the wrong Host header, and a + * certificate that could not match. + * + * @link https://github.com/SimpleMachines/SMF/commit/94b25ba27 Introduced by "WebFetchApi::isFetchSafe() → WebFetchApi::makeSafe()" + * @link https://github.com/SimpleMachines/SMF/issues/9533 + */ public function testIsFetchSafeLeavesTheUrlAlone(): void { - // The whole point of the exercise. When the host was swapped for a - // literal address, the request went out with the wrong SNI name, the - // wrong Host header, and a certificate that could not match. $url = new Url('https://93.184.216.34:8443/a/b?c=d#e'); $url->isFetchSafe(['http', 'https']); @@ -177,6 +335,14 @@ public function testIsFetchSafeLeavesTheUrlAlone(): void $this->assertSame('93.184.216.34', $url->host); } + /** + * isFetchSafe() leaves an internationalised host in UTF-8. + * + * Expected: with xn--mnchen-3ya.smf-unit-tests resolving to 93.184.216.34, + * isFetchSafe() is true for + * 'https://münchen.smf-unit-tests/straße', and the URL still + * reads that way afterwards. + */ public function testIsFetchSafeLeavesAnInternationalisedHostInUtf8(): void { $url = new Url('https://münchen.smf-unit-tests/straße'); @@ -188,6 +354,13 @@ public function testIsFetchSafeLeavesAnInternationalisedHostInUtf8(): void $this->assertSame('https://münchen.smf-unit-tests/straße', (string) $url); } + /** + * isFetchSafe() judges a name by every address it resolves to. + * + * Expected: each case in fetchSafeResolutionProvider() is fetch safe only + * when the host resolved to at least one address and every + * address is globally routable. + */ #[DataProvider('fetchSafeResolutionProvider')] public function testIsFetchSafeJudgesWhatTheHostResolvesTo(array $ips, bool $expected): void { @@ -196,6 +369,13 @@ public function testIsFetchSafeJudgesWhatTheHostResolvesTo(array $ips, bool $exp }); } + /** + * resolvesTo() matches any of the addresses the host resolves to. + * + * Expected: with a host resolving to 93.184.216.34 and 93.184.216.35, + * resolvesTo() is true for each of them and false for + * 93.184.216.36. + */ public function testResolvesToMatchesAnyOfTheKnownAddresses(): void { $this->withResolvedHosts(['resolved.smf-unit-tests' => ['93.184.216.34', '93.184.216.35']], function (): void { @@ -207,24 +387,40 @@ public function testResolvesToMatchesAnyOfTheKnownAddresses(): void }); } + /** + * resolvesTo() compares addresses rather than their written form. + * + * SMF\IP puts v6 addresses through inet_ntop(inet_pton()), so the expanded + * and the compressed spelling are the same address by the time they are + * compared. + * + * Expected: for 'http://[2606:4700:4700::1111]/x', resolvesTo() is true for + * the expanded 2606:4700:4700:0000:0000:0000:0000:1111 and false + * for 2606:4700:4700::1112. + */ public function testResolvesToComparesAddressesRatherThanTheirWrittenForm(): void { - // SMF\IP puts v6 addresses through inet_ntop(inet_pton()), so the - // expanded and the compressed spelling are the same address by the - // time we compare them. $url = new Url('http://[2606:4700:4700::1111]/x'); $this->assertTrue($url->resolvesTo(new IP('2606:4700:4700:0000:0000:0000:0000:1111'))); $this->assertFalse($url->resolvesTo(new IP('2606:4700:4700::1112'))); } + /** + * getIPs() finds the addresses of an internationalised host and leaves the + * URL as it was written. + * + * getIPs() punycodes the host, looks that up, and then puts the URL back + * into UTF-8 before returning. Both conversions re-parse the URL, so the + * host is spelled one way going in and another coming out, and the answer + * has to be found under the spelling it was stored with. + * + * Expected: with xn--mnchen-3ya.smf-unit-tests resolving to 93.184.216.34, + * getIPs() on 'https://münchen.smf-unit-tests/x' returns that one + * address, and the URL and its host keep their UTF-8 spelling. + */ public function testGetIPsSurvivesTheRoundTripThroughAscii(): void { - // getIPs() punycodes the host, looks that up, and then puts the URL - // back into UTF-8 before returning. Both conversions re-parse the URL, - // so the host is spelled one way going in and another coming out, and - // reaching for the answer by host name afterwards reaches for a key - // that was never written. $url = new Url('https://münchen.smf-unit-tests/x'); $this->withResolvedHosts(['xn--mnchen-3ya.smf-unit-tests' => ['93.184.216.34']], function () use ($url): void { @@ -239,6 +435,13 @@ public function testGetIPsSurvivesTheRoundTripThroughAscii(): void $this->assertSame('münchen.smf-unit-tests', $url->host); } + /** + * getIPs() returns an empty array for an internationalised host that + * resolves to nothing. + * + * Expected: with no addresses for xn--mnchen-3ya.smf-unit-tests, getIPs() + * on 'https://münchen.smf-unit-tests/x' returns []. + */ public function testGetIPsReturnsAnArrayForAnInternationalisedHostThatResolvesToNothing(): void { $url = new Url('https://münchen.smf-unit-tests/x'); @@ -248,11 +451,19 @@ public function testGetIPsReturnsAnArrayForAnInternationalisedHostThatResolvesTo }); } + /** + * resolvesTo() works on a URL whose host is internationalised. + * + * get_ips_for_url() and url_resolves_to() reach getIPs() and resolvesTo() + * without converting the URL first, so this is the shape the compatibility + * layer hands them. + * + * Expected: with xn--mnchen-3ya.smf-unit-tests resolving to 93.184.216.34, + * resolvesTo() on 'https://münchen.smf-unit-tests/x' is true for + * 93.184.216.34 and false for 10.0.0.1. + */ public function testResolvesToWorksOnAnInternationalisedHost(): void { - // get_ips_for_url() and url_resolves_to() reach getIPs() and - // resolvesTo() without converting the URL first, so this is the shape - // the compatibility layer hands them. $url = new Url('https://münchen.smf-unit-tests/x'); $this->withResolvedHosts(['xn--mnchen-3ya.smf-unit-tests' => ['93.184.216.34']], function () use ($url): void { @@ -261,12 +472,21 @@ public function testResolvesToWorksOnAnInternationalisedHost(): void }); } + /** + * proxied() decides without asking the resolver. + * + * proxied() runs for every image in every post, and only decides whether to + * route through proxy.php, which does its own checking. An empty answer + * from the resolver would read as unsafe to isFetchSafe(), so a URL that + * comes back proxied shows that proxied() never asked. + * + * Expected: with the image proxy on and images.smf-unit-tests seeded as + * resolving to nothing, proxied() on + * 'http://images.smf-unit-tests/pic.png' returns a URL under + * https://forum.test-site.com/forum/proxy.php. + */ public function testProxiedDoesNotConsultTheResolver(): void { - // proxied() runs for every image in every post, and only decides whether - // to route through proxy.php, which does its own checking. An empty - // answer here would read as "unsafe" to isFetchSafe(), so if this comes - // back proxied then proxied() never asked. $this->withResolvedHosts(['images.smf-unit-tests' => []], function (): void { $this->withProxySettings(function (): void { $this->assertStringStartsWith( diff --git a/tests/Unit/UtilsTest.php b/tests/Unit/UtilsTest.php index f9ac52937d8..5739723a20d 100644 --- a/tests/Unit/UtilsTest.php +++ b/tests/Unit/UtilsTest.php @@ -1,5 +1,16 @@ ab(?>c|d))'. + */ public function testBuildRegexSharesACommonPrefix(): void { $this->assertSame('(?>ab(?>c|d))', Utils::buildRegex(['abc', 'abd'])); } + /** + * The regex matches every string it was built from. + * + * Expected: a regex built from strings containing regex metacharacters, + * such as 'a.b', 'a+b' and 'a(b)', matches each of them in full. + */ public function testBuildRegexMatchesEveryStringItWasBuiltFrom(): void { $strings = ['abc', 'abd', 'xyz', 'a.b', 'a+b', 'a(b)']; @@ -31,42 +53,84 @@ public function testBuildRegexMatchesEveryStringItWasBuiltFrom(): void } } + /** + * A trailing character that is special in a regex stays quoted. + * + * Expected: buildRegex(['ab.', 'ab']) matches 'ab.' but not 'abx'. + * Guards: the trailing character that all the strings shared was added to + * the pattern unquoted, so a "." there matched any character and + * the pattern matched things it should not. + * + * @link https://github.com/SimpleMachines/SMF/commit/ea308c3cf Introduced by "Improvements to Utils::buildRegex()" + * @link https://github.com/SimpleMachines/SMF/pull/9318 + */ public function testBuildRegexQuotesTrailingSpecialCharacters(): void { - // A trailing character that is special in a regex must stay quoted, or the - // resulting pattern matches things it should not. $regex = Utils::buildRegex(['ab.', 'ab']); $this->assertMatchesRegularExpression('~^' . $regex . '$~', 'ab.'); $this->assertDoesNotMatchRegularExpression('~^' . $regex . '$~', 'abx'); } + /** + * A list holding a single string yields a regex that matches it. + * + * Expected: buildRegex(['solo']) matches 'solo'. + */ public function testBuildRegexHandlesASingleString(): void { $this->assertMatchesRegularExpression('~^' . Utils::buildRegex(['solo']) . '$~', 'solo'); } + /** + * An HTML entity counts as one character when measuring length. + * + * Expected: entityStrlen('a&b') returns 3 and entityStrlen('déjà') + * returns 4. + */ public function testEntityAwareLengthCountsAnEntityAsOneCharacter(): void { $this->assertSame(3, Utils::entityStrlen('a&b')); $this->assertSame(4, Utils::entityStrlen('déjà')); } + /** + * A substring is cut on entity boundaries, never inside an entity. + * + * Expected: entitySubstr('a&bc', 0, 2) returns 'a&'. + */ public function testEntityAwareSubstrDoesNotSplitAnEntity(): void { $this->assertSame('a&', Utils::entitySubstr('a&bc', 0, 2)); } + /** + * A position is counted in characters, with an entity as one. + * + * Expected: entityStrpos('a&bc', 'b') returns 2. + */ public function testEntityAwareStrposCountsEntitiesAsOne(): void { $this->assertSame(2, Utils::entityStrpos('a&bc', 'b')); } + /** + * Splitting a string into characters keeps each entity in one piece. + * + * Expected: entityStrSplit('a&b') returns ['a', '&', 'b']. + */ public function testEntityAwareSplitKeepsEntitiesWhole(): void { $this->assertSame(['a', '&', 'b'], Utils::entityStrSplit('a&b')); } + /** + * Trimming also removes whitespace written as an entity. + * + * Expected: htmlTrim('   a   ') returns 'a', htmlTrimLeft() + * removes the leading ' ' and htmlTrimRight() the + * trailing one. + */ public function testHtmlTrimRemovesEntityWhitespaceAtBothEnds(): void { $this->assertSame('a', Utils::htmlTrim('   a   ')); @@ -74,6 +138,12 @@ public function testHtmlTrimRemovesEntityWhitespaceAtBothEnds(): void $this->assertSame('a', Utils::htmlTrimRight('a  ')); } + /** + * Truncating drops an entity that does not fit rather than cutting it. + * + * Expected: truncate('abcdefghij', 5) returns 'abcde', and + * truncate('a&bcdef', 5) returns 'a'. + */ public function testTruncateRefusesToCutAnEntityInHalf(): void { $this->assertSame('abcde', Utils::truncate('abcdefghij', 5)); @@ -83,18 +153,37 @@ public function testTruncateRefusesToCutAnEntityInHalf(): void $this->assertSame('a', Utils::truncate('a&bcdef', 5)); } + /** + * An ellipsis is appended only when the string was actually shortened. + * + * Expected: shorten('abcdefghij', 5) returns 'abcde...' and + * shorten('abc', 5) returns 'abc'. + */ public function testShortenAppendsAnEllipsisOnlyWhenItShortens(): void { $this->assertSame('abcde...', Utils::shorten('abcdefghij', 5)); $this->assertSame('abc', Utils::shorten('abc', 5)); } + /** + * Unicode normalization can both compose and decompose. + * + * Expected: normalize() with form 'c' turns 'a' plus a combining acute + * accent into the single character U+00E1, and with form 'd' + * turns U+00E1 into two characters. + */ public function testNormalizeComposesAndDecomposes(): void { $this->assertSame("\u{00E1}", Utils::normalize("a\u{0301}", 'c')); $this->assertSame(2, mb_strlen(Utils::normalize("\u{00E1}", 'd'))); } + /** + * Case conversion handles characters that do not map one to one. + * + * Expected: convertCase('Straße', 'upper') returns 'STRASSE' and + * convertCase('hello world', 'title') returns 'Hello World'. + */ public function testConvertCaseHandlesCharactersWithNoSimpleMapping(): void { // Uppercasing the sharp s expands it to two characters, which a naive @@ -103,12 +192,22 @@ public function testConvertCaseHandlesCharactersWithNoSimpleMapping(): void $this->assertSame('Hello World', Utils::convertCase('hello world', 'title')); } + /** + * A digraph is titlecased to its dedicated titlecase character. + * + * Expected: convertCase() of U+01F3 with 'title' returns U+01F2. + */ public function testConvertCaseTitlecasesDigraphsToTheirTitleForm(): void { // U+01F3 dz titlecases to U+01F2 Dz, which is neither upper nor lower. $this->assertSame("\u{01F2}", Utils::convertCase("\u{01F3}", 'title')); } + /** + * Case folding makes strings that differ only in case compare equal. + * + * Expected: convertCase('ÄÖÜ', 'fold') equals convertCase('äöü', 'fold'). + */ public function testConvertCaseFoldsForCaseInsensitiveComparison(): void { $this->assertSame( @@ -117,6 +216,12 @@ public function testConvertCaseFoldsForCaseInsensitiveComparison(): void ); } + /** + * Directional overrides are replaced at level 1 and kept at level 0. + * + * Expected: sanitizeChars() of a string holding U+202E returns it + * unchanged at level 0 and with U+FFFD in its place at level 1. + */ public function testSanitizeCharsReplacesDirectionalOverridesAtLevelOne(): void { // A right-to-left override can be used to disguise a file name or link. @@ -124,23 +229,47 @@ public function testSanitizeCharsReplacesDirectionalOverridesAtLevelOne(): void $this->assertSame("a\u{FFFD}b", Utils::sanitizeChars("a\u{202E}b", 1)); } + /** + * Unusual whitespace characters are replaced by a plain space. + * + * Expected: normalizeSpaces("a\u{00A0}b", true, true) returns 'a b'. + */ public function testNormalizeSpacesCollapsesExoticWhitespace(): void { $this->assertSame('a b', Utils::normalizeSpaces("a\u{00A0}b", true, true)); } + /** + * Entities for control characters are replaced and others kept. + * + * Expected: sanitizeEntities('') returns '�' and + * sanitizeEntities('A') returns 'A'. + */ public function testSanitizeEntitiesReplacesEntitiesForControlCharacters(): void { $this->assertSame('�', Utils::sanitizeEntities('')); $this->assertSame('A', Utils::sanitizeEntities('A')); } + /** + * Single quotes are only escaped when ENT_QUOTES is asked for. + * + * Expected: htmlspecialchars('a"b\'c<>&') returns + * 'a"b\'c<>&', and with ENT_QUOTES the single + * quote becomes '''. + */ public function testHtmlspecialcharsLeavesSingleQuotesAloneByDefault(): void { $this->assertSame('a"b\'c<>&', Utils::htmlspecialchars('a"b\'c<>&')); $this->assertSame('a"b'c', Utils::htmlspecialchars('a"b\'c', ENT_QUOTES)); } + /** + * Decoding undoes what htmlspecialchars() did. + * + * Expected: htmlspecialcharsDecode(htmlspecialchars($x, ENT_QUOTES)) + * returns $x. + */ public function testHtmlspecialcharsDecodeRoundTrips(): void { $original = 'a"b&d'; @@ -151,18 +280,37 @@ public function testHtmlspecialcharsDecodeRoundTrips(): void ); } + /** + * JSON encoding and decoding are inverses of each other. + * + * Expected: jsonEncode(['a' => 1]) returns '{"a":1}' and + * jsonDecode('{"a":1}', true) returns ['a' => 1]. + */ public function testJsonRoundTrips(): void { $this->assertSame('{"a":1}', Utils::jsonEncode(['a' => 1])); $this->assertSame(['a' => 1], Utils::jsonDecode('{"a":1}', true)); } + /** + * Each input is measured in characters, with an entity as one. + * + * Expected: each case in entityLengthProvider() has the length that the + * provider gives, as returned by entityStrlen(). + */ #[DataProvider('entityLengthProvider')] public function testEntityStrlenAcrossInputs(string $input, int $expected): void { $this->assertSame($expected, Utils::entityStrlen($input)); } + /** + * A version string is standardized to dot-separated parts. + * + * Expected: each case in versionStringProvider() is turned into the + * standardized form that the provider gives, as returned by + * standardizeVersionString(). + */ #[DataProvider('versionStringProvider')] public function testStandardizeVersionString(string $input, string $expected): void { diff --git a/tests/Unit/UuidTest.php b/tests/Unit/UuidTest.php index 45a9653aa51..1241d4db279 100644 --- a/tests/Unit/UuidTest.php +++ b/tests/Unit/UuidTest.php @@ -1,5 +1,16 @@ assertMatchesRegularExpression( @@ -30,17 +46,33 @@ public function testGeneratedUuidsLookLikeUuids(): void ); } + /** + * Two generated UUIDs are not the same. + * + * Expected: the strings of two Uuid::create(4) calls differ. + */ public function testGeneratedUuidsAreDistinct(): void { $this->assertNotSame((string) Uuid::create(4), (string) Uuid::create(4)); } + /** + * Generated UUIDs always report the RFC 4122 variant. + * + * Expected: getVariant() returns 1 for versions 4 and 7. + */ public function testTheVariantIsAlwaysTheRfcOne(): void { $this->assertSame(1, Uuid::create(4)->getVariant()); $this->assertSame(1, Uuid::create(7)->getVariant()); } + /** + * The nil UUID is read back unchanged and reports version zero. + * + * Expected: Uuid::createFromString() of the all-zero UUID casts back to + * the same string, and getVersion() returns 0. + */ public function testTheNilUuidRoundTripsAndReportsVersionZero(): void { $uuid = Uuid::createFromString(self::NIL); @@ -49,16 +81,32 @@ public function testTheNilUuidRoundTripsAndReportsVersionZero(): void $this->assertSame(0, $uuid->getVersion()); } + /** + * The binary form of a UUID is sixteen bytes long. + * + * Expected: strlen(Uuid::create(4)->getBinary()) is 16. + */ public function testTheBinaryFormIsSixteenBytes(): void { $this->assertSame(16, \strlen(Uuid::create(4)->getBinary())); } + /** + * The short form of a UUID is twenty-two characters long. + * + * Expected: strlen(Uuid::create(4)->getShortForm()) is 22. + */ public function testTheShortFormIsTwentyTwoCharacters(): void { $this->assertSame(22, \strlen(Uuid::create(4)->getShortForm())); } + /** + * The lowercase short form is twenty-six lowercase base 32 characters. + * + * Expected: getShortForm(true) is 26 characters long and matches + * ^[0-9a-z]+$. + */ public function testShortFormLowercaseIsTwentySixCharactersAndOnlyLowercaseBase32Chars(): void { $short = Uuid::create(4)->getShortForm(true); @@ -68,6 +116,11 @@ public function testShortFormLowercaseIsTwentySixCharactersAndOnlyLowercaseBase3 $this->assertMatchesRegularExpression('~^[0-9a-z]+$~', $short); } + /** + * Compressing a UUID and expanding the result gives the UUID back. + * + * Expected: Uuid::expand(Uuid::compress($uuid)) returns $uuid. + */ public function testCompressAndExpandRoundTrip(): void { $uuid = (string) Uuid::create(4); @@ -75,6 +128,12 @@ public function testCompressAndExpandRoundTrip(): void $this->assertSame($uuid, Uuid::expand(Uuid::compress($uuid))); } + /** + * Strict parsing refuses a string that is not a UUID. + * + * Expected: Uuid::createFromString('not-a-uuid', true) throws a + * ValueError. + */ public function testStrictParsingRejectsRubbish(): void { $this->expectException(\ValueError::class); @@ -82,10 +141,17 @@ public function testStrictParsingRejectsRubbish(): void Uuid::createFromString('not-a-uuid', true); } + /** + * A version 7 UUID created later sorts after one created earlier. + * + * Version 7 puts a millisecond timestamp in the high bits, so the string + * form is monotonic. That is the whole point of using it for keys. + * + * Expected: strcmp() of the string of a version 7 UUID and one created + * a millisecond later is negative. + */ public function testVersionSevenUuidsSortByCreationOrder(): void { - // Version 7 puts a millisecond timestamp in the high bits, so the string - // form is monotonic. That is the whole point of using it for keys. $first = (string) Uuid::create(7); // Sleeping for a millisecond is not the same as one having passed. @@ -106,12 +172,25 @@ public function testVersionSevenUuidsSortByCreationOrder(): void $this->assertLessThan(0, strcmp($first, $second)); } + /** + * Uuid::create() produces a UUID of the version it was asked for. + * + * Expected: for each version in versionProvider(), getVersion() returns + * that version. + */ #[DataProvider('versionProvider')] public function testCreateProducesTheRequestedVersion(int $version): void { $this->assertSame($version, Uuid::create($version)->getVersion()); } + /** + * A UUID survives compression to binary, base 64 and base 32. + * + * Expected: for each version in versionProvider(), compress() gives 16, + * 22 and 26 characters respectively, and expand() of each + * returns the original UUID. + */ #[DataProvider('versionProvider')] public function testBinaryBase64AndBase32Roundtrips(int $version): void { @@ -133,6 +212,12 @@ public function testBinaryBase64AndBase32Roundtrips(int $version): void $this->assertSame(strtolower($uuid), strtolower(Uuid::expand($b32))); } + /** + * createFromString() reads the binary, base 64 and base 32 forms. + * + * Expected: for each version in versionProvider(), Uuid::createFromString() + * of each compressed form casts back to the original UUID. + */ #[DataProvider('versionProvider')] public function testCreateFromStringAcceptsBinaryBase64AndBase32(int $version): void { @@ -148,6 +233,13 @@ public function testCreateFromStringAcceptsBinaryBase64AndBase32(int $version): $this->assertSame(strtolower($uuid), strtolower((string) Uuid::createFromString($b32))); } + /** + * A UUID parsed from its canonical string has the matching binary form. + * + * Expected: for each version in versionProvider(), + * Uuid::createFromString($uuid)->getBinary() equals + * Uuid::compress($uuid, Uuid::COMPRESS_BINARY). + */ #[DataProvider('versionProvider')] public function testParsedCanonicalHasConsistentBinaryRepresentation(int $version): void { diff --git a/tests/Unit/WebFetchResultTest.php b/tests/Unit/WebFetchResultTest.php index d9670488e05..66266966561 100644 --- a/tests/Unit/WebFetchResultTest.php +++ b/tests/Unit/WebFetchResultTest.php @@ -1,5 +1,16 @@ assertIsString($result->create_xml($path)); } + /** + * Every accessor agrees about a path that ends at an attribute. + * + * Expected: for each case in provideAttributeCases(), the path exists, + * path() and fetch() return the attribute value, count() is 0, + * to_array() and set() are empty arrays, and create_xml() returns + * false. + */ #[DataProvider('provideAttributeCases')] public function testAttribute(string $path, string $name): void { @@ -64,6 +91,15 @@ public function testAttribute(string $path, string $name): void $this->assertFalse($result->create_xml($path)); } + /** + * to_array(), name(), fetch(), count() and set() read the sample document. + * + * Expected: to_array('characters[0]') and to_array() give the last + * character's name and weapon ('Sauron' and 'Evil Eye'), name() + * is '', fetch('characters[0]/character[1]/@film') is 'LOTR', + * count('characters') is 1, and set('characters[0]/character') + * has 2 elements. + */ public function testBasicOperations(): void { $result = new XmlArray(self::XML); @@ -101,6 +137,13 @@ public function testBasicOperations(): void $this->assertCount(2, $result->set('characters[0]/character')); } + /** + * create_xml() reproduces the document it was built from. + * + * Expected: create_xml() on the sample document equals the sample document + * once the CDATA markers and all whitespace are removed from + * both. + */ public function testCreateXml(): void { $result = new XmlArray(self::XML); @@ -120,6 +163,12 @@ public function testCreateXml(): void ); } + /** + * A missing path raises a notice when the error level asks for one. + * + * Expected: with an error handler that throws and new XmlArray(XML, true, + * E_ALL), to_array('error') throws an ErrorException. + */ public function testMissingPathRaisesNotice(): void { set_error_handler( @@ -138,6 +187,13 @@ static function (int $errno, string $errstr, string $errfile, int $errline): nev } } + /** + * A missing path is silent when the error level is 0. + * + * Expected: for the path 'error', exists(), path(), fetch() and + * create_xml() return false, count() is 0, and to_array() and + * set() return empty arrays. + */ public function testSilentMissingPath(): void { $result = new XmlArray(self::XML, true, 0); diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 17bafa5feb4..847709634dc 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -1,5 +1,16 @@