diff --git a/.docker/README.md b/.docker/README.md index ff2ea19733..a7aa96eab7 100644 --- a/.docker/README.md +++ b/.docker/README.md @@ -165,6 +165,196 @@ compose network. When the installer finishes, delete `install.php` from the repo root — while it exists, `Settings.php` redirects every request back into the installer. +## Running the tests + +```sh +.docker/test.sh # both engines +.docker/test.sh --engine postgresql +.docker/test.sh --engine both --filter ModSettings +``` + +Anything it does not recognise is passed on to PHPUnit. It installs a forum for +an engine that has not got one, and puts the previously active engine back when +it finishes. + +Running on both is the point rather than a thoroughness exercise. The counter +regression in `tests/Integration/ModSettingsTest.php` **passes on MySQL with the +bug still in place** and only fails on PostgreSQL, because MySQL coerces text to +a number where PostgreSQL refuses. A suite that only ever sees one engine proves +considerably less than it looks like it does. + +The unit suite needs none of this — `composer test` runs everything, and the +integration tests skip themselves when there is no forum to talk to. + +Some of the tests sign in, so they need to know the administrator. They default +to what `install-forum.sh` creates (`admin` / `password`); if your forum has +different credentials, export them: + +```sh +SMF_ADMIN_USER=admin SMF_ADMIN_PASS='…' .docker/test.sh +``` + +Getting that wrong makes those tests **skip**, with a message saying so, rather +than fail — a password the suite does not know is a misconfigured forum, not a +regression. `user.sh check admin '…'` settles which it is, and +`user.sh reset admin password` puts a forum installed some other way back on the +credentials the suite expects. + +## Writing a test + +### Which suite + +Three of them, and picking the wrong one is the usual reason a test is harder to +write than it should be: + +| Suite | Has | Use it for | +| ------------------------ | -------------------------------------- | ----------------------------- | +| `tests/Unit` | nothing — no database, no request | pure functions, value objects | +| `tests/Integration` | `Db::$db`, `$modSettings`, `User::$me` | anything needing real data | +| `tests/Integration/Http` | all of that, plus a real request | proving a *page* works | + +Work down the list and stop at the first that can hold the test. An HTTP test +costs about a second and cannot be rolled back; a unit test costs nothing. The +limits of the unit suite are spelled out in `AGENTS.md`. + +Reach for HTTP only when the thing worth proving is in the parts nothing else +touches: the session, the cookies, the security token, the theme and the +templates. `User::setMe()` skips every one of them, which is exactly why the +plain integration tests are cheap. + +### The shape of an HTTP test + +Four beats: fetch a page, submit a form on it, assert on what came back, assert +nothing was logged. + +```php +#[CoversNothing] +class ProfileTest extends HttpTestCase +{ + public function testAMemberCanChangeTheirSignature(): void + { + $this->signInAsAdmin(); + + $form = $this->fetch('?action=profile;area=forumprofile'); + + $response = $this->submitForm($form, [ + 'signature' => 'Set by the integration suite.', + 'save' => 'Change profile', // the button + ], '//form[contains(@action, "area=forumprofile")]'); + + $this->assertLessThan(400, $response->status, $response->errorText()); + $this->assertNoErrorsLogged('saving a signature logged something.' . "\n"); + } +} +``` + +`#[CoversNothing]` is not optional. These cross dozens of classes, so naming one +would be untrue, and `failOnRisky` wants an attribute either way. + +Four things that are easy to get wrong: + +- **Submit through `submitForm()`, not `HttpClient::submit()`.** Only the former + waits out flood control. `Security::spamProtection()` gives a moderator two + seconds between posts, per IP, and the tests all arrive from the same one far + faster than a person would; without the wait you get a suite that fails about + one run in five for no reproducible reason. +- **Name the button you are pressing.** `formFields()` leaves every button out on + purpose, because the posting form carries both `preview` and `post` and sending + the pair means preview quietly wins — no post, and a perfectly good 200 to show + for it. +- **Clean up whatever you write.** There is no transaction here; see + `HttpTestCase::usesTransaction()` for why one would not help. `PostingTest` + deletes through `Topic::remove()` rather than by hand, so the board and member + counters go back as well. +- **`assertNoErrorsLogged()` is the point of the test**, not a formality. A page + can return exactly the right HTML while logging an undefined index, and that is + the failure mode this whole suite exists to catch. + +One thing to rule out before believing a failure: if `install.php` is still in +the board root, SMF puts a "MAJOR SECURITY RISK" box on every page it shows an +administrator. That is an `errorbox`, so it fails `assertLooksLikeAForumPage()` +and turns up in `errorText()` in front of whatever the test was actually looking +at. `install-forum.sh` removes the file once it is done; a forum installed +through the browser needs it deleting by hand. + +### Who the request is + +The identity of an HTTP request is the cookie jar and nothing else. There are two +states out of the box: a guest, which is what `setUp()` leaves you, and the +administrator, through `signInAsAdmin()`. + +**`actingAs()` does not work here.** It is inherited from `IntegrationTestCase` +and it repoints `User::$me` in the PHPUnit process — but the request is handled +by Apache in a different process, which knows only the cookie. Calling it in an +HTTP test changes nothing about the request and leaves the assertions describing +a guest, confidently. + +So: + +- **Two users at once** means two `HttpClient` instances. Each opens its own + cookie jar, so they are independent browsers — which is how to test one member + sending another a PM. +- **Back to being a guest** is `$this->http->forgetCookies()`, then + `$this->http->get('')` to pick up a fresh session. +- **A member who is not the administrator** has to be made first, through + `Register2::registerMember()` with `interface => 'admin'` (which needs + `actingAs($this->adminId())` first, as it checks `moderate_forum`). That member + outlives the test, so delete it in `tearDown()`. + +### Finding the endpoint and the field names + +Endpoints are looked up; field names are not. + +`Forum::$actions` in `Sources/Forum.php` is the authoritative list of every +`?action=` the forum answers and the class behind it. Sub-actions — the `;area=` +and `;sa=` parts — are a `$subactions` property on that class. So +`?action=profile;area=forumprofile` resolves as `$actions['profile']` → +`Actions\Profile\Main` → its `$subactions`. That is quicker and more reliable +than reading templates. + +Field names you are deliberately not meant to know. `HttpClient::submit()` +scrapes every input, textarea and select out of the form it was handed and sends +them back, the way a browser does. That is what carries the session check and the +security token, both named unpredictably per session and neither hardcodable. All +a test supplies is the few values it is choosing, plus the button. + +When you do need to see them, ask the page rather than the template: + +```sh +docker compose exec web php -r ' + require "tests/bootstrap.php"; + $c = new SMF\Tests\Support\HttpClient(); + $c->get(""); + $p = $c->get("?action=login"); + print_r($p->formFields("//form[contains(@action, \"login2\")]"));' +``` + +``` +Array +( + [user] => + [passwrd] => + [d0004e1655] => b2f5189a9b3014cadee7bcb0b8d697f2 + [b8ae8fd32d] => 8f6026ed9a1b91bf6fec315801a2a93c +) +``` + +Two named fields, which are the ones a test writes, and two whose names are +different for every session — the session check and the security token, and +running the command twice gives two different pairs. That is what +`submit()` is for, and why a test that builds its own POST body by hand gets a +403 it cannot fix. + +The paths are relative because the container's working directory is +`/var/www/html` already. Spelling them absolutely also works, but not from Git +Bash on Windows, which rewrites anything that looks like a Unix path before +Docker sees it. + +Note the throwaway `get("")` before the form is fetched. The very first request +of a new session regenerates it, so a token minted on the first page a visitor +ever sees is bound to a session that no longer exists by the time it comes back. +The symptom is a 403 about the token, when the token was never the problem. + ## Running CI locally ```sh @@ -402,6 +592,7 @@ compose.yaml the stack .docker/reset.sh empty one engine and restage the installer .docker/use-engine.sh switch which installed forum is live .docker/user.sh inspect accounts, check and reset passwords +.docker/test.sh run the test suites against an installed forum .docker/upgrade-readings.sh shared: driving upgrade.php, reading a database .docker/rerun-upgrade.sh upgrade twice, report what the second run changed diff --git a/.docker/test.sh b/.docker/test.sh new file mode 100755 index 0000000000..8d6d818e20 --- /dev/null +++ b/.docker/test.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +# Runs the test suite against a real forum, on one engine or on both. +# +# .docker/test.sh both engines, whole suite +# .docker/test.sh --engine postgresql +# .docker/test.sh --engine both --filter ModSettings +# +# Anything after the recognised options is handed straight to PHPUnit, so +# --filter, --testsuite and friends work as usual. +# +# Installs a forum for an engine that has not got one yet. Use +# .docker/install-forum.sh --force to start any of them over. +# +# Running on both engines is the point rather than a thoroughness exercise: the +# two disagree often enough that a suite which only ever sees one of them +# proves considerably less than it appears to. The counter regression in +# tests/Integration/ModSettingsTest.php passes on MySQL with the bug still in +# place, and fails on PostgreSQL. +# +# Runs on the host. +set -euo pipefail + +. "$(dirname -- "${BASH_SOURCE[0]}")/lib.sh" + +ENGINE='both' +PHPUNIT_ARGS=() + +while [ $# -gt 0 ]; do + case "$1" in + --engine) ENGINE="$2"; shift 2 ;; + --engine=*) ENGINE="${1#*=}"; shift ;; + -h|--help) sed -n '2,19p' "${BASH_SOURCE[0]}"; exit 0 ;; + *) PHPUNIT_ARGS+=("$1"); shift ;; + esac +done + +ENGINES=$(engine_list "$ENGINE") || die "unknown engine: $ENGINE" + +cd "$BOARD_DIR" + +# Remember what was active, and put it back afterwards however this ends. A test +# run should not silently leave the forum pointed somewhere else. +ORIGINAL='' + +if [ -f Settings.php ]; then + ORIGINAL=$(sed -n "s|^\$db_type = '\([^']*\)';.*|\1|p" Settings.php | head -n 1 | tr '[:upper:]' '[:lower:]') +fi + +restore_engine() { + if [ -n "$ORIGINAL" ] && [ -f "$SETTINGS_DIR/Settings.$(engine_smf_type "$ORIGINAL").php" ]; then + "$DOCKER_DIR/use-engine.sh" "$ORIGINAL" >/dev/null 2>&1 || true + fi +} + +trap restore_engine EXIT + +FAILED='' + +for smf_type in $ENGINES; do + if [ -z "$(installed_version "$smf_type" || true)" ]; then + log "${smf_type}: no forum yet, installing one" + "$DOCKER_DIR/install-forum.sh" --engine "$smf_type" >/dev/null + fi + + "$DOCKER_DIR/use-engine.sh" "$smf_type" >/dev/null + + log "${smf_type}: running the tests" + + # The HTTP tests sign in, so they need to be told who the administrator is. + # These default to what install-forum.sh created; export them to point the + # suite at a forum that was set up some other way. + if docker compose exec -T \ + -e SMF_ADMIN_USER="$SMF_ADMIN_USER" \ + -e SMF_ADMIN_PASS="$SMF_ADMIN_PASS" \ + web vendor/bin/phpunit --no-coverage --colors=always "${PHPUNIT_ARGS[@]+"${PHPUNIT_ARGS[@]}"}"; then + log "${smf_type}: passed" + else + warn "${smf_type}: FAILED" + FAILED="${FAILED} ${smf_type}" + fi +done + +if [ -n "$FAILED" ]; then + die "failed on:${FAILED}" +fi + +log "passed on: ${ENGINES}" diff --git a/AGENTS.md b/AGENTS.md index cf8c29002a..08eb3ed4a1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -156,6 +156,7 @@ worked example in `tests/Unit/`: #### When it is not - Anything calling `Db::$db` — there is no connection, and faking one is not worth it. + This belongs in the integration suite below. - Anything reading `User::$me`, the session, `$_GET`/`$_POST`/`$_SERVER`, or expecting a loaded theme or `Utils::$context`. - Anything that emits output or sends headers. `beStrictAboutOutputDuringTests` is on, so @@ -164,6 +165,67 @@ worked example in `tests/Unit/`: `failOnRisky` and `failOnWarning` are on as well: a test that asserts nothing is a failure, not a pass. +### The integration suite + +`tests/Integration/` runs against a forum that is actually installed, so it reaches the +things above: `Db::$db`, `Config::$modSettings` as the database holds it, and `User::$me`. + +```bash +.docker/test.sh # both engines +.docker/test.sh --engine postgresql +``` + +`composer test` still runs everything. When there is no forum to talk to the integration +tests **skip** rather than fail, so it stays useful on a machine with no Docker. To get +one: `.docker/install-forum.sh --engine mysql`. + +Extend `SMF\Tests\Integration\IntegrationTestCase`, which gives you: + +- a transaction per test, rolled back afterwards, so tests do not have to order + themselves around each other; +- `actingAs($id)` and `adminId()` for a current user, via `User::setMe()` — the same seam + `Login2::DoLogin()` uses; +- `hook($name, $function)`, registered in `$modSettings` only, so it disappears with the + rollback; +- `assertNoErrorsLogged()`, which is usually the most valuable line in the test: SMF + records most of what goes wrong in `log_errors` rather than showing it, so a page that + returned the right thing while quietly logging an undefined index has still regressed; +- `queryRow()` and `rawSetting()`, which read past `$modSettings` and its cache and fail + with a readable message instead of a `TypeError` when a query fails. + +Two things the rollback does not cover: **DDL**, since MySQL commits implicitly on +`CREATE`/`ALTER`/`DROP`; and anything happening in another process, such as a request made +over HTTP, which runs on its own connection. + +#### HTTP tests + +`tests/Integration/Http/` drives the forum over the wire, through `HttpTestCase`. Use it +when the thing worth proving is that a *page* works: the session, the cookies, the theme +and the templates are all in the path, and none of them are otherwise reachable. + +They cannot use a transaction and do not try to - see `HttpTestCase::usesTransaction()` - +so a test that writes cleans up after itself. Four things about SMF make writing them +harder than it looks, all of them handled in the base class: + +- **Arrive at the forum before submitting anything.** The first request of a new session + regenerates it, so a security token minted on the very first page a visitor sees can + never be validated. The symptom is a 403 "Token verification failed" that looks like a + broken token rather than a replaced session. +- **Send the button you mean to press.** `HttpResponse::formFields()` deliberately leaves + buttons out. The posting form has both `preview` and `post`; submitting the pair means + preview wins, the post is never made, and the response is a perfectly ordinary 200. +- **Flood control will hit you.** `Security::spamProtection()` allows a moderator one + login or post every two seconds per IP, and tests are far faster than people. + `submitForm()` waits it out once rather than failing at random. +- **Quote `errorText()` in failure messages, not the body.** A fatal error in SMF is a + normal page, and its first few hundred characters are the menu. + +**Run both engines.** This is not thoroughness for its own sake — the two disagree often +enough to matter. `ModSettingsTest` pins a bug that *passes on MySQL with the bug still +in place*, because MySQL silently coerces text to a number where PostgreSQL refuses. +On PostgreSQL a failed query also poisons the rest of the transaction, so one swallowed +error turns every later query in the test into `false`. + #### Writing one `tests/Unit/Test.php`, namespace `SMF\Tests\Unit`, `declare(strict_types=1)`, diff --git a/composer.json b/composer.json index edb9014a7a..8c9352d4aa 100644 --- a/composer.json +++ b/composer.json @@ -21,6 +21,8 @@ }, "scripts": { "test": "phpunit --no-coverage", + "test-unit": "phpunit --no-coverage --testsuite unit", + "test-integration": "phpunit --no-coverage --testsuite integration", "lint": "php-cs-fixer --quiet check --config .php-cs-fixer.dist.php --path-mode=intersection $(git diff --name-only \"*.php\") --allow-risky=yes || php-cs-fixer check --diff --config .php-cs-fixer.dist.php --path-mode=intersection $(git diff --name-only \"*.php\") --allow-risky=yes", "lint-fix": "php-cs-fixer fix -v --config .php-cs-fixer.dist.php --path-mode=intersection $(git diff --name-only \"*.php\") --allow-risky=yes", "post-install-cmd": "php ./vendor/simplemachines/build-tools/secure-vendor-dir.php", diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 951db14dd1..720240fd3c 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -11,6 +11,14 @@ tests/Unit + + + tests/Integration + diff --git a/tests/Integration/HarnessTest.php b/tests/Integration/HarnessTest.php new file mode 100644 index 0000000000..ac7ceb8ffc --- /dev/null +++ b/tests/Integration/HarnessTest.php @@ -0,0 +1,140 @@ +assertNotEmpty(Config::$modSettings['smfVersion']); + $this->assertSame(SMF_VERSION, Config::$modSettings['smfVersion']); + } + + public function testTheEngineIsOneSmfSupports(): void + { + $this->assertContains( + strtolower(Config::$db_type), + ['mysql', 'postgresql'], + 'Settings.php names an engine this suite does not know about', + ); + } + + /** + * Writes a row the next test then looks for. Together with the test below, + * this is what proves the rollback in tearDown() is real. + */ + public function testAWriteIsVisibleInsideTheTestThatMadeIt(): void + { + Db::$db->insert( + 'replace', + '{db_prefix}settings', + ['variable' => 'string', 'value' => 'string'], + [[self::LEFTOVER, 'written']], + ['variable'], + ); + + $this->assertSame('written', $this->rawSetting(self::LEFTOVER)); + } + + #[Depends('testAWriteIsVisibleInsideTheTestThatMadeIt')] + public function testThatWriteIsGoneByTheNextTest(): void + { + $this->assertNull( + $this->rawSetting(self::LEFTOVER), + 'the previous test\'s write survived, so tests are not isolated', + ); + } + + 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); + } + + #[Depends('testModSettingsIsRestoredEvenThoughItIsAStaticArray')] + public function testModSettingsHasNoLeftoversFromTheLastTest(): void + { + $this->assertArrayNotHasKey('smf_tests_in_memory_only', Config::$modSettings); + } + + public function testAdminIdFindsAnAdministrator(): void + { + $id = $this->adminId(); + + $this->assertGreaterThan(0, $id); + + $this->actingAs($id); + + $this->assertSame($id, \SMF\User::$me->id); + $this->assertTrue(\SMF\User::$me->is_admin, 'actingAs() did not produce an administrator'); + } + + public function testNothingIsLoggedByAnEmptyTest(): void + { + $this->assertNoErrorsLogged(); + } + + /** + * 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. + */ + public function testAssertNoErrorsLoggedNoticesALoggedError(): void + { + Db::$db->insert( + 'insert', + '{db_prefix}log_errors', + [ + 'log_time' => 'int', + 'id_member' => 'int', + 'ip' => 'inet', + 'url' => 'string', + 'message' => 'string', + 'session' => 'string', + 'error_type' => 'string', + 'file' => 'string', + 'line' => 'int', + 'backtrace' => 'string', + ], + [[time(), 0, '', '', 'canary', '', 'general', __FILE__, __LINE__, '[]']], + ['id_error'], + ); + + $this->expectException(AssertionFailedError::class); + + $this->assertNoErrorsLogged(); + } +} diff --git a/tests/Integration/Http/GuestPagesTest.php b/tests/Integration/Http/GuestPagesTest.php new file mode 100644 index 0000000000..cf55b2226f --- /dev/null +++ b/tests/Integration/Http/GuestPagesTest.php @@ -0,0 +1,168 @@ +fetch($path); + + $this->assertLooksLikeAForumPage($response, $name); + $this->assertNoErrorsLogged($name . ' (' . $path . ') logged something.' . "\n"); + } + + public function testTheBoardIndexListsAtLeastOneBoard(): void + { + $response = $this->fetch(''); + + $this->assertGreaterThan( + 0, + $response->xpath('//a[contains(@href, "board=")]')->length, + 'the board index links to no boards, so a fresh install has nothing in it', + ); + + $this->assertNoErrorsLogged(); + } + + /** + * 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. + */ + public function testTheFeedIsXmlAndParses(): void + { + $response = $this->fetch('?action=.xml;type=rss2'); + + $this->assertStringContainsString( + 'xml', + strtolower($response->headers['content-type'] ?? ''), + 'the feed did not come back as XML', + ); + + $previous = libxml_use_internal_errors(true); + $parsed = simplexml_load_string($response->body); + libxml_clear_errors(); + libxml_use_internal_errors($previous); + + $this->assertNotFalse($parsed, 'the feed is not well formed XML'); + $this->assertNoErrorsLogged('the feed logged something.' . "\n"); + } + + /** + * An action that does not exist should be a 404, not a 200 with an apology + * and not a 500. + */ + public function testAnUnknownActionIsNotFound(): void + { + $this->fetch('?action=smf_tests_no_such_action', 404); + } + + /** + * Registration refuses to start for a visitor who sends no cookies at all, + * because a registration that cannot keep a session cannot be completed. + * + * It is in its own test rather than the sweep above because it is the one + * 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. + */ + public function testRegistrationNeedsASessionFirst(): void + { + $this->http->forgetCookies(); + + $cold = $this->http->get('?action=signup'); + + $this->assertSame(403, $cold->status, 'a cookieless visitor was allowed into registration'); + + // Arrive at the forum the way a person would, which sets the session + // cookie, and then go to register. + $this->fetch(''); + + $agreement = $this->fetch('?action=signup'); + + $this->assertLooksLikeAForumPage($agreement, 'the registration agreement'); + + // requireAgreement is on by default, so step one is the agreement rather + // than the form. Note the form has to be named: the first form on any SMF + // page is the search box in the header. + $registration_form = '//form[contains(@action, "action=signup")]'; + + $this->assertGreaterThan( + 0, + $agreement->xpath($registration_form . '//input[@name="accept_agreement"]')->length, + 'registration did not start at the agreement', + ); + + // Buttons are not submitted unless named, so say which one we press. + $form = $this->http->submit($agreement, [ + 'accept_agreement' => 'I accept the terms of the agreement.', + ], $registration_form); + + $this->assertSame(200, $form->status, 'accepting the agreement returned ' . $form->status); + + $this->assertGreaterThan( + 0, + $form->xpath('//input[@name="user"]')->length, + 'accepting the agreement did not lead to the registration form', + ); + + $this->assertNoErrorsLogged('registering logged something.' . "\n"); + } + + /*********************** + * Public static methods + ***********************/ + + /** + * The pages a guest can reach on a stock install. + * + * Deliberately only actions that a fresh forum can serve without any content + * having been created and without being logged in, so this stays green on a + * forum straight out of .docker/install-forum.sh. + * + * @return array The cases, path and a readable name. + */ + public static function guestPages(): array + { + return [ + 'board index' => ['', 'the board index'], + 'help' => ['?action=help', 'help'], + 'login form' => ['?action=login', 'the login form'], + 'recent posts' => ['?action=recent', 'recent posts'], + 'unread' => ['?action=unread', 'unread posts'], + 'search form' => ['?action=search', 'the search form'], + 'member list' => ['?action=mlist', 'the member list'], + 'statistics' => ['?action=stats', 'the statistics page'], + 'credits' => ['?action=credits', 'the credits page'], + 'who is online' => ['?action=who', 'who is online'], + 'agreement' => ['?action=agreement', 'the registration agreement'], + 'first board' => ['?board=1.0', 'the first board'], + ]; + } +} diff --git a/tests/Integration/Http/HttpTestCase.php b/tests/Integration/Http/HttpTestCase.php new file mode 100644 index 0000000000..a85e423c6c --- /dev/null +++ b/tests/Integration/Http/HttpTestCase.php @@ -0,0 +1,274 @@ +http = new HttpClient(); + + // Arrive at the forum before doing anything else, which is what a person + // does and what the tests below depend on. + // + // The very first request of a new session regenerates it - SMF sets a + // guest login cookie, and Cookie::setLoginCookie() throws the session + // away and starts another whenever that value changes. Anything minted + // earlier in that same request is minted against the session that just + // went away, so a security token taken from the first page a visitor + // ever sees can never be validated. Posting that form comes back 403, + // "Token verification failed", with nothing to suggest the token was + // fine and the session underneath it was not. + $this->http->get(''); + } + + /** + * Signs in as the forum administrator. + * + * The credentials are the ones .docker/install-forum.sh uses, overridable + * through the environment for a forum that was set up some other way. + * + * @return HttpResponse The response to the login post. + */ + protected function signInAsAdmin(): HttpResponse + { + $response = $this->attemptSignIn(); + + if (self::isThrottled($response)) { + sleep(self::FLOOD_WAIT); + + $response = $this->attemptSignIn(); + } + + // A password that does not match is a misconfigured forum rather than a + // regression, and failing every test in the file over it would say + // nothing useful. Skipping names the variable to set. + if (str_contains($response->text(), 'username or password you entered is incorrect')) { + self::markTestSkipped( + 'cannot sign in as "' . self::adminName() . '". Set SMF_ADMIN_USER and ' + . 'SMF_ADMIN_PASS to this forum\'s administrator, or reinstall with ' + . '.docker/install-forum.sh --engine mysql --force', + ); + } + + $this->assertLessThan( + 400, + $response->status, + 'logging in returned ' . $response->status . ': ' . $response->errorText(), + ); + + return $response; + } + + /** + * Submits a form, waiting out flood control if it gets in the way. + * + * Tests do in half a second what a person would take a minute over, so they + * trip SMF's flood protection routinely. That is the forum working, not a + * regression, and the difference between a suite people trust and one that + * fails now and then for reasons nobody can reproduce. + * + * @param HttpResponse $page The page holding the form. + * @param array $overrides Values to change or add, including the button. + * @param string $xpath Which form. + * @return HttpResponse The response. + */ + protected function submitForm(HttpResponse $page, array $overrides, string $xpath): HttpResponse + { + $response = $this->http->submit($page, $overrides, $xpath); + + if (!self::isThrottled($response)) { + return $response; + } + + sleep(self::FLOOD_WAIT); + + // The page has to be fetched again rather than resubmitted: its security + // token was spent on the attempt that just bounced. + return $this->http->submit($this->http->get($page->url), $overrides, $xpath); + } + + /** + * Asserts the client is, or is not, signed in. + * + * Uses the logout link, which the theme only renders for a member. + * + * @param bool $expected Whether we should be signed in. + * @param string $message What was being checked. + */ + protected function assertSignedIn(bool $expected, string $message = ''): void + { + $signed_in = $this->fetch('')->xpath('//a[contains(@href, "action=logout")]')->length > 0; + + $this->assertSame($expected, $signed_in, $message !== '' ? $message : ($expected ? 'not signed in' : 'still signed in')); + } + + /** + * Fetches a page and asserts it came back whole. + * + * @param string $path Where to go, as HttpClient::get() takes it. + * @param int $expected The status it should return. + * @return HttpResponse The response, for further assertions. + */ + protected function fetch(string $path, int $expected = 200): HttpResponse + { + $response = $this->http->get($path); + + $this->assertSame( + $expected, + $response->status, + $path . ' returned ' . $response->status . ' from ' . $this->http->base_url, + ); + + return $response; + } + + /** + * Asserts a page is a real forum page rather than an error SMF rendered + * with a 200. + * + * A fatal error in SMF is a normal page with an apologetic message in it, so + * the status code alone proves very little. + * + * @param HttpResponse $response The response to check. + * @param string $where What was being fetched, for the failure message. + */ + protected function assertLooksLikeAForumPage(HttpResponse $response, string $where): void + { + $this->assertNotSame('', $response->title(), $where . ' has no '); + + $this->assertGreaterThan( + 0, + $response->xpath('//div[@id="footer"] | //footer | //*[@id="bot"]')->length, + $where . ' has no footer, so the template did not finish rendering', + ); + + $this->assertSame( + 0, + $response->xpath('//*[contains(@class, "errorbox")]')->length, + $where . ' rendered an error box: ' . $response->errorText(), + ); + } + + /** + * One go at the login form. + * + * @return HttpResponse The response to the post. + */ + private function attemptSignIn(): HttpResponse + { + $form = $this->fetch('?action=login'); + + return $this->http->submit($form, [ + 'user' => self::adminName(), + 'passwrd' => self::adminPassword(), + ], '//form[contains(@action, "action=login2")]'); + } + + /************************* + * Internal static methods + *************************/ + + /** + * Whether a response is SMF turning us away for going too fast. + * + * @param HttpResponse $response The response to look at. + * @return bool Whether flood control rejected it. + */ + protected static function isThrottled(HttpResponse $response): bool + { + $error = $response->errorText(); + + return str_contains($error, 'You will have to wait') + || str_contains($error, 'The last posting from your IP'); + } +} diff --git a/tests/Integration/Http/LoginTest.php b/tests/Integration/Http/LoginTest.php new file mode 100644 index 0000000000..48b7ba7755 --- /dev/null +++ b/tests/Integration/Http/LoginTest.php @@ -0,0 +1,99 @@ +<?php + +declare(strict_types=1); + +namespace SMF\Tests\Integration\Http; + +use PHPUnit\Framework\Attributes\CoversNothing; +use SMF\Config; + +/** + * Signing in over HTTP. + * + * Worth testing this way rather than through User::setMe(), which is what the + * rest of the integration suite uses: the parts most likely to break are exactly + * the ones setMe() skips. The session has to survive between requests, the + * security token minted with the form has to still be valid when it comes back, + * the cookie has to be signed with $auth_secret, and a post that arrives without + * a session check has to be turned away. + */ +#[CoversNothing] +class LoginTest extends HttpTestCase +{ + /**************** + * Public methods + ****************/ + + public function testTheAdministratorCanSignIn(): void + { + $this->signInAsAdmin(); + + $this->assertSignedIn(true, 'the session did not survive the login redirect'); + $this->assertNoErrorsLogged('signing in logged something.' . "\n"); + } + + /** + * The cookie is what carries the login between requests, so it is worth + * checking it was issued rather than inferring it from the page changing. + */ + public function testSigningInIssuesTheForumCookie(): void + { + $response = $this->signInAsAdmin(); + + $this->assertNotEmpty($response->set_cookies, 'logging in set no cookie at all'); + + // Note the plural: the response carries the session cookie as well, and + // keeping only the last one would test whichever happened to come second. + $this->assertStringContainsString( + (string) Config::$cookiename, + implode("\n", $response->set_cookies), + 'the forum cookie was not among those set: ' . implode(' | ', $response->set_cookies), + ); + } + + public function testTheWrongPasswordDoesNotSignAnyoneIn(): void + { + $form = $this->fetch('?action=login'); + + $this->http->submit($form, [ + 'user' => self::adminName(), + 'passwrd' => 'definitely not the password', + ], '//form[contains(@action, "action=login2")]'); + + $this->assertSignedIn(false, 'a wrong password signed us in anyway'); + } + + /** + * 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. + */ + public function testAPostWithoutTheSessionCheckIsRejected(): void + { + // Hand built rather than submitted from the form, so none of the session + // fields the form carries are included. + $this->http->post('?action=login2', [ + 'user' => self::adminName(), + 'passwrd' => self::adminPassword(), + ]); + + $this->assertSignedIn(false, 'a login with no session check was accepted'); + } + + public function testSigningOutEndsTheSession(): void + { + $this->signInAsAdmin(); + $this->assertSignedIn(true); + + $page = $this->fetch(''); + $logout = $page->xpath('//a[contains(@href, "action=logout")]')->item(0); + + $this->assertNotNull($logout, 'no logout link to follow'); + + // The link carries its own session check in the query string. + $this->http->get((string) $logout?->attributes?->getNamedItem('href')?->nodeValue); + + $this->assertSignedIn(false, 'still signed in after logging out'); + $this->assertNoErrorsLogged('logging out logged something.' . "\n"); + } +} diff --git a/tests/Integration/Http/PostingTest.php b/tests/Integration/Http/PostingTest.php new file mode 100644 index 0000000000..8240481b32 --- /dev/null +++ b/tests/Integration/Http/PostingTest.php @@ -0,0 +1,222 @@ +<?php + +declare(strict_types=1); + +namespace SMF\Tests\Integration\Http; + +use PHPUnit\Framework\Attributes\CoversNothing; +use SMF\Topic; + +/** + * Starting a topic and replying to it, over HTTP. + * + * This is the journey the forum exists for, and the one with the most behind it: + * the editor, the session check, the security token, permissions, the post + * itself, and then every counter and index SMF updates afterwards. Nothing short + * of a real request covers that. + * + * These tests write, and an HTTP test cannot be rolled back - the request runs in + * the web server's process on its own connection. So whatever they create, they + * remove again in tearDown through SMF's own Topic::remove(), which puts the + * board and member counts back the way deleting a topic in the browser would. + */ +#[CoversNothing] +class PostingTest extends HttpTestCase +{ + /********************* + * Internal properties + *********************/ + + /** + * @var array Topics this test created, to be removed afterwards. + */ + private array $created_topics = []; + + /**************** + * Public methods + ****************/ + + public function testStartingATopicAndReplyingToIt(): void + { + $this->signInAsAdmin(); + + $subject = 'Integration test topic ' . bin2hex(random_bytes(6)); + $body = 'Posted by the integration suite at ' . date('c') . '.'; + + $topic_id = $this->startTopic($subject, $body); + + $topic = $this->fetch('?topic=' . $topic_id . '.0'); + + $this->assertStringContainsString( + $subject, + $topic->text(), + 'the new topic does not show its own subject', + ); + + $this->assertStringContainsString($body, $topic->text(), 'the post body is missing'); + + // And now a reply, which goes through a different form on a different + // page and updates a different set of counters. + $reply = 'A reply from the integration suite.'; + + $form = $this->fetch('?action=post;topic=' . $topic_id . '.0'); + + $posted = $this->submitForm($form, [ + 'subject' => 'Re: ' . $subject, + 'message' => $reply, + 'post' => 'Post', + ], '//form[contains(@action, "action=post2")]'); + + $this->assertLessThan( + 400, + $posted->status, + 'replying returned ' . $posted->status . ': ' . $posted->errorText(), + ); + + $after = $this->fetch('?topic=' . $topic_id . '.0'); + + $this->assertStringContainsString($reply, $after->text(), 'the reply is not on the topic'); + + $this->assertSame( + 2, + $this->countMessages($topic_id), + 'the topic should hold the first post and the reply', + ); + + $this->assertNoErrorsLogged('posting logged something.' . "\n"); + } + + /** + * A guest cannot post on a stock install, and the forum should say so rather + * than accept it. + */ + public function testAGuestCannotStartATopic(): void + { + $before = $this->countTopicsInBoard(1); + + $this->http->post('?action=post2;board=1', [ + 'subject' => 'Integration test guest post', + 'message' => 'This should not be accepted.', + ]); + + $this->assertSame( + $before, + $this->countTopicsInBoard(1), + 'a guest with no session check managed to start a topic', + ); + } + + /****************** + * Internal methods + ******************/ + + protected function tearDown(): void + { + // Before the parent runs, while the connection is still ours. + if ($this->created_topics !== []) { + Topic::remove($this->created_topics); + + $this->created_topics = []; + } + + parent::tearDown(); + } + + /** + * Starts a topic in the first board and returns its id. + * + * @param string $subject The subject. + * @param string $body The message. + * @return int The new topic's id. + */ + private function startTopic(string $subject, string $body): int + { + $before = $this->latestTopicId(); + + $form = $this->fetch('?action=post;board=1.0'); + + $this->assertGreaterThan( + 0, + $form->xpath('//form[contains(@action, "action=post2")]')->length, + 'there is no posting form on the new topic page', + ); + + $posted = $this->submitForm($form, [ + 'subject' => $subject, + 'message' => $body, + // The button we are pressing. Without it the form's other button, + // "preview", is the one SMF acts on. + 'post' => 'Post', + ], '//form[contains(@action, "action=post2")]'); + + $this->assertLessThan( + 400, + $posted->status, + 'posting returned ' . $posted->status . ': ' . $posted->errorText(), + ); + + $topic_id = $this->latestTopicId(); + + // Quote the page when this fails. SMF answers a rejected post with a + // perfectly ordinary 200 and the reason in a box, so without this the + // only evidence is a topic id that did not move. + $this->assertGreaterThan( + $before, + $topic_id, + 'no new topic appeared after posting. The forum said: ' + . ($posted->errorText() !== '' ? $posted->errorText() : '(nothing) - page title "' . $posted->title() . '"'), + ); + + $this->created_topics[] = $topic_id; + + return $topic_id; + } + + /** + * The highest topic id in the forum. + * + * @return int The id, or 0 when there are no topics. + */ + private function latestTopicId(): int + { + $row = $this->queryRow('SELECT COALESCE(MAX(id_topic), 0) AS id FROM {db_prefix}topics'); + + return (int) ($row['id'] ?? 0); + } + + /** + * How many messages a topic holds. + * + * @param int $topic_id The topic. + * @return int The number of messages. + */ + private function countMessages(int $topic_id): int + { + $row = $this->queryRow( + 'SELECT COUNT(*) AS total + FROM {db_prefix}messages + WHERE id_topic = {int:topic}', + ['topic' => $topic_id], + ); + + return (int) ($row['total'] ?? 0); + } + + /** + * How many topics a board holds. + * + * @param int $board_id The board. + * @return int The number of topics. + */ + private function countTopicsInBoard(int $board_id): int + { + $row = $this->queryRow( + 'SELECT COUNT(*) AS total + FROM {db_prefix}topics + WHERE id_board = {int:board}', + ['board' => $board_id], + ); + + return (int) ($row['total'] ?? 0); + } +} diff --git a/tests/Integration/Http/index.php b/tests/Integration/Http/index.php new file mode 100644 index 0000000000..2844a3b9e7 --- /dev/null +++ b/tests/Integration/Http/index.php @@ -0,0 +1,8 @@ +<?php + +// Try to handle it with the upper level index.php. (it should know what to do.) +if (file_exists(dirname(__DIR__) . DIRECTORY_SEPARATOR . 'index.php')) { + include dirname(__DIR__) . DIRECTORY_SEPARATOR . 'index.php'; +} else { + exit; +} diff --git a/tests/Integration/Installation.php b/tests/Integration/Installation.php new file mode 100644 index 0000000000..4505b2675f --- /dev/null +++ b/tests/Integration/Installation.php @@ -0,0 +1,111 @@ +<?php + +declare(strict_types=1); + +namespace SMF\Tests\Integration; + +use SMF\Config; +use SMF\Db\DatabaseApi as Db; +use SMF\Infrastructure\Container; + +/** + * Finds the forum the integration suite runs against. + * + * The unit bootstrap deliberately stops short of a database. This carries on + * from where it left off - reads Settings.php, connects, loads $modSettings - + * but only when an integration test actually asks, so `composer test` on a + * machine with no forum still costs nothing. + * + * It never throws. When there is nothing to test against it says why, and + * IntegrationTestCase turns that into a skip rather than a failure. + * + * To get a forum: .docker/install-forum.sh --engine mysql + */ +final class Installation +{ + /**************************** + * Internal static properties + ****************************/ + + /** + * @var string|null Why the forum is unusable, '' when it is fine, null + * before anything has looked. + */ + private static ?string $unavailable = null; + + /*********************** + * Public static methods + ***********************/ + + /** + * Connects, once per process. + * + * @return string Empty when there is a forum to test against, otherwise the + * reason there is not. + */ + public static function unavailableReason(): string + { + // Db::load() hands back the connection it already made and there is no + // way to drop it, so this can only usefully run once anyway. + if (self::$unavailable === null) { + self::$unavailable = self::connect(); + } + + return self::$unavailable; + } + + /************************* + * Internal static methods + *************************/ + + /** + * Does the actual work of unavailableReason(). + * + * @return string Empty on success, otherwise the reason. + */ + private static function connect(): string + { + if (!is_file(SMF_SETTINGS_FILE) || filesize(SMF_SETTINGS_FILE) === 0) { + return 'there is no Settings.php'; + } + + try { + Config::load(); + } catch (\Throwable $e) { + return 'Settings.php could not be loaded: ' . $e->getMessage(); + } + + if (empty(Config::$db_type) || empty(Config::$db_name)) { + return 'Settings.php names no database'; + } + + // index.php builds this before anything can ask for a service. + Container::init(); + + try { + // non_fatal, or a refused connection ends the process with SMF's own + // database error page instead of letting us report it here. + Db::load(['non_fatal' => true]); + } catch (\Throwable $e) { + return 'could not connect to ' . Config::$db_type . ': ' . $e->getMessage(); + } + + if (!isset(Db::$db->connection)) { + return 'could not connect to ' . Config::$db_type . ' as ' . Config::$db_user; + } + + // Connecting is not the same as finding a forum: the dev environment + // writes a Settings.php long before anything is installed behind it. + try { + Config::reloadModSettings(); + } catch (\Throwable $e) { + return 'the database holds no forum: ' . $e->getMessage(); + } + + if (empty(Config::$modSettings['smfVersion'])) { + return 'the database holds no forum (no smfVersion in ' . Config::$db_prefix . 'settings)'; + } + + return ''; + } +} diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php new file mode 100644 index 0000000000..b540e9158e --- /dev/null +++ b/tests/Integration/IntegrationTestCase.php @@ -0,0 +1,290 @@ +<?php + +declare(strict_types=1); + +namespace SMF\Tests\Integration; + +use PHPUnit\Framework\TestCase; +use SMF\Config; +use SMF\Db\DatabaseApi as Db; +use SMF\IntegrationHook; +use SMF\User; + +/** + * Base class for tests that need a real, installed forum. + * + * Everything here exists because the unit suite cannot have it: a database + * connection, $modSettings as the forum actually stores it, and a current user. + * + * Each test runs inside a transaction that is rolled back afterwards, so tests + * can write freely without ordering themselves around each other. Two things + * that does not cover: + * + * - DDL. MySQL commits implicitly on CREATE, ALTER and DROP, so a test that + * changes the schema has to put it back itself. + * - Anything a test causes to happen in another process, such as a request made + * over HTTP. That runs on its own connection and will not see the open + * transaction, nor be undone by the rollback. + */ +abstract class IntegrationTestCase extends TestCase +{ + /********************* + * Internal properties + *********************/ + + /** + * @var array Copy of Config::$modSettings taken before the test ran. + */ + private array $mod_settings_backup = []; + + /** + * @var int Highest id_error before the test ran. + */ + private int $error_watermark = 0; + + /*********************** + * Public static methods + ***********************/ + + public static function setUpBeforeClass(): void + { + $reason = Installation::unavailableReason(); + + if ($reason !== '') { + self::markTestSkipped( + 'no forum to test against: ' . $reason + . '. Run .docker/install-forum.sh --engine mysql', + ); + } + } + + /****************** + * Internal methods + ******************/ + + /** + * Whether to wrap the test in a transaction that is rolled back afterwards. + * + * Override and return false when the test causes work to happen in another + * process - a request made over HTTP, say. That runs on its own connection, + * so the transaction cannot undo it, and on MySQL, whose default isolation + * level is REPEATABLE READ, this connection would go on reading the snapshot + * it took before the request and never see what the request wrote. + * + * @return bool True to use a transaction, which is what most tests want. + */ + protected function usesTransaction(): bool + { + return true; + } + + protected function setUp(): void + { + parent::setUp(); + + if ($this->usesTransaction()) { + Db::$db->transaction('begin'); + } + + // $modSettings is a plain static array, so a test that calls + // updateModSettings() changes it for everything that runs after it. The + // rollback puts the table back, not the copy in memory. + $this->mod_settings_backup = Config::$modSettings; + + $this->error_watermark = $this->lastErrorId(); + } + + protected function tearDown(): void + { + if ($this->usesTransaction()) { + Db::$db->transaction('rollback'); + } + + Config::$modSettings = $this->mod_settings_backup; + + // Hooks added with permanent: false live in $modSettings, so restoring it + // above has already removed them. This only puts the switch back. + IntegrationHook::$enabled = true; + + parent::tearDown(); + } + + /** + * Becomes the given member for the rest of the test. + * + * This is the seam Login2::DoLogin() itself uses once it has checked the + * password, so everything downstream - permissions, bans, logging - behaves + * as it would for a real login, with no cookie and no request involved. + * + * Note that User::$me is a typed static and cannot be unset once assigned, + * so this outlives the test. Say who you are rather than assuming. + * + * @param int $id The member to become. + */ + protected function actingAs(int $id): void + { + User::setMe($id); + } + + /** + * The id of an administrator, for actingAs(). + * + * @return int The lowest member id in group 1. + */ + protected function adminId(): int + { + $request = Db::$db->query( + 'SELECT id_member + FROM {db_prefix}members + WHERE id_group = {int:admin_group} + ORDER BY id_member + LIMIT 1', + [ + 'admin_group' => 1, + ], + ); + + $row = Db::$db->fetch_assoc($request); + Db::$db->free_result($request); + + $this->assertNotEmpty($row, 'the forum has no administrator'); + + return (int) $row['id_member']; + } + + /** + * Registers a hook for the duration of the test. + * + * permanent: false keeps it in Config::$modSettings and out of the database, + * so tearDown() removes it by restoring that array. + * + * @param string $name The hook to add to, e.g. 'integrate_verify_user'. + * @param string $function The callable, in any form Utils::getCallable() takes. + */ + protected function hook(string $name, string $function): void + { + IntegrationHook::add($name, $function, false); + } + + /** + * Asserts the forum logged nothing since the test started. + * + * Most of what goes wrong in SMF is recorded here rather than shown, so a + * page that returned the right thing while quietly logging an undefined index + * has still regressed. + * + * @param string $message Optional context for the failure. + */ + protected function assertNoErrorsLogged(string $message = ''): void + { + $request = Db::$db->query( + 'SELECT error_type, message, file, line + FROM {db_prefix}log_errors + WHERE id_error > {int:watermark} + ORDER BY id_error', + [ + 'watermark' => $this->error_watermark, + ], + ); + + $this->assertNotFalse( + $request, + rtrim($message . "\n") . 'could not read the error log, so this proves nothing', + ); + + $errors = []; + + while ($row = Db::$db->fetch_assoc($request)) { + $errors[] = \sprintf( + ' [%s] %s (%s:%d)', + $row['error_type'], + html_entity_decode((string) $row['message'], ENT_QUOTES | ENT_HTML5, 'UTF-8'), + $row['file'], + $row['line'], + ); + } + + Db::$db->free_result($request); + + $this->assertSame( + [], + $errors, + rtrim($message . "\n") . 'the forum logged ' . \count($errors) . " error(s):\n" . implode("\n", $errors), + ); + } + + /** + * Runs a query and returns its first row. + * + * Exists because a failed query is not an exception here. MySQL returns + * false and carries on; PostgreSQL returns false and additionally puts the + * surrounding transaction into a failed state, so every later query in the + * same test returns false too. Handing that false to fetch_assoc() produces + * a TypeError about argument #1, which says nothing about what went wrong. + * + * @param string $sql The query, in SMF's dialect. + * @param array $params Its parameters. + * @return array|null The first row, or null when there were none. + */ + protected function queryRow(string $sql, array $params = []): ?array + { + $request = Db::$db->query($sql, $params); + + $this->assertNotFalse( + $request, + "the query failed:\n" . trim($sql) + . "\non PostgreSQL this also aborts the transaction, so every query after it fails too", + ); + + $row = Db::$db->fetch_assoc($request); + Db::$db->free_result($request); + + return \is_array($row) ? $row : null; + } + + /** + * Reads a setting straight out of the table. + * + * Bypasses Config::$modSettings and its cache, so what comes back is what + * the database actually holds rather than what the process believes. + * + * @param string $variable The setting to read. + * @return string|null The value, or null when there is no such row. + */ + protected function rawSetting(string $variable): ?string + { + $row = $this->queryRow( + 'SELECT value + FROM {db_prefix}settings + WHERE variable = {string:variable}', + [ + 'variable' => $variable, + ], + ); + + return $row === null ? null : (string) $row['value']; + } + + /** + * The highest id_error currently in the log. + * + * @return int The id, or 0 when nothing has ever been logged. + */ + private function lastErrorId(): int + { + $request = Db::$db->query( + 'SELECT COALESCE(MAX(id_error), 0) AS id_error + FROM {db_prefix}log_errors', + [], + ); + + if ($request === false) { + return 0; + } + + $row = Db::$db->fetch_assoc($request); + Db::$db->free_result($request); + + return (int) ($row['id_error'] ?? 0); + } +} diff --git a/tests/Integration/ModSettingsTest.php b/tests/Integration/ModSettingsTest.php new file mode 100644 index 0000000000..93a8312a96 --- /dev/null +++ b/tests/Integration/ModSettingsTest.php @@ -0,0 +1,123 @@ +<?php + +declare(strict_types=1); + +namespace SMF\Tests\Integration; + +use PHPUnit\Framework\Attributes\CoversMethod; +use SMF\Config; + +/** + * Config::updateModSettings() against a real database, on whichever engine is + * configured. + * + * The counter case is a regression test. Passing true or false as a value means + * "add one" or "take one away", and that was emitted as SET value = value + 1 + * against settings.value, which is a text column. MySQL coerces that silently; + * PostgreSQL rejects it outright, and because the PostgreSQL API discarded + * failed queries without a word, the statement simply did nothing. Every counter + * that goes through this path - totalMessages, totalTopics, totalMembers, + * unapprovedMembers - stayed frozen at whatever the last full recount produced. + * + * Nothing about that is reachable without a database, and it only shows up on + * one of the two engines, which is exactly the gap this suite exists to close. + */ +#[CoversMethod(Config::class, 'updateModSettings')] +class ModSettingsTest extends IntegrationTestCase +{ + /***************** + * Class constants + *****************/ + + private const COUNTER = 'smf_tests_counter'; + + /**************** + * Public methods + ****************/ + + public function testWritesAValue(): void + { + Config::updateModSettings([self::COUNTER => '41']); + + $this->assertSame('41', $this->rawSetting(self::COUNTER)); + $this->assertSame('41', Config::$modSettings[self::COUNTER]); + } + + public function testIncrementsACounter(): void + { + Config::updateModSettings([self::COUNTER => '41']); + Config::updateModSettings([self::COUNTER => true], true); + + $this->assertSame( + 42, + (int) $this->rawSetting(self::COUNTER), + 'the counter did not increment - on PostgreSQL this means the ' + . 'arithmetic was rejected and the failure swallowed', + ); + } + + public function testDecrementsACounter(): void + { + Config::updateModSettings([self::COUNTER => '41']); + Config::updateModSettings([self::COUNTER => false], true); + + $this->assertSame(40, (int) $this->rawSetting(self::COUNTER)); + } + + /** + * The value has to stay something the next increment can read back, so a + * cast that leaves '42.0000' behind is not good enough. + */ + public function testAnIncrementedCounterStaysAPlainInteger(): void + { + Config::updateModSettings([self::COUNTER => '41']); + Config::updateModSettings([self::COUNTER => true], true); + + $this->assertMatchesRegularExpression( + '~^\d+$~', + (string) $this->rawSetting(self::COUNTER), + 'the incremented value is not a plain integer, so it will not survive a round trip', + ); + } + + 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++) { + Config::updateModSettings([self::COUNTER => true], true); + } + + $this->assertSame(13, (int) $this->rawSetting(self::COUNTER)); + } + + /** + * 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. + */ + public function testDoesNotCreateANewSettingHoldingAFalsyValue(): void + { + Config::updateModSettings([self::COUNTER => '0']); + + $this->assertNull($this->rawSetting(self::COUNTER)); + + // An existing one can be set to zero perfectly well. + Config::updateModSettings([self::COUNTER => '7']); + Config::updateModSettings([self::COUNTER => '0']); + + $this->assertSame('0', $this->rawSetting(self::COUNTER)); + } + + public function testUpdatingSettingsLogsNoErrors(): void + { + Config::updateModSettings([self::COUNTER => '1']); + Config::updateModSettings([self::COUNTER => true], true); + Config::updateModSettings([self::COUNTER => false], true); + + $this->assertNoErrorsLogged('updating a counter should be silent'); + } +} diff --git a/tests/Integration/SchemaTest.php b/tests/Integration/SchemaTest.php new file mode 100644 index 0000000000..145975992b --- /dev/null +++ b/tests/Integration/SchemaTest.php @@ -0,0 +1,104 @@ +<?php + +declare(strict_types=1); + +namespace SMF\Tests\Integration; + +use PHPUnit\Framework\Attributes\CoversNothing; +use SMF\Db\DatabaseApi as Db; +use SMF\Db\Schema\Table; + +/** + * Compares the schema SMF declares in PHP against the one it actually built. + * + * The schema for 3.0 lives in Sources/Db/Schema/v3_0/ as 70-odd classes, and the + * installer creates the database from them. Nothing checks afterwards that the + * two still agree, and a query naming a column that is no longer there fails at + * runtime only - inside a background task, it retries forever and takes down + * unrelated page loads. + * + * Both engines get their own DDL out of the same declarations, so this is worth + * running twice. + */ +#[CoversNothing] +class SchemaTest extends IntegrationTestCase +{ + /**************** + * Public methods + ****************/ + + 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'); + } + + public function testEveryDeclaredTableExists(): void + { + $existing = array_map( + static fn($table): string => strtolower($table), + Db::$db->list_tables(), + ); + + $missing = []; + + foreach (Table::getAll('v3_0') as $table) { + if (!\in_array(strtolower(Db::$db->prefix . $table->name), $existing, true)) { + $missing[] = $table->name; + } + } + + $this->assertSame([], $missing, 'tables the schema declares but the database does not have'); + } + + public function testEveryDeclaredColumnExists(): void + { + $missing = []; + + foreach (Table::getAll('v3_0') as $table) { + $columns = array_map( + static fn($column): string => strtolower($column), + Db::$db->list_columns('{db_prefix}' . $table->name), + ); + + // A table that is missing entirely is the other test's business. + if ($columns === []) { + continue; + } + + foreach ($table->columns as $column) { + if (!\in_array(strtolower($column->name), $columns, true)) { + $missing[] = $table->name . '.' . $column->name; + } + } + } + + $this->assertSame([], $missing, 'columns the schema declares but the database does not have'); + } + + /** + * 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. + */ + public function testTheDatabaseHasNoColumnsTheSchemaDoesNotDeclare(): void + { + $unexpected = []; + + foreach (Table::getAll('v3_0') as $table) { + $declared = array_map( + static fn($column): string => strtolower($column->name), + $table->columns, + ); + + foreach (Db::$db->list_columns('{db_prefix}' . $table->name) as $column) { + if (!\in_array(strtolower($column), $declared, true)) { + $unexpected[] = $table->name . '.' . $column; + } + } + } + + $this->assertSame([], $unexpected, 'columns the database has that the schema does not declare'); + } +} diff --git a/tests/Integration/index.php b/tests/Integration/index.php new file mode 100644 index 0000000000..2844a3b9e7 --- /dev/null +++ b/tests/Integration/index.php @@ -0,0 +1,8 @@ +<?php + +// Try to handle it with the upper level index.php. (it should know what to do.) +if (file_exists(dirname(__DIR__) . DIRECTORY_SEPARATOR . 'index.php')) { + include dirname(__DIR__) . DIRECTORY_SEPARATOR . 'index.php'; +} else { + exit; +} diff --git a/tests/Support/HttpClient.php b/tests/Support/HttpClient.php new file mode 100644 index 0000000000..be674df3ca --- /dev/null +++ b/tests/Support/HttpClient.php @@ -0,0 +1,361 @@ +<?php + +declare(strict_types=1); + +namespace SMF\Tests\Support; + +use SMF\Config; + +/** + * A very small browser, for driving the forum over HTTP. + * + * Requests have to be real ones. Utils::obExit(), redirectexit(), + * serverResponse() and ErrorHandler::fatal*() all end in exit, and Db::$db, + * ActionTrait::$obj, Theme::$loaded and User::$loaded have no way to be reset, so + * a test process can carry out exactly one request in itself and no more. Going + * over the wire sidesteps all of that and exercises the same path a visitor does, + * including the session, the cookies and the theme. + * + * Uses curl through the extension the forum already requires, so it costs no new + * dependency. + */ +final class HttpClient +{ + /******************* + * Public properties + *******************/ + + /** + * @var string Where requests go. Public so a test can report it on failure. + */ + public readonly string $base_url; + + /********************* + * Internal properties + *********************/ + + /** + * @var \CurlHandle One handle for the life of the client. + * + * Reused rather than opened per request, and that is load bearing. curl only + * writes cookies that carry an expiry to the jar file; a session cookie has + * none, so closing the handle between requests threw the session away and + * every request arrived as a brand new visitor. The symptom is not an obvious + * one - pages still render, but any POST is rejected because the session + * check and the security token it carries were issued to a session that no + * longer exists. + */ + private \CurlHandle $handle; + + /** + * @var string Path to this client's cookie jar. + */ + private string $jar; + + /** + * @var HttpResponse|null The most recent response. + */ + private ?HttpResponse $last = null; + + /**************** + * Public methods + ****************/ + + /** + * @param string|null $base_url Override the forum URL to talk to. + */ + public function __construct(?string $base_url = null) + { + $this->base_url = rtrim($base_url ?? self::detectBaseUrl(), '/'); + + $this->jar = (string) tempnam(sys_get_temp_dir(), 'smf_tests_cookies_'); + + $handle = curl_init(); + + if (!$handle instanceof \CurlHandle) { + throw new \RuntimeException('could not start curl'); + } + + $this->handle = $handle; + } + + public function __destruct() + { + curl_close($this->handle); + + if ($this->jar !== '' && is_file($this->jar)) { + @unlink($this->jar); + } + } + + /** + * Fetches a page. + * + * @param string $path Either a full URL or something to hang off the board + * URL, with or without a leading slash. '?action=login' is typical. + * @return HttpResponse The response. + */ + public function get(string $path = ''): HttpResponse + { + return $this->request($this->url($path), null); + } + + /** + * Posts to a page. + * + * @param string $path Where to post, as for get(). + * @param array $fields The form fields. + * @return HttpResponse The response. + */ + public function post(string $path, array $fields): HttpResponse + { + return $this->request($this->url($path), $fields); + } + + /** + * Submits a form on a page the client has already fetched. + * + * This is the method to reach for. SMF forms carry a session check that + * User::checkSession() rejects the request without, and often a SecurityToken + * as well, both named unpredictably per session; resubmitting every field the + * page offered is what a browser does and saves the test knowing about any of + * it. + * + * @param HttpResponse $page The page holding the form. + * @param array $overrides Values to change or add. + * @param string $xpath Which form. Defaults to the first on the page. + * @return HttpResponse The response. + */ + public function submit(HttpResponse $page, array $overrides = [], string $xpath = '//form'): HttpResponse + { + return $this->request( + $this->url($page->formAction($xpath)), + array_merge($page->formFields($xpath), $overrides), + ); + } + + /** + * The most recent response, for reporting on a failure. + * + * @return HttpResponse|null The response, or null if nothing has been sent. + */ + public function lastResponse(): ?HttpResponse + { + return $this->last; + } + + /** + * Throws away this client's cookies, making it a fresh visitor. + */ + public function forgetCookies(): void + { + // In memory, not in the file: session cookies never reach the file. + curl_setopt($this->handle, CURLOPT_COOKIELIST, 'ALL'); + + if (is_file($this->jar)) { + file_put_contents($this->jar, ''); + } + } + + /****************** + * Internal methods + ******************/ + + /** + * Turns whatever a caller passed into an absolute URL. + * + * @param string $path A full URL, a query string, or a path. + * @return string An absolute URL. + */ + private function url(string $path): string + { + if ($path === '') { + return $this->base_url . '/'; + } + + // A form action is usually an absolute URL built from Config::$boardurl, + // which is not necessarily the host we are talking to - inside the + // container the forum answers on port 80 while boardurl names 8080. Keep + // the path and query, drop the rest. + if (preg_match('~^https?://~i', $path)) { + $parts = parse_url($path); + + $path = ($parts['path'] ?? '/') + . (isset($parts['query']) ? '?' . $parts['query'] : '') + . (isset($parts['fragment']) ? '#' . $parts['fragment'] : ''); + } + + if (str_starts_with($path, '?')) { + return $this->base_url . '/index.php' . $path; + } + + return $this->base_url . '/' . ltrim($path, '/'); + } + + /** + * Sends one request. + * + * @param string $url The absolute URL. + * @param array|null $fields POST fields, or null for a GET. + * @return HttpResponse The response. + */ + private function request(string $url, ?array $fields): HttpResponse + { + $handle = $this->handle; + + $options = [ + CURLOPT_URL => $url, + CURLOPT_RETURNTRANSFER => true, + CURLOPT_HEADER => true, + // Off on purpose. A redirect is frequently the thing under test - + // posting a reply is a success only if it sends you somewhere - and + // following it silently would hide both the status and the location. + CURLOPT_FOLLOWLOCATION => false, + CURLOPT_COOKIEJAR => $this->jar, + CURLOPT_COOKIEFILE => $this->jar, + CURLOPT_CONNECTTIMEOUT => 5, + CURLOPT_TIMEOUT => 30, + CURLOPT_USERAGENT => 'SMF test suite', + ]; + + if ($fields !== null) { + $options[CURLOPT_POST] = true; + $options[CURLOPT_POSTFIELDS] = http_build_query($fields); + } else { + // The handle is reused, so a GET after a POST has to say so or it + // would repeat the previous body. + $options[CURLOPT_HTTPGET] = true; + } + + curl_setopt_array($handle, $options); + + $raw = curl_exec($handle); + + if ($raw === false) { + throw new \RuntimeException('request to ' . $url . ' failed: ' . curl_error($handle)); + } + + $status = (int) curl_getinfo($handle, CURLINFO_HTTP_CODE); + $header_size = (int) curl_getinfo($handle, CURLINFO_HEADER_SIZE); + + $raw = (string) $raw; + + [$headers, $set_cookies] = self::parseHeaders(substr($raw, 0, $header_size)); + + return $this->last = new HttpResponse( + $status, + substr($raw, $header_size), + $headers, + $url, + $set_cookies, + ); + } + + /************************* + * Internal static methods + *************************/ + + /** + * Works out which URL the forum answers on. + * + * SMF_TESTS_BASE_URL wins. Otherwise it is Config::$boardurl, unless nothing + * is listening there - which is the normal case when the tests run inside the + * web container, where the forum is on port 80 and boardurl names whatever + * port the host publishes. + * + * @return string The base URL. + */ + private static function detectBaseUrl(): string + { + $override = (string) getenv('SMF_TESTS_BASE_URL'); + + if ($override !== '') { + return $override; + } + + $boardurl = (string) (Config::$boardurl ?? ''); + + if ($boardurl !== '' && self::listening($boardurl)) { + return $boardurl; + } + + $parts = parse_url($boardurl) ?: []; + + return ($parts['scheme'] ?? 'http') . '://localhost' . ($parts['path'] ?? ''); + } + + /** + * Whether anything answers on the host and port of a URL. + * + * @param string $url The URL to try. + * @return bool Whether a connection could be opened. + */ + private static function listening(string $url): bool + { + $parts = parse_url($url); + + if (!isset($parts['host'])) { + return false; + } + + $port = $parts['port'] ?? (($parts['scheme'] ?? 'http') === 'https' ? 443 : 80); + + $socket = @fsockopen($parts['host'], (int) $port, $errno, $errstr, 2); + + if ($socket === false) { + return false; + } + + fclose($socket); + + return true; + } + + /** + * Splits a raw header block into name => value. + * + * Only the last set is kept, which matters because CURLOPT_HEADER includes + * every hop when a proxy or a 100-continue is involved. + * + * @param string $raw The raw headers. + * @return array Two items: the headers as lowercased name => value, and + * every Set-Cookie value in order. + */ + private static function parseHeaders(string $raw): array + { + $headers = []; + $cookies = []; + + foreach (preg_split('~\R~', $raw) ?: [] as $line) { + $line = trim($line); + + if ($line === '') { + continue; + } + + if (stripos($line, 'HTTP/') === 0) { + $headers = []; + $cookies = []; + + continue; + } + + $colon = strpos($line, ':'); + + if ($colon === false) { + continue; + } + + $name = strtolower(substr($line, 0, $colon)); + $value = trim(substr($line, $colon + 1)); + + $headers[$name] = $value; + + if ($name === 'set-cookie') { + $cookies[] = $value; + } + } + + return [$headers, $cookies]; + } +} diff --git a/tests/Support/HttpResponse.php b/tests/Support/HttpResponse.php new file mode 100644 index 0000000000..15b844577e --- /dev/null +++ b/tests/Support/HttpResponse.php @@ -0,0 +1,259 @@ +<?php + +declare(strict_types=1); + +namespace SMF\Tests\Support; + +/** + * One response from HttpClient. + * + * Parsing is deliberately DOM based rather than string matching. The theme moves + * around a lot, so a test that greps for a phrase fails the next time somebody + * reflows the markup, which teaches everyone to ignore it. + */ +final class HttpResponse +{ + /********************* + * Internal properties + *********************/ + + /** + * @var \DOMDocument|null The parsed body, once something has asked for it. + */ + private ?\DOMDocument $dom = null; + + /**************** + * Public methods + ****************/ + + /** + * @param int $status The HTTP status. + * @param string $body The response body. + * @param array $headers Headers, lowercased name => value. Where a header + * appeared more than once only the last is here; Set-Cookie is the one + * that routinely does, so it has its own list below. + * @param string $url The URL that was requested. + * @param array $set_cookies Every Set-Cookie header, in the order sent. + * Logging in sends two - the session and the forum's own - and keeping + * only one of them loses whichever the test cares about. + */ + public function __construct( + public readonly int $status, + public readonly string $body, + public readonly array $headers, + public readonly string $url, + public readonly array $set_cookies = [], + ) {} + + /** + * The response body as a DOM document. + * + * SMF emits HTML5, which DOMDocument grumbles about; the warnings are not + * interesting and would fail the test under failOnWarning, so they are + * collected and discarded rather than raised. + * + * Parsed once per response. Note the cache has to be a property: a static + * inside this method is shared by every instance of the class, so the first + * page fetched would be handed back for every page after it. + * + * @return \DOMDocument The parsed body. + */ + public function dom(): \DOMDocument + { + if ($this->dom instanceof \DOMDocument) { + return $this->dom; + } + + $dom = new \DOMDocument(); + + $previous = libxml_use_internal_errors(true); + $dom->loadHTML('<?xml encoding="UTF-8">' . $this->body, LIBXML_NOWARNING | LIBXML_NOERROR); + libxml_clear_errors(); + libxml_use_internal_errors($previous); + + return $this->dom = $dom; + } + + /** + * Runs an XPath query against the body. + * + * @param string $expression The XPath expression. + * @return \DOMNodeList The matching nodes. + */ + public function xpath(string $expression): \DOMNodeList + { + $result = (new \DOMXPath($this->dom()))->query($expression); + + return $result === false ? new \DOMNodeList() : $result; + } + + /** + * The page title, without the forum name SMF appends to it. + * + * @return string The title, or an empty string when there is none. + */ + public function title(): string + { + $titles = $this->xpath('//title'); + + return $titles->length === 0 ? '' : trim((string) $titles->item(0)?->textContent); + } + + /** + * All visible text, with runs of whitespace collapsed. + * + * For assertions where the structure genuinely does not matter, such as + * checking an error message reached the page at all. + * + * @return string The text content of the body. + */ + public function text(): string + { + $body = $this->xpath('//body'); + $text = $body->length === 0 ? $this->body : (string) $body->item(0)?->textContent; + + return trim((string) preg_replace('~\s+~u', ' ', $text)); + } + + /** + * Whatever the page is complaining about. + * + * SMF renders a fatal error as an ordinary page with a box on it, and the + * surrounding menus and news run to several hundred characters, so quoting + * the start of the body in a failure message reliably shows everything + * except the reason. This picks out the reason. + * + * @return string The error text, or an empty string when there is none. + */ + public function errorText(): string + { + $found = []; + + foreach ($this->xpath('//*[contains(@class, "errorbox")] | //*[contains(@class, "error_message")] | //*[@id="fatal_error"]') as $node) { + $text = trim((string) preg_replace('~\s+~u', ' ', $node->textContent)); + + if ($text !== '') { + $found[] = $text; + } + } + + return implode(' / ', array_unique($found)); + } + + /** + * Every field a browser would submit for the given form. + * + * SMF puts more than one hidden field in its forms - the session check that + * User::checkSession() insists on, and often a SecurityToken as well - and + * the names of both are generated per session. Collecting them all is both + * simpler and more honest than knowing which is which. + * + * Buttons are left out, because a browser submits only the one that was + * clicked and SMF branches on which that was. The posting form offers both + * "preview" and "post"; sending the pair means the preview wins and the reply + * is silently never made, with a perfectly good 200 to show for it. Pass the + * button you mean to press as an override. + * + * @param string $xpath Which form. Defaults to the first one on the page. + * @return array The fields, name => value. + */ + public function formFields(string $xpath = '//form'): array + { + $form = $this->xpath($xpath)->item(0); + + if (!$form instanceof \DOMElement) { + return []; + } + + $fields = []; + $finder = new \DOMXPath($this->dom()); + + foreach ($finder->query('.//input | .//textarea | .//select', $form) ?: [] as $input) { + if (!$input instanceof \DOMElement) { + continue; + } + + $name = $input->getAttribute('name'); + + if ($name === '') { + continue; + } + + $type = strtolower($input->getAttribute('type')); + + // A browser only submits these when they are ticked, and submitting + // an unticked one turns every checkbox on the page into a yes. + if (\in_array($type, ['checkbox', 'radio'], true) && !$input->hasAttribute('checked')) { + continue; + } + + // Only the button that was clicked gets submitted. See above. + if (\in_array($type, ['submit', 'button', 'reset', 'image'], true)) { + continue; + } + + $fields[$name] = match ($input->nodeName) { + 'textarea' => $input->textContent, + 'select' => $this->selectedOption($finder, $input), + default => $input->getAttribute('value'), + }; + } + + return $fields; + } + + /** + * Where the given form posts to. + * + * @param string $xpath Which form. Defaults to the first one on the page. + * @return string The action attribute, or the current URL when it has none. + */ + public function formAction(string $xpath = '//form'): string + { + $form = $this->xpath($xpath)->item(0); + + if (!$form instanceof \DOMElement) { + return $this->url; + } + + $action = $form->getAttribute('action'); + + return $action === '' ? $this->url : $action; + } + + /****************** + * Internal methods + ******************/ + + /** + * The value a browser would submit for a select element. + * + * @param \DOMXPath $finder An XPath instance for this document. + * @param \DOMElement $select The select element. + * @return string The selected value, or the first option's. + */ + private function selectedOption(\DOMXPath $finder, \DOMElement $select): string + { + $options = $finder->query('.//option', $select) ?: new \DOMNodeList(); + + $first = ''; + + foreach ($options as $option) { + if (!$option instanceof \DOMElement) { + continue; + } + + $value = $option->hasAttribute('value') ? $option->getAttribute('value') : $option->textContent; + + if ($first === '') { + $first = $value; + } + + if ($option->hasAttribute('selected')) { + return $value; + } + } + + return $first; + } +} diff --git a/tests/Support/index.php b/tests/Support/index.php new file mode 100644 index 0000000000..2844a3b9e7 --- /dev/null +++ b/tests/Support/index.php @@ -0,0 +1,8 @@ +<?php + +// Try to handle it with the upper level index.php. (it should know what to do.) +if (file_exists(dirname(__DIR__) . DIRECTORY_SEPARATOR . 'index.php')) { + include dirname(__DIR__) . DIRECTORY_SEPARATOR . 'index.php'; +} else { + exit; +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 0f861e3542..78ed35d45c 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -52,6 +52,12 @@ $loader->setPsr4('SMF\\', TESTS_BOARDDIR . '/Sources'); $loader->setPsr4('SMF\\Themes\\', TESTS_BOARDDIR . '/Themes'); +// The unit tests are each self-contained, so nothing had to autoload them. +// Anything sharing a base class or a helper does, and registering it here keeps +// it beside the other two rather than adding an autoload-dev section that only +// the test suite would ever use. +$loader->setPsr4('SMF\\Tests\\', TESTS_BOARDDIR . '/tests'); + /* * Paths and the default language, which the Unicode and entity helpers need in * order to locate their data files. These are the only pieces of Config the suite