From df38c8b439386ccb37bd109a8f8a0ab746691d47 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 2 Aug 2026 08:18:55 +0200 Subject: [PATCH 1/6] Adds an integration suite that runs against a real forum The unit suite is deliberately database-free, and says so: its bootstrap notes that anything reaching Config::$modSettings, User::$me or Db::$db "belongs in an integration suite running against a real install". There was not one, so most of the forum had no automated proof of anything. Adds tests/Integration/ as a second PHPUnit testsuite, and .docker/test.sh to run it on one engine or both. composer test still runs everything; when there is no forum to talk to the integration tests skip rather than fail, so it stays useful without Docker. IntegrationTestCase gives each test a transaction that is rolled back afterwards, actingAs()/adminId() via User::setMe(), hook() registration that lives only in $modSettings, and assertNoErrorsLogged() - which is usually the point of the test, because SMF records most of what goes wrong in log_errors rather than showing it. Three tests to start: HarnessTest checks the harness itself, including that the rollback really happens and that assertNoErrorsLogged can fail ModSettingsTest the counter regression: updateModSettings($x, true) emitted SET value = value + 1 against a text column SchemaTest compares Sources/Db/Schema/v3_0/ against the database in both directions, which is the drift AGENTS.md warns only ever shows up at runtime ModSettingsTest is why running both engines matters rather than being tidy: with the fix reverted it still passes on MySQL, which coerces text to a number, and fails only on PostgreSQL, which refuses. Verified in both directions before committing. Co-Authored-By: Claude Opus 5 Signed-off-by: albertlast --- .docker/README.md | 22 ++ .docker/test.sh | 81 +++++++ AGENTS.md | 39 ++++ composer.json | 2 + phpunit.xml.dist | 8 + tests/Integration/HarnessTest.php | 132 +++++++++++ tests/Integration/Installation.php | 111 +++++++++ tests/Integration/IntegrationTestCase.php | 270 ++++++++++++++++++++++ tests/Integration/ModSettingsTest.php | 116 ++++++++++ tests/Integration/SchemaTest.php | 100 ++++++++ tests/Integration/index.php | 8 + tests/bootstrap.php | 6 + 12 files changed, 895 insertions(+) create mode 100755 .docker/test.sh create mode 100644 tests/Integration/HarnessTest.php create mode 100644 tests/Integration/Installation.php create mode 100644 tests/Integration/IntegrationTestCase.php create mode 100644 tests/Integration/ModSettingsTest.php create mode 100644 tests/Integration/SchemaTest.php create mode 100644 tests/Integration/index.php diff --git a/.docker/README.md b/.docker/README.md index ff2ea197338..d4ace260467 100644 --- a/.docker/README.md +++ b/.docker/README.md @@ -165,6 +165,27 @@ 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. + ## Running CI locally ```sh @@ -402,6 +423,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 00000000000..7ea104629e9 --- /dev/null +++ b/.docker/test.sh @@ -0,0 +1,81 @@ +#!/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" + + if docker compose exec -T 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 cf8c29002a9..06d399ffc8b 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,44 @@ 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. + +**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 edb9014a7af..8c9352d4aa0 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 951db14dd10..720240fd3c8 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 00000000000..894aaae79c5 --- /dev/null +++ b/tests/Integration/HarnessTest.php @@ -0,0 +1,132 @@ +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/Installation.php b/tests/Integration/Installation.php new file mode 100644 index 00000000000..4505b2675f8 --- /dev/null +++ b/tests/Integration/Installation.php @@ -0,0 +1,111 @@ +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 00000000000..7ba116e57df --- /dev/null +++ b/tests/Integration/IntegrationTestCase.php @@ -0,0 +1,270 @@ +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 + { + 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 00000000000..b609eaff399 --- /dev/null +++ b/tests/Integration/ModSettingsTest.php @@ -0,0 +1,116 @@ + '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 00000000000..ba8855bb4c2 --- /dev/null +++ b/tests/Integration/SchemaTest.php @@ -0,0 +1,100 @@ +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 00000000000..2844a3b9e7b --- /dev/null +++ b/tests/Integration/index.php @@ -0,0 +1,8 @@ +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 From 8383749f69c908efed1f22dbb69f1e1c3f134871 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 10:52:58 +0200 Subject: [PATCH 2/6] Lets the section comment fixer place its own banners The banners in the new test classes were written by hand with the wrong number of asterisks, so SMF/section_comments did not recognise them and inserted its own alongside, leaving IntegrationTestCase with two "Internal properties" headings and two "Internal methods" ones. AGENTS.md says not to hand-write these. Removes them and takes what the fixer produces, along with the single_quote, native_function_invocation and no_unused_imports changes it wanted in the same pass. Co-Authored-By: Claude Opus 5 Signed-off-by: albertlast --- tests/Integration/HarnessTest.php | 8 ++++++++ tests/Integration/IntegrationTestCase.php | 18 +++++++++--------- tests/Integration/ModSettingsTest.php | 9 ++++++++- tests/Integration/SchemaTest.php | 4 ++++ 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/tests/Integration/HarnessTest.php b/tests/Integration/HarnessTest.php index 894aaae79c5..ac7ceb8ffcd 100644 --- a/tests/Integration/HarnessTest.php +++ b/tests/Integration/HarnessTest.php @@ -20,12 +20,20 @@ #[CoversNothing] class HarnessTest extends IntegrationTestCase { + /***************** + * Class constants + *****************/ + /** * The variable the rollback tests write. Named so that finding it left * behind in a real forum points straight back here. */ private const LEFTOVER = 'smf_tests_rollback_canary'; + /**************** + * Public methods + ****************/ + public function testTheForumIsInstalled(): void { $this->assertNotEmpty(Config::$modSettings['smfVersion']); diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index 7ba116e57df..3475c80dd33 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -28,9 +28,9 @@ */ abstract class IntegrationTestCase extends TestCase { - /******************* + /********************* * Internal properties - *******************/ + *********************/ /** * @var array Copy of Config::$modSettings taken before the test ran. @@ -42,9 +42,9 @@ abstract class IntegrationTestCase extends TestCase */ private int $error_watermark = 0; - /**************** - * Public methods - ****************/ + /*********************** + * Public static methods + ***********************/ public static function setUpBeforeClass(): void { @@ -58,9 +58,9 @@ public static function setUpBeforeClass(): void } } - /******************* + /****************** * Internal methods - *******************/ + ******************/ protected function setUp(): void { @@ -175,7 +175,7 @@ protected function assertNoErrorsLogged(string $message = ''): void $errors = []; while ($row = Db::$db->fetch_assoc($request)) { - $errors[] = sprintf( + $errors[] = \sprintf( ' [%s] %s (%s:%d)', $row['error_type'], html_entity_decode((string) $row['message'], ENT_QUOTES | ENT_HTML5, 'UTF-8'), @@ -189,7 +189,7 @@ protected function assertNoErrorsLogged(string $message = ''): void $this->assertSame( [], $errors, - rtrim($message . "\n") . "the forum logged " . \count($errors) . " error(s):\n" . implode("\n", $errors), + rtrim($message . "\n") . 'the forum logged ' . \count($errors) . " error(s):\n" . implode("\n", $errors), ); } diff --git a/tests/Integration/ModSettingsTest.php b/tests/Integration/ModSettingsTest.php index b609eaff399..93a8312a962 100644 --- a/tests/Integration/ModSettingsTest.php +++ b/tests/Integration/ModSettingsTest.php @@ -6,7 +6,6 @@ use PHPUnit\Framework\Attributes\CoversMethod; use SMF\Config; -use SMF\Db\DatabaseApi as Db; /** * Config::updateModSettings() against a real database, on whichever engine is @@ -26,8 +25,16 @@ #[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']); diff --git a/tests/Integration/SchemaTest.php b/tests/Integration/SchemaTest.php index ba8855bb4c2..145975992b9 100644 --- a/tests/Integration/SchemaTest.php +++ b/tests/Integration/SchemaTest.php @@ -23,6 +23,10 @@ #[CoversNothing] class SchemaTest extends IntegrationTestCase { + /**************** + * Public methods + ****************/ + public function testTheSchemaDeclaresTables(): void { // Guards the two tests below: if getAll() ever returns nothing they From 68f810cff5250220397bc918cca7d20d02788cd8 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 2 Aug 2026 16:21:24 +0200 Subject: [PATCH 3/6] Adds HTTP smoke tests that drive a running forum The integration suite reaches the database, but not a page. Everything between a request arriving and HTML coming back - the session, the cookies, the theme, the templates, the permission checks - had no automated coverage at all, and that is where the failures people actually report live. Requests have to be real ones: obExit(), redirectexit() and fatal*() all end in exit, and Db::$db, ActionTrait::$obj and Theme::$loaded cannot be reset, so a test process can carry out one request in itself and no more. tests/Support/HttpClient.php is a small browser built on the curl extension the forum already requires, so this costs no new dependency. Three files to start: a sweep of the pages a guest can reach, the login journey, and starting a topic and replying to it. Every one of them ends in assertNoErrorsLogged(), which is the point - SMF records most of what goes wrong in log_errors rather than showing it, so a page can return a flawless 200 while logging an undefined index on every hit. Four things about SMF made these harder to write than expected, and each is commented where it bites rather than worked around silently: - 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. It looks like a broken token, not a replaced session. - Only the button that was clicked gets submitted. The posting form offers "preview" and "post"; sending both means preview wins and the post is never made, with an ordinary 200 to show for it. - Security::spamProtection() allows one login or post every two seconds per IP, and tests are much faster than people, so submitForm() waits it out once instead of failing at random. - curl only writes cookies with an expiry to its jar file, so a handle opened per request loses the session every time. HTTP tests cannot be wrapped in a transaction - the request runs in the web server's process on its own connection, and on MySQL's REPEATABLE READ an open transaction here would never see what it wrote, quietly making assertNoErrorsLogged() incapable of failing. IntegrationTestCase gains usesTransaction() so they can opt out, and PostingTest removes what it creates through Topic::remove(). Co-Authored-By: Claude Opus 5 Signed-off-by: albertlast --- .docker/README.md | 12 + .docker/test.sh | 8 +- AGENTS.md | 23 ++ tests/Integration/Http/GuestPagesTest.php | 168 ++++++++++ tests/Integration/Http/HttpTestCase.php | 274 ++++++++++++++++ tests/Integration/Http/LoginTest.php | 99 ++++++ tests/Integration/Http/PostingTest.php | 222 +++++++++++++ tests/Integration/Http/index.php | 8 + tests/Integration/IntegrationTestCase.php | 24 +- tests/Support/HttpClient.php | 361 ++++++++++++++++++++++ tests/Support/HttpResponse.php | 259 ++++++++++++++++ tests/Support/index.php | 8 + 12 files changed, 1463 insertions(+), 3 deletions(-) create mode 100644 tests/Integration/Http/GuestPagesTest.php create mode 100644 tests/Integration/Http/HttpTestCase.php create mode 100644 tests/Integration/Http/LoginTest.php create mode 100644 tests/Integration/Http/PostingTest.php create mode 100644 tests/Integration/Http/index.php create mode 100644 tests/Support/HttpClient.php create mode 100644 tests/Support/HttpResponse.php create mode 100644 tests/Support/index.php diff --git a/.docker/README.md b/.docker/README.md index d4ace260467..3cb981e43f6 100644 --- a/.docker/README.md +++ b/.docker/README.md @@ -186,6 +186,18 @@ 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. + ## Running CI locally ```sh diff --git a/.docker/test.sh b/.docker/test.sh index 7ea104629e9..8d6d818e206 100755 --- a/.docker/test.sh +++ b/.docker/test.sh @@ -66,7 +66,13 @@ for smf_type in $ENGINES; do log "${smf_type}: running the tests" - if docker compose exec -T web vendor/bin/phpunit --no-coverage --colors=always "${PHPUNIT_ARGS[@]+"${PHPUNIT_ARGS[@]}"}"; then + # 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" diff --git a/AGENTS.md b/AGENTS.md index 06d399ffc8b..08eb3ed4a1e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -197,6 +197,29 @@ Two things the rollback does not cover: **DDL**, since MySQL commits implicitly `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. diff --git a/tests/Integration/Http/GuestPagesTest.php b/tests/Integration/Http/GuestPagesTest.php new file mode 100644 index 00000000000..7c321f9fba3 --- /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 00000000000..a85e423c6c4 --- /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 00000000000..48b7ba77555 --- /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 00000000000..8240481b32c --- /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 00000000000..2844a3b9e7b --- /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/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index 3475c80dd33..b540e9158ee 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -62,11 +62,29 @@ public static function setUpBeforeClass(): void * 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(); - Db::$db->transaction('begin'); + 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 @@ -78,7 +96,9 @@ protected function setUp(): void protected function tearDown(): void { - Db::$db->transaction('rollback'); + if ($this->usesTransaction()) { + Db::$db->transaction('rollback'); + } Config::$modSettings = $this->mod_settings_backup; diff --git a/tests/Support/HttpClient.php b/tests/Support/HttpClient.php new file mode 100644 index 00000000000..be674df3ca3 --- /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 00000000000..15b844577e6 --- /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 00000000000..2844a3b9e7b --- /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; +} From b646b540ebd24491765a8b49851eecebef312653 Mon Sep 17 00:00:00 2001 From: albertlast <mathiaspapealbert@hotmail.com> Date: Sun, 2 Aug 2026 20:51:39 +0200 Subject: [PATCH 4/6] Documents how to write an HTTP test The suite ships the machinery but not the instructions for using it, and three things are not discoverable from reading it: which of the three suites a new test belongs in, who the request is actually made as, and where the endpoint and field names come from. The second is the one that misleads. HttpTestCase inherits actingAs() from IntegrationTestCase, where it repoints User::$me in the PHPUnit process - but the request is handled by Apache in another process, which knows only the cookie. Calling it in an HTTP test changes nothing and leaves the assertions describing a guest, confidently. The identity of a request here is the cookie jar and nothing else. Field names are the opposite problem: they look like something to look up, and are not. submit() scrapes the form the way a browser does, which is what carries the session check and the security token - both named differently for every session, so a hand built POST body gets a 403 it cannot fix. The worked example prints them to make that concrete. Also notes that install.php left in the board root puts an errorbox on every page an administrator sees, which fails assertLooksLikeAForumPage() and crowds out whatever the test was looking at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com> --- .docker/README.md | 155 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) diff --git a/.docker/README.md b/.docker/README.md index 3cb981e43f6..9462cb6274a 100644 --- a/.docker/README.md +++ b/.docker/README.md @@ -198,6 +198,161 @@ 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. +## 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 From 6c458f6fd14bbe1296fd71f306453d1b80e98981 Mon Sep 17 00:00:00 2001 From: albertlast <mathiaspapealbert@hotmail.com> Date: Sun, 2 Aug 2026 21:14:08 +0200 Subject: [PATCH 5/6] Points the credentials note at user.sh The tests skip rather than fail when they cannot sign in, which says what is wrong but not what to do about it. user.sh answers both halves: check says whether the password the suite is using is the right one, and reset puts a forum installed some other way back on the credentials it expects. Signed-off-by: albertlast <mathiaspapealbert@hotmail.com> --- .docker/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.docker/README.md b/.docker/README.md index 9462cb6274a..a7aa96eab70 100644 --- a/.docker/README.md +++ b/.docker/README.md @@ -196,7 +196,9 @@ 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. +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 From fc5a9a4ccc98f391f22834b13f27afd2fa904f2d Mon Sep 17 00:00:00 2001 From: albertlast <mathiaspapealbert@hotmail.com> Date: Mon, 24 Aug 2026 18:57:24 +0200 Subject: [PATCH 6/6] Fixes the code style in the guest pages test The SMF/section_comments fixer puts the Public methods banner at the top of the group, and the DataProvider attribute belongs to the method under it, not above the banner. That one file was all the style check was failing on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com> --- tests/Integration/Http/GuestPagesTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Integration/Http/GuestPagesTest.php b/tests/Integration/Http/GuestPagesTest.php index 7c321f9fba3..cf55b2226f8 100644 --- a/tests/Integration/Http/GuestPagesTest.php +++ b/tests/Integration/Http/GuestPagesTest.php @@ -24,11 +24,11 @@ #[CoversNothing] class GuestPagesTest extends HttpTestCase { - #[DataProvider('guestPages')] /**************** * Public methods ****************/ + #[DataProvider('guestPages')] public function testThePageLoadsAndLogsNothing(string $path, string $name): void { $response = $this->fetch($path);