From f5766871bc46696fcf36d00f813eec886aafee80 Mon Sep 17 00:00:00 2001 From: albertlast Date: Mon, 5 Oct 2026 06:13:25 +0200 Subject: [PATCH 1/2] Adds the license header to the existing test files Every PHP file under tests/ other than the index.php stubs now opens with the same license header as Sources/, as AGENTS.md asks. The header is copied byte for byte from Sources/Sapi.php. check-smf-license.php skips ./tests/, so nothing checked this before. Co-Authored-By: Claude Opus 5.5 Signed-off-by: albertlast --- tests/AutoReview/ComposerFileTest.php | 11 +++++++++++ tests/AutoReview/ForumTest.php | 11 +++++++++++ tests/AutoReview/LicenseTest.php | 11 +++++++++++ tests/Integration/GroupsTest.php | 11 +++++++++++ tests/Integration/HarnessTest.php | 11 +++++++++++ tests/Integration/Http/CronTest.php | 11 +++++++++++ tests/Integration/Http/GuestPagesTest.php | 11 +++++++++++ tests/Integration/Http/HttpTestCase.php | 11 +++++++++++ tests/Integration/Http/LoginTest.php | 11 +++++++++++ tests/Integration/Http/MemberlistSearchTest.php | 11 +++++++++++ tests/Integration/Http/PostingTest.php | 11 +++++++++++ tests/Integration/Http/ProfileTrackingTest.php | 11 +++++++++++ tests/Integration/Http/RecentTest.php | 11 +++++++++++ tests/Integration/Http/ReplyNotificationTest.php | 11 +++++++++++ tests/Integration/Http/StatsStartersTest.php | 11 +++++++++++ tests/Integration/Http/StatsTest.php | 11 +++++++++++ tests/Integration/Http/ThemeCopyListTest.php | 11 +++++++++++ tests/Integration/Http/ThemeCopyTest.php | 11 +++++++++++ tests/Integration/Http/TopicSplitTest.php | 11 +++++++++++ tests/Integration/Http/UnsubscribeLinkTest.php | 11 +++++++++++ tests/Integration/Installation.php | 11 +++++++++++ tests/Integration/IntegrationTestCase.php | 11 +++++++++++ tests/Integration/ModLogTest.php | 11 +++++++++++ tests/Integration/ModSettingsTest.php | 11 +++++++++++ tests/Integration/NotifyTest.php | 11 +++++++++++ tests/Integration/PermissionProfileTest.php | 11 +++++++++++ tests/Integration/SchemaTest.php | 11 +++++++++++ tests/Support/HttpClient.php | 11 +++++++++++ tests/Support/HttpResponse.php | 11 +++++++++++ tests/TestCase/AbstractRouteTestCase.php | 11 +++++++++++ tests/TestCase/SlugRouteTestTrait.php | 11 +++++++++++ tests/Unit/ActionTraitTest.php | 11 +++++++++++ tests/Unit/AvatarFallbackTest.php | 11 +++++++++++ tests/Unit/AvatarGalleryDirectoryTest.php | 11 +++++++++++ tests/Unit/AvatarTest.php | 11 +++++++++++ tests/Unit/BoardsCountPostsBackcompatTest.php | 11 +++++++++++ tests/Unit/CacheApiTest.php | 11 +++++++++++ tests/Unit/ClassNameSyntaxTest.php | 11 +++++++++++ tests/Unit/ClassReferenceTest.php | 11 +++++++++++ tests/Unit/CreatePostNotifyTest.php | 11 +++++++++++ tests/Unit/ForumTest.php | 11 +++++++++++ tests/Unit/IPTest.php | 11 +++++++++++ tests/Unit/Infrastructure/ContainerFactoryTest.php | 11 +++++++++++ tests/Unit/Infrastructure/ServicesTest.php | 11 +++++++++++ tests/Unit/LangTest.php | 11 +++++++++++ tests/Unit/MarkReadRouteTest.php | 11 +++++++++++ tests/Unit/MessageFormatterTest.php | 11 +++++++++++ tests/Unit/PageIndexTest.php | 11 +++++++++++ tests/Unit/PunycodeTest.php | 11 +++++++++++ tests/Unit/QueryCaseFoldingTest.php | 11 +++++++++++ tests/Unit/QueryStringRouteTest.php | 11 +++++++++++ tests/Unit/Route/ActionRouterTest.php | 11 +++++++++++ tests/Unit/Route/CalendarRouteTest.php | 11 +++++++++++ tests/Unit/Route/ProfileRouteTest.php | 11 +++++++++++ tests/Unit/SecurityTest.php | 11 +++++++++++ tests/Unit/SpoofDetectorTest.php | 11 +++++++++++ tests/Unit/TimeIntervalTest.php | 11 +++++++++++ tests/Unit/TimeTest.php | 11 +++++++++++ tests/Unit/UrlTest.php | 11 +++++++++++ tests/Unit/UtilsTest.php | 11 +++++++++++ tests/Unit/UuidTest.php | 11 +++++++++++ tests/Unit/WebFetchResultTest.php | 11 +++++++++++ tests/Unit/XmlArrayTest.php | 11 +++++++++++ tests/bootstrap.php | 11 +++++++++++ tests/fixtures/DatabaseConnection.php | 11 +++++++++++ tests/fixtures/SimpleAction.php | 11 +++++++++++ tests/fixtures/SimpleActionExecuted.php | 11 +++++++++++ tests/fixtures/SimpleDependencyAwareAction.php | 11 +++++++++++ tests/fixtures/SimpleForum.php | 11 +++++++++++ tests/fixtures/UserRepository.php | 11 +++++++++++ tests/fixtures/integrationhooks.php | 11 +++++++++++ tests/fixtures/languages/de_DE/TestStrings.php | 11 +++++++++++ tests/fixtures/languages/en_US/TestStrings.php | 11 +++++++++++ tests/fixtures/languages/es_ES/TestStrings.php | 11 +++++++++++ 74 files changed, 814 insertions(+) diff --git a/tests/AutoReview/ComposerFileTest.php b/tests/AutoReview/ComposerFileTest.php index 31e76a2f240..b69bdb8750d 100644 --- a/tests/AutoReview/ComposerFileTest.php +++ b/tests/AutoReview/ComposerFileTest.php @@ -1,5 +1,16 @@ Date: Mon, 5 Oct 2026 06:25:38 +0200 Subject: [PATCH 2/2] Documents every existing test method in the agreed shape Each test method now has the docblock AGENTS.md describes: a summary, Expected: with the call and the result its assertions check, and for a regression test, Guards: with what went wrong, a @link to the commit that introduced the bug and a @link to the issue or pull request that fixed it. The Introduced by line is left out where the faulty code is older than the history (it is already in the 2012 import) or where the commit could not be confirmed from its diff. A comment at the top of a test body that explained why the test exists has moved into the docblock; nothing else in the bodies has changed. ThemeCopyTest is left alone, because #9728 rewrites it. Co-Authored-By: Claude Opus 5.5 Signed-off-by: albertlast --- tests/AutoReview/ComposerFileTest.php | 13 + tests/AutoReview/ForumTest.php | 21 +- tests/AutoReview/LicenseTest.php | 17 +- tests/Integration/GroupsTest.php | 23 +- tests/Integration/HarnessTest.php | 60 +++- tests/Integration/Http/CronTest.php | 19 +- tests/Integration/Http/GuestPagesTest.php | 33 ++- tests/Integration/Http/LoginTest.php | 29 +- .../Integration/Http/MemberlistSearchTest.php | 17 +- tests/Integration/Http/PostingTest.php | 15 +- .../Integration/Http/ProfileTrackingTest.php | 15 +- tests/Integration/Http/RecentTest.php | 19 +- .../Http/ReplyNotificationTest.php | 19 +- tests/Integration/Http/StatsStartersTest.php | 21 +- tests/Integration/Http/StatsTest.php | 19 +- tests/Integration/Http/ThemeCopyListTest.php | 15 +- tests/Integration/Http/TopicSplitTest.php | 26 +- .../Integration/Http/UnsubscribeLinkTest.php | 20 +- tests/Integration/ModLogTest.php | 34 ++- tests/Integration/ModSettingsTest.php | 56 +++- tests/Integration/NotifyTest.php | 19 +- tests/Integration/PermissionProfileTest.php | 20 +- tests/Integration/SchemaTest.php | 35 ++- tests/TestCase/AbstractRouteTestCase.php | 18 ++ tests/TestCase/SlugRouteTestTrait.php | 20 ++ tests/Unit/ActionTraitTest.php | 52 +++- tests/Unit/AvatarFallbackTest.php | 34 ++- tests/Unit/AvatarGalleryDirectoryTest.php | 40 ++- tests/Unit/AvatarTest.php | 46 ++- tests/Unit/BoardsCountPostsBackcompatTest.php | 42 ++- tests/Unit/CacheApiTest.php | 117 +++++++- tests/Unit/ClassNameSyntaxTest.php | 11 + tests/Unit/ClassReferenceTest.php | 18 +- tests/Unit/CreatePostNotifyTest.php | 13 + tests/Unit/ForumTest.php | 27 +- tests/Unit/IPTest.php | 103 ++++++- .../Infrastructure/ContainerFactoryTest.php | 6 + tests/Unit/Infrastructure/ServicesTest.php | 12 + tests/Unit/LangTest.php | 115 +++++++- tests/Unit/MarkReadRouteTest.php | 53 +++- tests/Unit/MessageFormatterTest.php | 71 ++++- tests/Unit/PageIndexTest.php | 137 ++++++++- tests/Unit/PunycodeTest.php | 11 + tests/Unit/QueryCaseFoldingTest.php | 21 +- tests/Unit/QueryStringRouteTest.php | 80 +++++- tests/Unit/Route/ActionRouterTest.php | 17 ++ tests/Unit/Route/CalendarRouteTest.php | 6 + tests/Unit/SapiTest.php | 47 ++- tests/Unit/SecurityTest.php | 54 +++- tests/Unit/SpoofDetectorTest.php | 51 +++- tests/Unit/TimeIntervalTest.php | 202 ++++++++++--- tests/Unit/TimeTest.php | 26 +- tests/Unit/UrlTest.php | 269 ++++++++++++++++-- tests/Unit/UtilsTest.php | 141 ++++++++- tests/Unit/UuidTest.php | 85 +++++- tests/Unit/WebFetchResultTest.php | 17 +- tests/Unit/XmlArrayTest.php | 45 +++ 57 files changed, 2269 insertions(+), 303 deletions(-) diff --git a/tests/AutoReview/ComposerFileTest.php b/tests/AutoReview/ComposerFileTest.php index b69bdb8750d..aa862f88fa4 100644 --- a/tests/AutoReview/ComposerFileTest.php +++ b/tests/AutoReview/ComposerFileTest.php @@ -25,6 +25,13 @@ final class ComposerFileTest extends TestCase * Public methods ****************/ + /** + * Every Composer script has a description. + * + * Expected: apart from Composer's own event names, the keys of scripts in + * composer.json and the keys of scripts-descriptions are the same + * set. + */ public function testScriptsHaveDescriptions(): void { $composer_json = $this->readComposerJson(); @@ -78,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 d2290217173..30934ad776b 100644 --- a/tests/AutoReview/ForumTest.php +++ b/tests/AutoReview/ForumTest.php @@ -28,10 +28,14 @@ class ForumTest extends TestCase ****************/ /** - * Tests that every action definition has the expected structure. + * Every action definition has the expected structure. * * Each action must contain a source file and a handler. The handler must * either be an ActionInterface implementation or a callable. + * + * Expected: each entry of Forum::$actions is a two-element list whose first + * element is a string and whose second is an ActionInterface + * class name or a callable. */ public function testActionDefinitions(): void { @@ -51,7 +55,10 @@ public function testActionDefinitions(): void } /** - * Tests that all renamed actions point to actions that exist. + * All renamed actions point to actions that exist. + * + * Expected: array_diff(Forum::$renamed_actions, + * array_keys(Forum::$actions)) is empty. */ public function testRenamedActionsExist(): void { @@ -63,7 +70,10 @@ public function testRenamedActionsExist(): void } /** - * Tests that all actions excluded from logging exist. + * All actions excluded from logging exist. + * + * Expected: array_diff_key(Forum::$unlogged_actions, Forum::$actions) is + * empty. */ public function testUnloggedActionsExist(): void { @@ -75,7 +85,10 @@ public function testUnloggedActionsExist(): void } /** - * Tests that all actions accessible to guests exist. + * All actions accessible to guests exist. + * + * Expected: array_diff(Forum::$guest_access_actions, + * array_keys(Forum::$actions)) is empty. */ public function testGuestAccessActionsExist(): void { diff --git a/tests/AutoReview/LicenseTest.php b/tests/AutoReview/LicenseTest.php index 6263d08542c..a5c7d14d84a 100644 --- a/tests/AutoReview/LicenseTest.php +++ b/tests/AutoReview/LicenseTest.php @@ -63,8 +63,11 @@ class LicenseTest extends TestCase ****************/ /** - * Tests that every PHP file that is not explicitly ignored has a valid - * SMF license header. + * Every PHP file that is not explicitly ignored has a valid SMF license + * header. + * + * Expected: the contents of each file from getPhpFiles() match + * getLicensePattern(). */ public function testFilesHaveValidLicense(): void { @@ -94,8 +97,14 @@ public function testFilesHaveValidLicense(): void } /** - * Tests that files whose headers contain the current software version and - * year use the values defined by index.php. + * Files whose headers carry the software version and year use the current + * ones. + * + * The current values are the ones defined by index.php. + * + * Expected: each file matching VERSION_AND_YEAR_FILES matches + * getLicensePattern() with SMF_SOFTWARE_YEAR and SMF_VERSION from + * index.php. */ public function testFilesHaveCurrentVersionAndYear(): void { diff --git a/tests/Integration/GroupsTest.php b/tests/Integration/GroupsTest.php index b5d239df5c8..f8bb776d9e2 100644 --- a/tests/Integration/GroupsTest.php +++ b/tests/Integration/GroupsTest.php @@ -31,10 +31,17 @@ class GroupsTest extends IntegrationTestCase ****************/ /** - * The query had no ORDER BY, so the database chose the order, and with a - * limit it chose which members were listed at all. PostgreSQL returns a - * row it has just rewritten after the others, which MySQL does not, so on - * PostgreSQL the list changed as members' rows were updated. + * Members of a membergroup are listed in the order they joined. + * + * Expected: listMembergroupMembers_Href($members, 1) lists two new + * administrators in id order, even after the earlier one's row has + * been rewritten. + * Guards: the query had no ORDER BY, so the database chose the order. A + * row that PostgreSQL has just rewritten is returned after the + * others, so there the list changed as members' rows were + * updated. + * + * @link https://github.com/SimpleMachines/SMF/pull/9721 */ public function testMembersAreListedInTheOrderTheyJoined(): void { @@ -58,6 +65,14 @@ public function testMembersAreListedInTheOrderTheyJoined(): void /** * With a limit, the members left out are the ones who joined last. + * + * Expected: listMembergroupMembers_Href($members, 1, $count - 1) returns true + * for "more" and lists every administrator but the last, in id + * order. + * Guards: with no ORDER BY and a limit, the database chose which members + * were listed at all. + * + * @link https://github.com/SimpleMachines/SMF/pull/9721 */ public function testALimitKeepsTheMembersWhoJoinedFirst(): void { diff --git a/tests/Integration/HarnessTest.php b/tests/Integration/HarnessTest.php index 3440ad2faa8..dfa11543a3d 100644 --- a/tests/Integration/HarnessTest.php +++ b/tests/Integration/HarnessTest.php @@ -45,12 +45,22 @@ class HarnessTest extends IntegrationTestCase * Public methods ****************/ + /** + * The forum the suite runs against is installed, at the current version. + * + * Expected: modSettings['smfVersion'] is not empty and equals SMF_VERSION. + */ public function testTheForumIsInstalled(): void { $this->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( @@ -61,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 { @@ -77,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 { @@ -86,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(); @@ -113,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 4994e47064e..d81319d2bda 100644 --- a/tests/Integration/Http/CronTest.php +++ b/tests/Integration/Http/CronTest.php @@ -51,13 +51,20 @@ class CronTest extends HttpTestCase ****************/ /** - * cron.php can report an error, and carries on when it does. + * An error in a task is logged by cron.php, and the task is cleared. * - * Only Forum gave ErrorHandler its service, so anything cron.php tried to - * log threw a LogicException instead. TaskRunner's exception handler then - * tried to log that, and threw again. The request died with the task still - * claimed, so it ran again five minutes later: a notification email whose - * task logged so much as a warning was sent again every five minutes. + * Expected: requesting cron.php with a queued task whose class does not + * exist returns no fatal error, removes the task from + * background_tasks and logs a message naming the missing class. + * Guards: only Forum gave ErrorHandler its service, so anything cron.php + * tried to log threw a LogicException instead. TaskRunner's + * exception handler then tried to log that, and threw again. The + * request died with the task still claimed, so it ran again five + * minutes later: a notification email whose task logged so much + * as a warning was sent again every five minutes. + * + * @link https://github.com/SimpleMachines/SMF/commit/058a81c27 Introduced by "wire the error handler service up to the new service implementation" + * @link https://github.com/SimpleMachines/SMF/pull/9737 */ public function testAnErrorInATaskIsLoggedAndTheTaskIsCleared(): void { diff --git a/tests/Integration/Http/GuestPagesTest.php b/tests/Integration/Http/GuestPagesTest.php index 928192fee63..49f804fe0cd 100644 --- a/tests/Integration/Http/GuestPagesTest.php +++ b/tests/Integration/Http/GuestPagesTest.php @@ -39,6 +39,12 @@ class GuestPagesTest extends HttpTestCase * Public methods ****************/ + /** + * Each page a guest can reach loads and logs nothing. + * + * Expected: each case in guestPages() returns a page that looks like a + * forum page, and leaves nothing in the error log. + */ #[DataProvider('guestPages')] public function testThePageLoadsAndLogsNothing(string $path, string $name): void { @@ -48,6 +54,11 @@ public function testThePageLoadsAndLogsNothing(string $path, string $name): void $this->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(''); @@ -62,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 { @@ -86,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 { @@ -102,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/LoginTest.php b/tests/Integration/Http/LoginTest.php index 416bf31e81b..d878789b109 100644 --- a/tests/Integration/Http/LoginTest.php +++ b/tests/Integration/Http/LoginTest.php @@ -35,6 +35,13 @@ class LoginTest extends HttpTestCase * Public methods ****************/ + /** + * The administrator can sign in over HTTP. + * + * Expected: submitting the login form with the administrator's credentials + * leaves the session signed in, sets the forum cookie, and logs + * nothing. + */ public function testTheAdministratorCanSignIn(): void { $response = $this->signInAsAdmin(); @@ -54,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'); @@ -67,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 { @@ -83,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 7431d3acce9..7ee39f9f224 100644 --- a/tests/Integration/Http/MemberlistSearchTest.php +++ b/tests/Integration/Http/MemberlistSearchTest.php @@ -31,10 +31,19 @@ class MemberlistSearchTest extends HttpTestCase /** * A search finds a member whatever case it is typed in. * - * On a case sensitive database - PostgreSQL - Memberlist::search() folds the - * columns it searches to lower case, and compared them with the search as - * typed. So any search with a capital letter in it matched nobody. MySQL - * compares without regard to case anyway, and passes either way. + * MySQL compares without regard to case anyway, so on MySQL this passes + * with or without the bug. + * + * Expected: each case in spellings() searches the member list for the + * administrator's name, in capitals or capitalised, and finds + * the administrator among the results, with nothing logged. + * Guards: on a case sensitive database - PostgreSQL - Memberlist::search() + * folded the columns it searches to lower case, and compared + * them with the search as typed. So any search with a capital + * letter in it matched nobody. + * + * @link https://github.com/SimpleMachines/SMF/commit/7ee5156e2 Introduced by "Uses correct param when searching by email in memberlist" + * @link https://github.com/SimpleMachines/SMF/pull/9716 */ #[DataProvider('spellings')] public function testASearchIgnoresCase(string $how): void diff --git a/tests/Integration/Http/PostingTest.php b/tests/Integration/Http/PostingTest.php index 1f2c44b5e0c..94ad221c344 100644 --- a/tests/Integration/Http/PostingTest.php +++ b/tests/Integration/Http/PostingTest.php @@ -47,6 +47,13 @@ class PostingTest extends HttpTestCase * Public methods ****************/ + /** + * A topic can be started and replied to. + * + * Expected: posting through the new topic form creates a topic that shows + * its subject and body, and replying through the reply form adds + * a second message that shows on the topic, with nothing logged. + */ public function testStartingATopicAndReplyingToIt(): void { $this->signInAsAdmin(); @@ -98,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 7ea54625259..093c859c88f 100644 --- a/tests/Integration/Http/ProfileTrackingTest.php +++ b/tests/Integration/Http/ProfileTrackingTest.php @@ -42,10 +42,17 @@ class ProfileTrackingTest extends HttpTestCase ****************/ /** - * The page starts from the IP address the member last used. Tracking - * yourself always worked, because you are the member the forum has - * loaded; tracking anybody else read the address from a member who had - * not been loaded, and failed with a TypeError. + * Another member's IP address can be tracked. + * + * Expected: ?action=profile;area=tracking;sa=ip for a member other than + * the moderator shows the IP address that member last used, with + * nothing logged. + * Guards: the page starts from the IP address the member last used. + * Tracking yourself worked, because you are the member the forum + * has loaded; tracking anybody else read the address from a + * member who had not been loaded, and failed with a TypeError. + * + * @link https://github.com/SimpleMachines/SMF/pull/9718 */ public function testAnotherMembersIpAddressCanBeTracked(): void { diff --git a/tests/Integration/Http/RecentTest.php b/tests/Integration/Http/RecentTest.php index 30df35167e3..50de3288e3b 100644 --- a/tests/Integration/Http/RecentTest.php +++ b/tests/Integration/Http/RecentTest.php @@ -30,12 +30,21 @@ class RecentTest extends HttpTestCase ****************/ /** - * ?action=recent;boards= was a 500 for any value at all. + * Recent posts can be narrowed to a list of boards. * - * Recent::execute() turns the comma separated list into an array of - * integers, and getBoards() then ran explode() over it a second time, which - * is a TypeError on an array. The boards list is how the board index's - * "recent posts" links for a set of boards reach the page. + * The boards list is how the board index's "recent posts" links for a set + * of boards reach the page. + * + * Expected: each case in boardLists() requests ?action=recent;boards= with + * that many visible boards, and gets a forum page with nothing + * logged. + * Guards: ?action=recent;boards= was a 500 for any value at all. + * Recent::execute() turns the comma separated list into an array + * of integers, and getBoards() then ran explode() over it a + * second time, which is a TypeError on an array. + * + * @link https://github.com/SimpleMachines/SMF/commit/633b3fa97 Introduced by "Implements SMF\Actions\Recent and related classes" + * @link https://github.com/SimpleMachines/SMF/pull/9713 */ #[DataProvider('boardLists')] public function testRecentPostsCanBeNarrowedToBoards(int $count): void diff --git a/tests/Integration/Http/ReplyNotificationTest.php b/tests/Integration/Http/ReplyNotificationTest.php index 0146e5f3569..75b9656d18c 100644 --- a/tests/Integration/Http/ReplyNotificationTest.php +++ b/tests/Integration/Http/ReplyNotificationTest.php @@ -69,10 +69,15 @@ class ReplyNotificationTest extends HttpTestCase /** * A member emailed about every reply is not told they will hear no more. * - * The "until you visit it" wording was chosen for any frequency other - * than "never", so a member who asked for an email about every reply was - * told they would get no more until they visited the topic, and then - * went on getting them. + * Expected: the reply notification sent to a member whose msg_notify_pref + * is 1 does not contain the "until you visit it" wording. + * Guards: the "until you visit it" wording was chosen for any frequency + * other than "never", so a member who asked for an email about + * every reply was told they would get no more until they visited + * the topic, and then went on getting them. + * + * @link https://github.com/SimpleMachines/SMF/commit/147061675 Introduced by "Shift watched topics and watched boards to use alerts" + * @link https://github.com/SimpleMachines/SMF/pull/9747 */ public function testAMemberEmailedAboutEveryReplyIsNotToldTheEmailsWillStop(): void { @@ -81,6 +86,12 @@ public function testAMemberEmailedAboutEveryReplyIsNotToldTheEmailsWillStop(): v $this->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 df1e4987c75..5cebf302921 100644 --- a/tests/Integration/Http/StatsStartersTest.php +++ b/tests/Integration/Http/StatsStartersTest.php @@ -47,8 +47,15 @@ class StatsStartersTest extends HttpTestCase ****************/ /** - * The list skipped the members ranked after index 10, which kept indexes - * 0 to 10: eleven members under a heading that says ten. + * The top topic starters list holds ten members. + * + * Expected: with eleven members who each started two topics, ?action=stats + * lists exactly ten starters, with nothing logged. + * Guards: the list skipped the members ranked after index 10, which kept + * indexes 0 to 10: eleven members under a heading that says ten. + * + * @link https://github.com/SimpleMachines/SMF/commit/58920877d Introduced by "Show 10 top topic starters" + * @link https://github.com/SimpleMachines/SMF/pull/9725 */ public function testTheTopTenStartersAreTen(): void { @@ -71,9 +78,19 @@ public function testTheTopTenStartersAreTen(): void } /** + * Starters with no member do not take a place in the top ten. + * * Topics credited to nobody, or to a member since deleted, are counted * too, but there is no name to list. Such a starter must not take one of * the ten places and leave it empty. + * + * Expected: with eleven members who each started two topics and three + * topics credited to no member, ?action=stats lists exactly ten + * starters, with nothing logged. + * Guards: a starter with no member to name kept its place in the list + * empty, so the ten places held fewer than ten members. + * + * @link https://github.com/SimpleMachines/SMF/pull/9725 */ public function testStartersWithNoMemberDoNotTakeAPlace(): void { diff --git a/tests/Integration/Http/StatsTest.php b/tests/Integration/Http/StatsTest.php index 65508f0b67c..e5132f24606 100644 --- a/tests/Integration/Http/StatsTest.php +++ b/tests/Integration/Http/StatsTest.php @@ -42,11 +42,20 @@ class StatsTest extends HttpTestCase ****************/ /** - * The top tens were ordered by their count alone, so members with the same - * count came in whatever order the database returned them: here, on both - * engines, the later member first. PostgreSQL also returns a row it has - * just rewritten after the others, so there the order changes whenever a - * tied member's row is updated. + * Members with the same post count are ranked in the order they joined. + * + * Expected: with two members tied on post count, the earlier member is + * listed before the later one in the top posters on + * ?action=stats, even after the earlier row is rewritten, with + * nothing logged. + * Guards: the top tens were ordered by their count alone, so members with + * the same count came in whatever order the database returned + * them: here, on both engines, the later member first. + * PostgreSQL also returns a row it has just rewritten after the + * others, so there the order changed whenever a tied member's row + * was updated. + * + * @link https://github.com/SimpleMachines/SMF/pull/9722 */ public function testMembersWithTheSamePostCountAreRankedInTheOrderTheyJoined(): void { diff --git a/tests/Integration/Http/ThemeCopyListTest.php b/tests/Integration/Http/ThemeCopyListTest.php index 7c87a329a7a..343db2020ea 100644 --- a/tests/Integration/Http/ThemeCopyListTest.php +++ b/tests/Integration/Http/ThemeCopyListTest.php @@ -28,10 +28,19 @@ class ThemeCopyListTest extends HttpTestCase ****************/ /** + * The theme copy page lists the templates and logs nothing. + * * Besides the templates, the page works out which language files the theme - * already has, and whether each can be written. It built that file's path - * out of the array describing it, which logged "Array to string - * conversion" for every language file the theme had. + * already has, and whether each can be written. + * + * Expected: ?action=admin;area=theme;th=1;sa=copy lists Stats.template.php + * and leaves nothing in the error log. + * Guards: the page built each language file's path out of the array + * describing it, which logged "Array to string conversion" for + * every language file the theme had. + * + * @link https://github.com/SimpleMachines/SMF/commit/74b3480a6 Introduced by "Fixes a few more "undefined" issues" + * @link https://github.com/SimpleMachines/SMF/pull/9720 */ public function testThePageLogsNothing(): void { diff --git a/tests/Integration/Http/TopicSplitTest.php b/tests/Integration/Http/TopicSplitTest.php index 6d3e0c84ecc..9e74e1daca5 100644 --- a/tests/Integration/Http/TopicSplitTest.php +++ b/tests/Integration/Http/TopicSplitTest.php @@ -41,17 +41,23 @@ class TopicSplitTest extends HttpTestCase /** * The posts to choose from are listed oldest first. * - * The queries behind the list only said ORDER BY when the member views - * topics newest first, and otherwise left the order to the database. MySQL - * happens to return InnoDB rows in primary key order, so it never showed - * there. PostgreSQL returns them in the order they sit in the table, and an - * UPDATE writes a row's new version at the end, so a post that had been - * edited was listed after the ones posted later. With LIMIT on the same - * queries, it could also turn up on two pages of the list and be missing - * from another. + * The edit in the test is what makes PostgreSQL misorder the list. On MySQL + * the test passes either way. * - * The edit below is what makes PostgreSQL misorder the list. On MySQL the - * test passes either way. + * Expected: after the first post of a three post topic is edited, + * ?action=splittopics lists the posts in the order of their + * ids, with nothing logged. + * Guards: the queries behind the list only said ORDER BY when the member + * views topics newest first, and otherwise left the order to the + * database. MySQL happens to return InnoDB rows in primary key + * order, so it never showed there. PostgreSQL returns them in the + * order they sit in the table, and an UPDATE writes a row's new + * version at the end, so a post that had been edited was listed + * after the ones posted later. With LIMIT on the same queries, it + * could also turn up on two pages of the list and be missing from + * another. + * + * @link https://github.com/SimpleMachines/SMF/pull/9714 */ public function testThePostsAreListedOldestFirst(): void { diff --git a/tests/Integration/Http/UnsubscribeLinkTest.php b/tests/Integration/Http/UnsubscribeLinkTest.php index 30a3bdca5dd..28b314d5281 100644 --- a/tests/Integration/Http/UnsubscribeLinkTest.php +++ b/tests/Integration/Http/UnsubscribeLinkTest.php @@ -57,6 +57,10 @@ class UnsubscribeLinkTest extends HttpTestCase /** * A token made for one topic does not unsubscribe from another. + * + * Expected: following a link for one topic with a token made for the next + * topic returns 403 with "does not appear to be valid", and the + * member's preference for the topic is unchanged. */ public function testATokenForAnotherTopicIsRefused(): void { @@ -76,10 +80,18 @@ public function testATokenForAnotherTopicIsRefused(): void /** * Following the link stops the emails and leaves the alerts alone. * - * None of these situations worked. The token was checked against topic 0 - * whatever topic it was made for, so every link was refused as invalid. A - * forum with guest access off, or a topic in a board that guests cannot - * see, sent the member to the login form before the link was looked at. + * Expected: each case in situations() follows a valid unsubscribe link as + * a guest and sees "has been unsubscribed from new reply + * notifications". The member's preference for the topic changes + * from both to alerts only, and nothing is logged. + * Guards: none of these situations worked. The token was checked against + * topic 0 whatever topic it was made for, so every link was + * refused as invalid. A forum with guest access off, or a topic + * in a board that guests cannot see, sent the member to the login + * form before the link was looked at. + * + * @link https://github.com/SimpleMachines/SMF/commit/4bf14e2a6 Introduced by "Implements SMF\Actions\Notify and sub-classes" + * @link https://github.com/SimpleMachines/SMF/pull/9734 */ #[DataProvider('situations')] public function testTheLinkUnsubscribesAMemberWhoIsNotLoggedIn(bool $guest_access, bool $guests_see_board): void diff --git a/tests/Integration/ModLogTest.php b/tests/Integration/ModLogTest.php index 51b4a5f89c9..f5e65217582 100644 --- a/tests/Integration/ModLogTest.php +++ b/tests/Integration/ModLogTest.php @@ -42,12 +42,18 @@ class ModLogTest extends IntegrationTestCase ****************/ /** - * Entries logged in the same second tie on the time column, and with only - * that column to go on the database chooses their order. MySQL lists them - * oldest first, the opposite of the rest of the log. PostgreSQL lists them - * newest first until one of the rows is rewritten, after which it does not, - * so at the edge of a page which of them are shown can change between page - * loads. + * Entries logged in the same second are listed newest first. + * + * Expected: list_getModLogEntries() with the sort lm.log_time DESC lists two + * entries logged at the same time with the newer one first. + * Guards: tied entries had only the time column to go on, so the database + * chose their order. MySQL listed them oldest first, the opposite + * of the rest of the log. PostgreSQL listed them newest first until + * one of the rows was rewritten, after which it did not, so at the + * edge of a page which of them were shown could change between + * page loads. + * + * @link https://github.com/SimpleMachines/SMF/pull/9726 */ public function testEntriesLoggedInTheSameSecondAreNewestFirst(): void { @@ -57,12 +63,18 @@ public function testEntriesLoggedInTheSameSecondAreNewestFirst(): void } /** - * Every column can tie, so every column needs the same tiebreaker. The - * sort a list passes is the column's default or its reverse, and the ties - * turn around along with it. + * Entries that tie on the sorted column follow the direction of the sort. + * + * Every column can tie, so every column needs the same tiebreaker. The sort + * a list passes is the column's default or its reverse, and the ties turn + * around along with it. + * + * Expected: each case in sorts() lists two tied entries oldest first, or + * newest first when the sort is descending. + * Guards: only the time column had a tiebreaker, so ties on any other + * column were left to the database. * - * @param string $sort The ORDER BY clause the list passes. - * @param bool $newest_first Whether the tied entries should come out newest first. + * @link https://github.com/SimpleMachines/SMF/pull/9726 */ #[DataProvider('sorts')] public function testTiesFollowTheDirectionOfTheSort(string $sort, bool $newest_first): void diff --git a/tests/Integration/ModSettingsTest.php b/tests/Integration/ModSettingsTest.php index 5f473fb18eb..fc6887730ab 100644 --- a/tests/Integration/ModSettingsTest.php +++ b/tests/Integration/ModSettingsTest.php @@ -46,6 +46,12 @@ class ModSettingsTest extends IntegrationTestCase * Public methods ****************/ + /** + * Writing a value stores it, in the database and in Config::$modSettings. + * + * Expected: updateModSettings([$name => '41']) makes rawSetting() and + * Config::$modSettings both return '41'. + */ public function testWritesAValue(): void { Config::updateModSettings([self::COUNTER => '41']); @@ -54,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']); @@ -67,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']); @@ -76,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 { @@ -91,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++) { @@ -105,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 { @@ -123,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 c5be5f11635..c36220aba97 100644 --- a/tests/Integration/NotifyTest.php +++ b/tests/Integration/NotifyTest.php @@ -35,11 +35,20 @@ class NotifyTest extends IntegrationTestCase ****************/ /** - * changeBoardTopicPref() wrote the log_notify row for User::$me rather - * than for the member whose preference was being changed. Subscribing - * through a token therefore subscribed whoever followed the link, which - * for a guest meant a row for member 0, and left the member themselves - * with a preference saying they were subscribed and nothing to send to. + * Subscribing through a token records the member the token is for. + * + * Expected: changeBoardTopicPref() with a member_info for another member + * adds a log_notify row for that member and leaves the acting + * administrator's subscription as it was, with no errors logged. + * Guards: changeBoardTopicPref() wrote the log_notify row for User::$me + * rather than for the member whose preference was being changed. + * Subscribing through a token therefore subscribed whoever + * followed the link, which for a guest meant a row for member 0, + * and left the member with a preference saying they were + * subscribed and nothing to send to. + * + * @link https://github.com/SimpleMachines/SMF/commit/6228a85e4 Introduced by "Implements unsubscribe tokens for email notifications" + * @link https://github.com/SimpleMachines/SMF/pull/9735 */ public function testSubscribingThroughATokenRecordsTheMemberItIsFor(): void { diff --git a/tests/Integration/PermissionProfileTest.php b/tests/Integration/PermissionProfileTest.php index 7eecf3179d1..01a80f3cb4b 100644 --- a/tests/Integration/PermissionProfileTest.php +++ b/tests/Integration/PermissionProfileTest.php @@ -29,14 +29,20 @@ class PermissionProfileTest extends IntegrationTestCase /** * The profiles come back in the order they were made, as they did in 2.1. * - * The query lost its ORDER BY when it was joined to the boards, which left - * the order to the database. MySQL happens to return InnoDB rows in primary - * key order. PostgreSQL returns them in the order they sit in the table, and - * an UPDATE writes a row's new version at the end, so renaming the default - * profile moved it to the bottom of every list of profiles. + * The rename in the test is what makes PostgreSQL misorder them. On MySQL + * the test passes either way. * - * The rename below is what makes PostgreSQL misorder them. On MySQL the test - * passes either way. + * Expected: after the default profile is rewritten, loadAll() returns at + * least four profiles, with their ids in ascending order. + * Guards: the query lost its ORDER BY when it was joined to the boards, + * which left the order to the database. MySQL happens to return + * InnoDB rows in primary key order. PostgreSQL returns them in the + * order they sit in the table, and an UPDATE writes a row's new + * version at the end, so renaming the default profile moved it to + * the bottom of every list of profiles. + * + * @link https://github.com/SimpleMachines/SMF/commit/1c8b96a95 Introduced by "Implements SMF\Permissions\PermissionProfile" + * @link https://github.com/SimpleMachines/SMF/pull/9715 */ public function testProfilesAreLoadedInTheOrderTheyWereMade(): void { diff --git a/tests/Integration/SchemaTest.php b/tests/Integration/SchemaTest.php index e7f7b940415..61ae25deb3f 100644 --- a/tests/Integration/SchemaTest.php +++ b/tests/Integration/SchemaTest.php @@ -38,13 +38,25 @@ class SchemaTest extends IntegrationTestCase * Public methods ****************/ + /** + * The v3_0 schema declares tables. + * + * This guards the two tests below: if getAll() ever returned nothing they + * would both pass by iterating over an empty list. + * + * Expected: Table::getAll('v3_0') is not empty. + */ public function testTheSchemaDeclaresTables(): void { - // Guards the two tests below: if getAll() ever returns nothing they - // would both pass by iterating over an empty list. $this->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( @@ -63,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 = []; @@ -89,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/TestCase/AbstractRouteTestCase.php b/tests/TestCase/AbstractRouteTestCase.php index bcf090e4510..1bb3751ad7e 100644 --- a/tests/TestCase/AbstractRouteTestCase.php +++ b/tests/TestCase/AbstractRouteTestCase.php @@ -49,12 +49,23 @@ abstract class AbstractRouteTestCase extends TestCase * Public methods ****************/ + /** + * Each class under test implements Routable. + * + * Expected: each class in provideClassNameCases() is a Routable. + */ #[DataProvider('provideClassNameCases')] public function testImplementsRoutable(string $class_name): void { $this->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 { @@ -67,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 412b13c7dcf..603608a63ff 100644 --- a/tests/TestCase/SlugRouteTestTrait.php +++ b/tests/TestCase/SlugRouteTestTrait.php @@ -64,6 +64,13 @@ abstract public static function provideCorrectSlugRouteCases(): iterable; * Public methods ****************/ + /** + * A route with an incorrect slug redirects to the canonical route. + * + * Expected: for each case in provideIncorrectSlugRouteCases(), parseRoute() + * redirects to a URL containing the parameters, returns the + * parameters, and records the requested slug as Slug::$requested. + */ #[DataProvider('provideIncorrectSlugRouteCases')] public function testIncorrectSlugRoute(array $route, array $params, string $expected_slug): void { @@ -82,6 +89,13 @@ public function testIncorrectSlugRoute(array $route, array $params, string $expe } } + /** + * A route with the correct slug parses and builds without a redirect. + * + * Expected: for each case in provideCorrectSlugRouteCases(), parseRoute() + * returns the parameters, and buildRoute() on them returns the route + * with no leftover parameters. + */ #[DataProvider('provideCorrectSlugRouteCases')] public function testCorrectSlugRoute(array $route, array $params): void { @@ -101,6 +115,12 @@ public function testCorrectSlugRoute(array $route, array $params): void } } + /** + * Each slug fixture is registered as an empty known slug. + * + * Expected: for each case in provideSlugCases(), Slug::$known holds a Slug for + * that type and ID whose string value is empty. + */ #[DataProvider('provideSlugCases')] public function testSlug(string $type, int $id, string $slug): void { diff --git a/tests/Unit/ActionTraitTest.php b/tests/Unit/ActionTraitTest.php index b8c179290a4..1b1ff6baeb3 100644 --- a/tests/Unit/ActionTraitTest.php +++ b/tests/Unit/ActionTraitTest.php @@ -33,23 +33,45 @@ class ActionTraitTest extends TestCase * Public methods ****************/ + /** + * load() returns an instance of the class it was called on. + * + * Expected: Login2::load() returns a Login2 and Logout::load() returns a Logout. + */ public function testLoadReturnsAnInstanceOfTheClassItWasCalledOn(): void { $this->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(); @@ -57,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(); @@ -70,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 b0eb467d423..3a7440b97ba 100644 --- a/tests/Unit/AvatarFallbackTest.php +++ b/tests/Unit/AvatarFallbackTest.php @@ -48,16 +48,22 @@ class AvatarFallbackTest extends TestCase ****************/ /** - * The constructor tries each way of finding an image in turn, and stops - * when it has a URL that validates. The last of those ways is a 1x1 - * transparent GIF as a data URI, which is there so that there is always an - * answer - but a data URI is not something Url::isValid() accepts, so it - * never satisfied the condition that ends the search and the search went - * round again, and again, until the request was killed. + * An avatar that cannot be found ends as a transparent GIF. * - * Reaching it takes nothing exotic: the step before only produces a URL - * when avatar_url is set, so a forum where that setting was never written - * hung on any member with an avatar. + * Reaching the last resort takes nothing exotic: the step before it only + * produces a URL when avatar_url is set, so a forum where that setting was + * never written reaches it for any member with an avatar. + * + * Expected: new Avatar(url: 'Oxygen/beagle.png', id_member: 1) with no + * avatar_url setting and Gravatar off has a url starting with + * data:image/gif;base64,. + * Guards: the constructor tries each way of finding an image in a loop + * that runs until it holds a URL that Url::isValid() accepts. The + * last of those ways is a data URI, which is not one, so the + * search went round again and again until the request was killed. + * + * @link https://github.com/SimpleMachines/SMF/commit/399ceedb4 Introduced by "Implements SMF\Avatar" + * @link https://github.com/SimpleMachines/SMF/pull/9587 */ public function testAnAvatarThatCannotBeFoundEndsAsATransparentGif(): void { @@ -71,8 +77,14 @@ public function testAnAvatarThatCannotBeFoundEndsAsATransparentGif(): void } /** - * The control, and the reason the loop exists: when there is somewhere to - * look, it is looked in, and the search ends long before the last resort. + * An avatar that can be found is still found. + * + * The control: when there is somewhere to look, it is looked in, and the + * search ends long before the last resort. + * + * Expected: new Avatar(url: 'Oxygen/beagle.png', id_member: 1) with + * avatar_url set to https://example.com/avatars has the url + * https://example.com/avatars/Oxygen/beagle.png. */ public function testAnAvatarThatCanBeFoundIsStillFound(): void { diff --git a/tests/Unit/AvatarGalleryDirectoryTest.php b/tests/Unit/AvatarGalleryDirectoryTest.php index 30039b540b1..275c39328b0 100644 --- a/tests/Unit/AvatarGalleryDirectoryTest.php +++ b/tests/Unit/AvatarGalleryDirectoryTest.php @@ -50,11 +50,24 @@ class AvatarGalleryDirectoryTest extends TestCase ****************/ /** + * A gallery avatar is looked for in the configured directory. + * * The admin can move the avatar gallery, and the admin panel offers the - * setting and warns when the directory it names is not there. Profile - * lists the gallery out of that directory and refuses to save a choice - * from anywhere else, so it is the directory a stored avatar is a path - * into, and it is where the file has to be looked for. + * setting and warns when the directory it names is not there. Profile lists + * the gallery out of that directory and refuses to save a choice from + * anywhere else, so it is the directory a stored avatar is a path into, and + * it is where the file has to be looked for. + * + * Expected: with avatar_directory pointing at Themes/default/images, new + * Avatar(url: 'cake.png', id_member: 1) has the url + * https://example.com/gallery/cake.png. + * Guards: the file was looked for under the board directory's avatars + * directory whatever the setting said, while the address was + * built from avatar_url. A forum whose admin had moved the + * gallery showed the default image for every member. + * + * @link https://github.com/SimpleMachines/SMF/commit/399ceedb4 Introduced by "Implements SMF\Avatar" + * @link https://github.com/SimpleMachines/SMF/pull/9588 */ public function testAGalleryAvatarIsLookedForInTheConfiguredDirectory(): void { @@ -65,6 +78,19 @@ public function testAGalleryAvatarIsLookedForInTheConfiguredDirectory(): void $this->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'); @@ -75,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 81fc5940d0e..95d77960c97 100644 --- a/tests/Unit/AvatarTest.php +++ b/tests/Unit/AvatarTest.php @@ -50,8 +50,13 @@ class AvatarTest extends TestCase ****************/ /** + * A remote avatar URL is left alone. + * * The control. Something with a scheme on it is an address to fetch from, * and is left as one. + * + * Expected: new Avatar(url: 'https://example.org/pictures/me.png', + * id_member: 1) has that same url. */ public function testARemoteAvatarUrlIsLeftAlone(): void { @@ -63,12 +68,24 @@ public function testARemoteAvatarUrlIsLeftAlone(): void } /** + * A gallery avatar is found under the avatars directory. + * * An avatar chosen from the gallery is stored as a path under the avatars * directory, not as a URL, so there is no scheme on it and no host in it. - * Saying so up front means the file is looked for by name; reading it as a - * URL instead and working back to a file from that URL's path lands outside - * the avatar directories and finds nothing, and on a forum at the root of - * its domain that path is null and stripping the board URL off it throws. + * Saying so up front means the file is looked for by name. + * + * Expected: each case in boardUrls() resolves new Avatar(url: + * 'Oxygen/beagle.png', id_member: 1) to the url + * /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 @@ -81,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 { @@ -93,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 8a0c4e3a869..b6d5d4835b5 100644 --- a/tests/Unit/BoardsCountPostsBackcompatTest.php +++ b/tests/Unit/BoardsCountPostsBackcompatTest.php @@ -33,13 +33,26 @@ class BoardsCountPostsBackcompatTest extends TestCase ****************/ /** + * While installing or upgrading, queries naming count_posts are not + * rewritten. + * * The upgrader meets the boards table while it still has count_posts, and - * its queries mean that column. Rewriting them to name posts_count made the - * backup step fail with "Unknown column 'posts_count' in 'field list'", and - * would have made the migration read the value it was in the middle of - * writing. + * its queries mean that column. * * Its own process, because SMF_INSTALLING cannot be undefined again. + * + * Expected: with SMF_INSTALLING defined, on both engines, + * backcompatQuoteFixes() returns each query in upgraderQueries() + * unchanged, and backcompatInsertFixes() returns a row naming + * count_posts unchanged. + * Guards: rewriting the upgrader's queries to name posts_count made the + * backup step fail with "Unknown column 'posts_count' in 'field + * list'", and would have made the migration read the value it was + * in the middle of writing. + * + * @link https://github.com/SimpleMachines/SMF/commit/59b22b12e Introduced by "Replace count_posts (where 0 = true) with posts_count (where 1 = true)" + * @link https://github.com/SimpleMachines/SMF/commit/a4aec80eb Introduced by "Implements backcompatInsertFixes() in database APIs" + * @link https://github.com/SimpleMachines/SMF/pull/9677 */ #[RunInSeparateProcess] #[PreserveGlobalState(false)] @@ -60,12 +73,26 @@ public function testTheUpgraderGetsTheQueriesItWrote(): void } } + /** + * A legacy query is still rewritten for a mod that sends one. + * + * Expected: each case in legacyQueryProvider() has its count_posts + * reference rewritten by backcompatQuoteFixes() to the new + * posts_count column and its inverted logic. + */ #[DataProvider('legacyQueryProvider')] public function testALegacyQueryIsStillRewrittenForAModThatSendsOne(string $class, string $query, string $expected): void { $this->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 { @@ -76,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 857b0ab08f6..06f8f65c980 100644 --- a/tests/Unit/CacheApiTest.php +++ b/tests/Unit/CacheApiTest.php @@ -102,6 +102,13 @@ public function tearDown(): void rmdir($this->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 { @@ -114,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 { @@ -127,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 { @@ -138,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 { @@ -146,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 { @@ -158,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 @@ -172,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 { @@ -188,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 { @@ -202,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 { @@ -219,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); @@ -229,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 81797b035d1..c6522aaf435 100644 --- a/tests/Unit/ClassNameSyntaxTest.php +++ b/tests/Unit/ClassNameSyntaxTest.php @@ -93,6 +93,12 @@ class ClassNameSyntaxTest extends TestCase * Public methods ****************/ + /** + * Every class name is written as a resolver rather than a string. + * + * Expected: no string literal in Sources, Themes or the entry points names + * an SMF class, except the names listed in ALLOWED for that file. + */ public function testEveryClassNameIsWrittenAsAResolverRatherThanAString(): void { $found = []; @@ -113,8 +119,13 @@ public function testEveryClassNameIsWrittenAsAResolverRatherThanAString(): void } /** + * Nothing in the list of exceptions has gone stale. + * * An exception whose reason has gone stale is an exception nobody notices, * so the list is only allowed to name things that are really still there. + * + * Expected: every name in ALLOWED is still written as a string literal in + * the file it is listed under. */ public function testNothingInTheListOfExceptionsHasGoneStale(): void { diff --git a/tests/Unit/ClassReferenceTest.php b/tests/Unit/ClassReferenceTest.php index 378885c500c..63b07557948 100644 --- a/tests/Unit/ClassReferenceTest.php +++ b/tests/Unit/ClassReferenceTest.php @@ -96,10 +96,20 @@ class ClassReferenceTest extends TestCase ****************/ /** - * The regression this pins is a background task queued as - * 'SMF\Tasks\FetchSMfiles' by the upgrader, where the class is FetchSMFiles. - * The wrong spelling is the scheduled task's own name, fetchSMfiles, which - * sits two lines from the right answer in TaskRunner::$scheduled_tasks. + * Every class name written as a string names something real. + * + * Expected: each SMF\... string literal in the shipped files, apart from + * the arguments of existence probes, names a class or namespace + * that the file tree defines, and any ::method it names exists. + * Guards: the upgrader queued a background task as + * 'SMF\Tasks\FetchSMfiles' where the class is FetchSMFiles. The + * wrong spelling is the scheduled task's own name, fetchSMfiles, + * which sits two lines from the right answer in + * TaskRunner::$scheduled_tasks, so the task could never be found + * and the work never happened. + * + * @link https://github.com/SimpleMachines/SMF/commit/99321ba41 Introduced by "Rewrites upgrader" + * @link https://github.com/SimpleMachines/SMF/pull/9661 */ public function testEveryClassNameWrittenAsAStringNamesSomethingReal(): void { diff --git a/tests/Unit/CreatePostNotifyTest.php b/tests/Unit/CreatePostNotifyTest.php index fb90ae7eb1b..392ddb5b201 100644 --- a/tests/Unit/CreatePostNotifyTest.php +++ b/tests/Unit/CreatePostNotifyTest.php @@ -28,6 +28,12 @@ class CreatePostNotifyTest extends TestCase * Public methods ****************/ + /** + * The offset is relative to the forum's own time zone. + * + * Expected: with default_timezone set to Etc/GMT-2, + * getTimeOffset('Etc/GMT-5') returns 3.0. + */ public function testTheOffsetIsRelativeToTheForumTimezone(): void { Config::$modSettings['default_timezone'] = 'Etc/GMT-2'; @@ -39,6 +45,13 @@ public function testTheOffsetIsRelativeToTheForumTimezone(): void // the first member carries an attribute. The SMF/section_comments fixer inserts // the banner between the attribute and its method, which is why the data provider // case is second rather than first. + /** + * A time zone is reported as its offset in hours from the forum time zone. + * + * Expected: each case in timezoneProvider() returns the expected offset + * from UTC as a float, keeping half and quarter hours and + * treating an empty time zone as no offset. + */ #[DataProvider('timezoneProvider')] public function testGetTimeOffset(string $timezone, float $expected): void { diff --git a/tests/Unit/ForumTest.php b/tests/Unit/ForumTest.php index 94a7e44ceae..1671575dfa8 100644 --- a/tests/Unit/ForumTest.php +++ b/tests/Unit/ForumTest.php @@ -37,6 +37,13 @@ class ForumTest extends TestCase * Public methods ****************/ + /** + * Executing the forum registers services before the action is preflighted. + * + * Expected: execute() on an action that depends on services, with an + * integrate_services hook registered, runs the action with its + * UserRepository and DatabaseConnection injected. + */ public function testExecuteRegistersServicesBeforePreflight(): void { $action_called = false; @@ -69,7 +76,9 @@ public function testExecuteRegistersServicesBeforePreflight(): void } /** - * Tests that an unknown action returns null. + * An unknown action resolves to no current action. + * + * Expected: getCurrentAction() returns null for an unregistered action. */ public function testUnknownAction(): void { @@ -83,7 +92,10 @@ public function testUnknownAction(): void } /** - * Tests that an unknown action is handled by the configured fallback action. + * An unknown action is handled by the configured fallback action. + * + * Expected: with integrate_fallback_action set, getCurrentAction() returns an + * instance of that action for an unregistered action. */ public function testFallbackAction(): void { @@ -100,7 +112,10 @@ public function testFallbackAction(): void } /** - * Tests the implied action selected from the request parameters. + * The request parameters select the implied action. + * + * Expected: each case in provideImpliedActions() makes getCurrentAction() return + * an instance of the expected action. */ #[DataProvider('provideImpliedActions')] public function testImpliedAction(array $get, string $expected): void @@ -117,8 +132,10 @@ public function testImpliedAction(array $get, string $expected): void } /** - * Tests that the configured default action takes precedence when no action, - * topic, or board is specified. + * The configured default action wins when nothing else is requested. + * + * Expected: with integrate_default_action set and no action, topic or board, + * getCurrentAction() returns an instance of the configured action. */ public function testDefaultAction(): void { diff --git a/tests/Unit/IPTest.php b/tests/Unit/IPTest.php index 4ec1b40b7d1..6799f4d42e0 100644 --- a/tests/Unit/IPTest.php +++ b/tests/Unit/IPTest.php @@ -27,16 +27,33 @@ class IPTest extends TestCase * Public methods ****************/ + /** + * An IPv6 address is cast to its shortest, lowercase form. + * + * Expected: (string) new IP('2001:DB8::0001') returns '2001:db8::1'. + */ public function testItNormalisesIPv6ToItsShortestForm(): void { $this->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)); @@ -44,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'); @@ -53,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()); @@ -60,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()); @@ -72,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'); @@ -102,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, @@ -120,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 fe5b121a1b6..6e6f4c59ee2 100644 --- a/tests/Unit/Infrastructure/ContainerFactoryTest.php +++ b/tests/Unit/Infrastructure/ContainerFactoryTest.php @@ -27,6 +27,12 @@ class ContainerFactoryTest extends TestCase * Public methods ****************/ + /** + * A container factory resolves its dependencies through Services. + * + * Expected: getting UserRepository from the container calls its factory, which + * gets the DatabaseConnection through Services and passes it on. + */ public function testFactoryReceivesContainerAndResolvesDependencies(): void { $container = new Container(); diff --git a/tests/Unit/Infrastructure/ServicesTest.php b/tests/Unit/Infrastructure/ServicesTest.php index cfe38ed7e27..b2a8f5b43dc 100644 --- a/tests/Unit/Infrastructure/ServicesTest.php +++ b/tests/Unit/Infrastructure/ServicesTest.php @@ -25,6 +25,12 @@ class ServicesTest extends TestCase * Public methods ****************/ + /** + * Services::has() delegates to the container. + * + * Expected: has('test') asks the container's has('test') once and returns its + * result, true. + */ public function testHasDelegatesToContainer(): void { $container = $this->createMock(ContainerInterface::class); @@ -40,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 56dcb2cb808..6ed4f494728 100644 --- a/tests/Unit/LangTest.php +++ b/tests/Unit/LangTest.php @@ -58,6 +58,13 @@ class LangTest extends TestCase * Public methods ****************/ + /** + * Language strings load in a process with no database. + * + * Expected: load('General') returns 'en_US', and Lang::$txt + * then has the 'number_of_days' and 'days' + * strings. + */ public function testItLoadsLanguageStringsWithNoDatabase(): void { $this->assertSame('en_US', Lang::load('General')); @@ -66,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(); @@ -94,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'); @@ -106,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); @@ -122,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 @@ -130,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')); @@ -138,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()); @@ -154,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()); @@ -182,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 6c1cd50ff9e..fbb08524ab6 100644 --- a/tests/Unit/MarkReadRouteTest.php +++ b/tests/Unit/MarkReadRouteTest.php @@ -28,12 +28,21 @@ class MarkReadRouteTest extends TestCase ****************/ /** - * '??' binds tighter than '?:', so the line that restored the implied - * sub-action read as '($params['sa'] ?? isset($params['topic'])) ? ...'. - * A sub-action that the route did state is a non-empty string, which is - * truthy, so it was thrown away and replaced with 'topic'. Marking - * everything read then ran the per-topic branch with no topic loaded, - * which is a database error and a 500. + * A sub-action stated in the route survives parsing. + * + * Expected: MarkRead::parseRoute(['markasread', 'all']) returns + * ['action' => '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 { @@ -48,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( @@ -57,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 { @@ -74,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 d1faf4ddda8..09bf4a774a8 100644 --- a/tests/Unit/MessageFormatterTest.php +++ b/tests/Unit/MessageFormatterTest.php @@ -33,12 +33,22 @@ class MessageFormatterTest extends TestCase ****************/ /** - * An object that can be a string is one of the things a caller may hand to - * Lang::getTxt(), and several of SMF's own value objects are exactly that: - * Url, IP, TimeInterval and PageIndex all implement \Stringable. The class - * skips any argument that is not already a string, and the intl formatter - * is handed only the scalar ones, so an object reached neither and its - * placeholder was printed to the member as it was written. + * A Stringable argument is used for its string value. + * + * Several of SMF's own value objects are Stringable, among them Url, + * IP, TimeInterval and PageIndex, and a caller may hand any of them to + * Lang::getTxt(). + * + * Expected: formatMessage('Hello {name}!', ['name' => + * ]) 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 { @@ -49,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 { @@ -62,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( @@ -70,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( @@ -78,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( @@ -86,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 086aa3f1f91..027200cadad 100644 --- a/tests/Unit/PageIndexTest.php +++ b/tests/Unit/PageIndexTest.php @@ -51,9 +51,16 @@ class PageIndexTest extends TestCase ****************/ /** + * A start is clamped to the first position of a real page. + * * Negative starts are invalid, but are clamped to zero. The invalid state * is retained so that page 1 is rendered as a link rather than as the * current page. + * + * Expected: each case in pageIndexProvider() leaves both the start that was + * passed in and PageIndex::$start at the expected start, sets + * Utils::$context['current_page'] to the expected zero-based + * page, and keeps all three after the object is cast to a string. */ #[DataProvider('pageIndexProvider')] public function testStartIsNormalised(int $start, int $num_items, int $num_per_page, int $expected_start, int $expected_page): void @@ -72,8 +79,12 @@ public function testStartIsNormalised(int $start, int $num_items, int $num_per_p } /** - * The rendered current page is one-based, while current_page in the - * context is zero-based. + * The rendered current page is one-based, while current_page is zero-based. + * + * Expected: each case in pageRenderingProvider() sets + * Utils::$context['current_page'] to the expected page minus one + * and renders the expected page in a + * element. */ #[DataProvider('pageRenderingProvider')] public function testCurrentPageIsRendered(int $start, int $num_items, int $num_per_page, int $expected_page): void @@ -85,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; @@ -99,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; @@ -111,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 { @@ -123,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; @@ -133,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 { @@ -150,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 { @@ -166,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 { @@ -184,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; @@ -195,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; @@ -207,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; @@ -219,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; @@ -238,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; @@ -249,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; @@ -268,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; @@ -284,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; @@ -299,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 5ce698e1643..e7399bd4862 100644 --- a/tests/Unit/PunycodeTest.php +++ b/tests/Unit/PunycodeTest.php @@ -27,11 +27,22 @@ class PunycodeTest extends TestCase * Public methods ****************/ + /** + * An all-ASCII domain passes through encode() unchanged. + * + * Expected: encode('example.com') returns 'example.com'. + */ public function testAsciiDomainsPassThroughUnchanged(): void { $this->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 93bc20c0dba..921e03bc889 100644 --- a/tests/Unit/QueryCaseFoldingTest.php +++ b/tests/Unit/QueryCaseFoldingTest.php @@ -125,6 +125,11 @@ class QueryCaseFoldingTest extends TestCase * Public methods ****************/ + /** + * No new LIKE comparison on user text skips the case folding types. + * + * Expected: scan() returns the per-file counts listed in BASELINE. + */ public function testNoNewComparisonSkipsTheCaseFoldingTypes(): void { $found = $this->scan(); @@ -143,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(); @@ -159,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 4d24a6c2226..c29becd8327 100644 --- a/tests/Unit/QueryStringRouteTest.php +++ b/tests/Unit/QueryStringRouteTest.php @@ -31,6 +31,12 @@ class QueryStringRouteTest extends TestCase * Public methods ****************/ + /** + * A plain topic route is parsed as the display action. + * + * Expected: parseRoute('/topics/1', []) returns + * ['action' => 'display', 'topic' => '1']. + */ public function testAPlainTopicRouteIsTheDisplayAction(): void { $this->assertSame( @@ -39,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( @@ -47,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( @@ -60,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']; @@ -70,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( @@ -81,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, @@ -92,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 @@ -108,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 02e80b1716c..a003c51afca 100644 --- a/tests/Unit/Route/ActionRouterTest.php +++ b/tests/Unit/Route/ActionRouterTest.php @@ -43,6 +43,12 @@ class ActionRouterTest extends AbstractRouteTestCase * Public methods ****************/ + /** + * A set of parameters is built into an action route. + * + * Expected: each case in buildRouteProvider() makes buildRoute() return the + * expected route segments and the parameters left over. + */ #[DataProvider('buildRouteProvider')] public function testBuildsAnActionRoute(array $params, array $expected): void { @@ -52,6 +58,12 @@ public function testBuildsAnActionRoute(array $params, array $expected): void ); } + /** + * An action route is parsed into the action, area and sub-action. + * + * Expected: each case in parseRouteProvider() makes parseRoute() return the + * expected parameters. + */ #[DataProvider('parseRouteProvider')] public function testParsesAnActionRoute(array $route, array $expected): void { @@ -61,6 +73,11 @@ public function testParsesAnActionRoute(array $route, array $expected): void ); } + /** + * A route that starts with an unknown action is not parsed. + * + * Expected: parseRoute(['unknown-action', 'foo']) returns an empty array. + */ public function testDoesNotParseAnUnknownAction(): void { $this->assertEmpty(TestActionRouter::parseRoute(['unknown-action', 'foo'])); diff --git a/tests/Unit/Route/CalendarRouteTest.php b/tests/Unit/Route/CalendarRouteTest.php index cdd827295ff..11968dbc6da 100644 --- a/tests/Unit/Route/CalendarRouteTest.php +++ b/tests/Unit/Route/CalendarRouteTest.php @@ -24,6 +24,12 @@ class CalendarRouteTest extends AbstractRouteTestCase * Public methods ****************/ + /** + * The clock alias is parsed as the clock sub-action of the calendar. + * + * Expected: Calendar::parseRoute(['clock']) returns + * ['action' => 'calendar', 'sa' => 'clock']. + */ public function testParsesClockAlias(): void { $this->assertSame( diff --git a/tests/Unit/SapiTest.php b/tests/Unit/SapiTest.php index 6b48337996f..8f4aa997dd9 100644 --- a/tests/Unit/SapiTest.php +++ b/tests/Unit/SapiTest.php @@ -27,6 +27,13 @@ class SapiTest extends TestCase * Public methods ****************/ + /** + * Dot segments in a relative path are resolved. + * + * Expected: canonicalPath('a/./b/../c') returns 'a' and 'c' + * joined by the directory separator, and + * canonicalPath('a/b/..') returns 'a'. + */ public function testCanonicalPathResolvesDotSegments(): void { // A relative path has no root to differ over, so once the separator is @@ -37,6 +44,14 @@ public function testCanonicalPathResolvesDotSegments(): void $this->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 da27a041121..a10ef93cdd3 100644 --- a/tests/Unit/SecurityTest.php +++ b/tests/Unit/SecurityTest.php @@ -38,9 +38,17 @@ class SecurityTest extends TestCase ****************/ /** - * A wrong password with no older hashing scheme left to try compares - * nothing, and was answered by reading a variable the comparisons had - * never set: a warning in the error log on every failed attempt. + * A wrong password is rejected when no older hashing scheme applies. + * + * Expected: checkPassword() with a bcrypt hash of a different password, + * and no fallback to try, returns false. + * Guards: with no older hashing scheme left to compare against, the result + * was read from a variable the comparisons had never set, which + * logged an "Undefined variable $is_correct" warning on every + * failed login attempt. + * + * @link https://github.com/SimpleMachines/SMF/commit/b44c6ea08 Introduced by "Prevents timing attacks against Security::checkPasswordFallbacks()" + * @link https://github.com/SimpleMachines/SMF/pull/9719 */ public function testAWrongPasswordIsRejectedWhenNoFallbackApplies(): void { @@ -50,6 +58,12 @@ public function testAWrongPasswordIsRejectedWhenNoFallbackApplies(): void $this->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); @@ -57,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); @@ -65,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( @@ -73,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(); @@ -92,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 9b47866d114..dc3443331f0 100644 --- a/tests/Unit/SpoofDetectorTest.php +++ b/tests/Unit/SpoofDetectorTest.php @@ -46,11 +46,19 @@ class SpoofDetectorTest extends TestCase ****************/ /** + * The list the installer writes is read as one name per line. + * * The installer stores the default list with the separators written out as * the two characters backslash and n, because the value travels through - * Table::populate() as a placeholder in a PHP string. Splitting it on real - * newlines alone gives one long name that nobody would ever type, so every - * name an admin put on the list was free to register. + * Table::populate() as a placeholder in a PHP string. + * + * Expected: with that list, checkReservedName() is true for Admin, + * Webmaster, Guest and root. + * Guards: the list was split on real newlines alone, which gave one long + * name that nobody would ever type, so every name an admin put + * on the list was free to register. + * + * @link https://github.com/SimpleMachines/SMF/pull/9484 */ public function testTheListTheInstallerWritesIsOneNamePerLine(): void { @@ -63,8 +71,13 @@ public function testTheListTheInstallerWritesIsOneNamePerLine(): void } /** + * A list written with real line breaks is read as one name per line. + * * A list an admin edited by hand arrives with real line breaks, whichever * kind their browser sent. + * + * Expected: with 'Admin\nWebmaster' and 'Admin\r\nWebmaster' as the list, + * checkReservedName('Webmaster') is true. */ public function testAListWrittenWithRealLineBreaksStillWorks(): void { @@ -77,6 +90,11 @@ public function testAListWrittenWithRealLineBreaksStillWorks(): void $this->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'; @@ -84,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'] = ''; @@ -92,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 { @@ -104,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 { @@ -114,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 { @@ -123,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 58609cbc9d6..f3e0f66bd77 100644 --- a/tests/Unit/TimeIntervalTest.php +++ b/tests/Unit/TimeIntervalTest.php @@ -36,27 +36,50 @@ class TimeIntervalTest extends TestCase * Public methods ****************/ + /** + * An ISO 8601 duration is written back out as it was given. + * + * Expected: (string) new TimeInterval('P1Y2M3DT4H5M6S') returns + * 'P1Y2M3DT4H5M6S'. + */ public function testItRoundTripsAnIso8601Duration(): void { $this->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( @@ -65,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')); @@ -82,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); @@ -110,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( @@ -128,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')); @@ -138,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 @@ -167,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 f5b630e2479..5f1b33250ce 100644 --- a/tests/Unit/TimeTest.php +++ b/tests/Unit/TimeTest.php @@ -43,13 +43,23 @@ class TimeTest extends TestCase * Public methods ****************/ + /** + * A Time built before the user is loaded uses the forum's default zone. + * + * Expected: new Time('@1785441064') while User::$me is not set has that + * timestamp and the default time zone, 'Pacific/Auckland'. + * Guards: a request can build a date before User::$me exists: redirecting + * from '?msg=1' to the topic that message is in happens in + * cleanRequest(), which runs long before the user is loaded, and + * cron.php never loads a user at all. User::$me is a typed + * static, so reading it then was a fatal error rather than an + * empty value. + * + * @link https://github.com/SimpleMachines/SMF/commit/7bc3a536c Introduced by "Removes unnecessary SMF\User::getTimezone() method" + * @link https://github.com/SimpleMachines/SMF/pull/9635 + */ public function testItFallsBackToTheForumsDefaultTimeZoneBeforeTheUserIsLoaded(): void { - // A request can build a date before User::$me exists: redirecting from - // '?msg=1' to the topic that message is in happens in cleanRequest(), - // which runs long before the user is loaded, and cron.php never loads a - // user at all. Reading User::$me then is a fatal error rather than an - // empty value, so the constructor has to manage without it. $this->assertFalse(isset(User::$me)); // The constructor only consults User::$me while it has no time zone @@ -68,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 29b594efc68..27c70f20595 100644 --- a/tests/Unit/UrlTest.php +++ b/tests/Unit/UrlTest.php @@ -29,6 +29,13 @@ class UrlTest extends TestCase * Public methods ****************/ + /** + * A URL exposes its host, path, query, fragment and port as properties. + * + * Expected: new Url('https://user@a.example.com:8080/a/b?c=d#f') has host + * 'a.example.com', path '/a/b', query 'c=d', fragment 'f' and + * port 8080. + */ public function testItExposesTheParsedComponents(): void { $url = new Url('https://user@a.example.com:8080/a/b?c=d#f'); @@ -40,6 +47,12 @@ public function testItExposesTheParsedComponents(): void $this->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'); @@ -48,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'; @@ -55,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( @@ -63,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( @@ -71,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( @@ -79,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')); @@ -93,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()); @@ -109,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 { @@ -148,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()); @@ -175,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']); @@ -188,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'); @@ -199,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 { @@ -207,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 { @@ -218,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 { @@ -250,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'); @@ -259,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 { @@ -272,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 b0124a68149..5739723a20d 100644 --- a/tests/Unit/UtilsTest.php +++ b/tests/Unit/UtilsTest.php @@ -27,11 +27,22 @@ class UtilsTest extends TestCase * Public methods ****************/ + /** + * Strings that start the same share one copy of that prefix in the regex. + * + * Expected: buildRegex(['abc', 'abd']) returns '(?>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)']; @@ -42,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   ')); @@ -85,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)); @@ -94,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 @@ -114,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( @@ -128,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. @@ -135,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'; @@ -162,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 19d4992515c..1241d4db279 100644 --- a/tests/Unit/UuidTest.php +++ b/tests/Unit/UuidTest.php @@ -33,6 +33,11 @@ class UuidTest extends TestCase * Public methods ****************/ + /** + * A generated UUID is written as five lowercase hex groups. + * + * Expected: (string) Uuid::create(4) matches 8-4-4-4-12 lowercase hex digits. + */ public function testGeneratedUuidsLookLikeUuids(): void { $this->assertMatchesRegularExpression( @@ -41,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); @@ -60,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); @@ -79,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); @@ -86,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); @@ -93,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. @@ -117,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 { @@ -144,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 { @@ -159,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 94de8348ff3..66266966561 100644 --- a/tests/Unit/WebFetchResultTest.php +++ b/tests/Unit/WebFetchResultTest.php @@ -30,14 +30,21 @@ class WebFetchResultTest extends TestCase ****************/ /** + * result() returns null when nothing has been fetched. + * * A fetcher that was never asked for anything, or one whose request was * refused before a connection was attempted, has nothing in $response. - * result() worked out the last index as count() - 1, which is -1, and read - * that. WebFetchApi::fetch() asks for result('success') on exactly that - * path, so every refused fetch emitted two warnings on its way to - * returning false. - * + * WebFetchApi::fetch() asks for result('success') on exactly that path. * The suite fails on warnings, so this needs no assertion about them. + * + * Expected: result('success'), result('body') and result() on a new + * CurlFetcher and on a new SocketFetcher all return null. + * Guards: result() worked out the last index as count() - 1, which is -1, + * and read that, so every refused fetch emitted two warnings on + * its way to returning false. + * + * @link https://github.com/SimpleMachines/SMF/commit/4846a993a Introduced by "Implements SMF\WebFetch\WebFetchApi and related classes" + * @link https://github.com/SimpleMachines/SMF/pull/9535 */ #[DataProvider('fetcherProvider')] public function testResultIsNullWhenNothingWasFetched(string $class): void diff --git a/tests/Unit/XmlArrayTest.php b/tests/Unit/XmlArrayTest.php index 2b8fd028bf6..9ab67857fea 100644 --- a/tests/Unit/XmlArrayTest.php +++ b/tests/Unit/XmlArrayTest.php @@ -43,6 +43,14 @@ class XmlArrayTest extends TestCase * Public methods ****************/ + /** + * Every accessor agrees about a path that matches elements. + * + * Expected: for each case in providePathCases(), the path exists, path() + * returns an XmlArray, count() is the expected number, to_array() + * is a non-empty array, set() is a list of that many XmlArray + * instances, and create_xml() returns a string. + */ #[DataProvider('providePathCases')] public function testPath(string $path, int $count): void { @@ -59,6 +67,14 @@ public function testPath(string $path, int $count): void $this->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 { @@ -75,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); @@ -112,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); @@ -131,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( @@ -149,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);