[3.0][Testing] Add HTTP smoke tests that drive a running forum

.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

.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}"

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/<Class>Test.php`, namespace `SMF\Tests\Unit`, `declare(strict_types=1)`,

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",

phpunit.xml.dist

@@ -11,6 +11,14 @@
 		<testsuite name="unit">
 			<directory>tests/Unit</directory>
 		</testsuite>
+		<!--
+			Needs an installed forum, and skips itself when there is not one, so
+			running the whole file is safe with or without a database. See
+			tests/Integration/Installation.php.
+		-->
+		<testsuite name="integration">
+			<directory>tests/Integration</directory>
+		</testsuite>
 	</testsuites>
 	<source>
 		<include>