[3.0] Add a PHPUnit suite for the parts that need no database - #9326
Open
albertlast wants to merge 17 commits into
Open
[3.0] Add a PHPUnit suite for the parts that need no database#9326albertlast wants to merge 17 commits into
albertlast wants to merge 17 commits into
Conversation
ActionTrait declares $obj as a static property, and a static property is
shared with every descendant class that does not redeclare it. None of
the eleven action classes that extend another action redeclare it, so
they all share one slot with their parent.
Once the parent has been loaded, load() finds that slot occupied and
returns the parent's instance, which does not satisfy the "static"
return type:
SMF\Actions\Login2::load(): Return value must be of type
SMF\Actions\Logout, SMF\Actions\Login2 returned
This is reachable during login: User::enforceBans() calls Logout::call()
to kick a banned member, by which point Login2 has already been loaded,
so a banned member gets a fatal error instead of being logged out.
Checks that the cached instance is of the class being loaded, rather than
merely present.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The members table lost its time_offset column in 3.0, in the
DropTimeOffset migration, and User::$time_offset became a virtual
property derived from the member's time zone. User::setProperties() even
lists time_offset among the "obsolete data" it ignores.
CreatePost_Notify still asked the database for it:
mem.smiley_set, mem.time_format, mem.time_offset, mem.timezone,
so the query fails outright with "column mem.time_offset does not
exist", and the task dies on the fetch_assoc() of a false result. The
task is never marked done, so it is retried on later page loads and
takes down whichever request happens to run it:
SMF\Db\APIs\PostgreSQL::fetch_assoc(): Argument #1 ($result) must be
of type object, false given
Nobody watching a board or topic receives their notification either.
Derives the offset from mem.timezone, which the query already selects,
the same way User::$time_offset does. Also keys the parsed-message cache
by time zone rather than by offset, and drops an (int) cast that
truncated the offsets of half-hour and quarter-hour time zones.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PHP-CS-Fixer sorts class members, and a static method does not belong in the non-static "Internal methods" group. No change to the code itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AGENTS.md is the cross-tool convention, read directly by Copilot, Codex, Cursor, Gemini CLI and others. Claude Code reads only CLAUDE.md, so that file imports AGENTS.md rather than duplicating it. A symlink would also work, but not on Windows without Developer Mode, and .gitattributes already forces LF for Windows checkouts, so the import is the portable choice. Contents are derived from the repository's own configuration rather than from habit: .editorconfig, .gitattributes, .php-cs-fixer.dist.php and its custom SectionComments fixer, composer.json, the workflows in .github, DCO.txt, the PR template and compose.yaml. Two points are worth stating explicitly for tools that cannot infer them: that there is no test suite, so green CI only means the code parses and is formatted; and that the sign-off check can pass on a pull request even when the commits are unsigned, because it walks up to the parents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SMF has no automated tests. CI proves that the code parses and that it is formatted; it never executes anything. Every one of the bugs fixed in SimpleMachines#9319 through SimpleMachines#9322 parsed cleanly and passed every check. Quite a lot of 3.0 is reachable without a forum behind it. The bootstrap here defines the constants index.php would define and points the autoloader at Sources/, and that is enough: no Settings.php, no database, no request. Anything that reaches Config::$modSettings, User::$me or Db::$db is out of scope and belongs in an integration suite. The first tests cover ground that recently broke: - ActionTrait::load() returning an instance of the class it was called on, in both orders and in two separate class hierarchies. - CreatePost_Notify::getTimeOffset(), including the half-hour and quarter-hour zones that an int cast used to truncate. - Utils::buildRegex(), including the trailing quoted character from SimpleMachines#9318. tests/ is already excluded from the license header check in BuildTools, and the directory index.php files keep check-smf-index happy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a PHPUnit workflow across the same 8.4 and 8.5 matrix the syntax check already uses, and a composer test script. Two adjustments fall out of running the suite next to the existing checks. The PHPUnit cache lives in .phpunit.cache rather than under cache/, because check-smf-index walks every directory that is not hidden and would otherwise report a missing index file the moment anyone runs the tests locally. And AGENTS.md no longer says there is no test suite; it now says what the suite does and does not cover, so an agent does not mistake a green run for proof that a change works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first pass covered only the three things that had recently broken. Quite a lot more is reachable without a database once the bootstrap sets the paths and default language that the Unicode and entity helpers use to find their data files, which is six lines and still reads nothing from Settings.php. Adds coverage for Utils' entity-aware string handling and Unicode case conversion, IP, Url, Uuid, Sapi, Security's password hashing, Punycode and TimeInterval. 99 tests, 144 assertions, on 8.4 and 8.5. Two behaviours are deliberately described rather than asserted, because pinning them down would preserve something that looks wrong: - Sapi::memoryReturnBytes() strips the last character before parsing, so a unit-less value such as '128' reads as 12 and the '-1' that means "no limit" reads as 0. Only suffixed values are asserted. - Url::isScheme() compares the scheme without normalising case, so an uppercase scheme fails to match its own name. Only exact-case matching is asserted. IP's constructor accepts the packed binary form, which it cannot tell apart from any other 4 or 16 byte string, so 'nope' becomes 110.111.112.101. That one is genuine ambiguity rather than a defect, so it is pinned down as a test in its own right. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
memoryReturnBytes() removed the last character of the value before
parsing the number, on the assumption that it is always a designator.
PHP's shorthand notation is optional, so a plain byte count loses its
last digit: '128' reads as 12, and '2097152' reads as 209715.
Graphics\Image does exactly that, passing a computed byte count with no
designator, so resizing an image asks for a tenth of the memory it just
worked out that it needs.
The other value with no designator is '-1', which means there is no
limit. It read as 0, because intval('-') is 0, so setMemoryLimit() found
the current limit to be smaller than anything and set one. On a server
with no memory limit, asking for 128M capped it at 128M.
Only strips the last character when it is one of the designators PHP
accepts, and reports "no limit" as PHP_INT_MAX so that the callers
comparing it against an amount they need do not each have to special
case it.
The dead is_integer() check went with it; the parameter is typed string.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RFC 3986, section 3.1, makes scheme names case insensitive, and this class keeps the scheme exactly as it was written rather than normalizing it. isScheme() compared the two with in_array(), so a URL written with an uppercase scheme did not match its own name. That reaches two callers. isWebsite() stops recognising HTTP:// and HTTPS:// as websites, and the avatar handling in User treats a DATA: URI as though it were a remote address. Folds both sides before comparing, and makes the comparison strict while it is there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The memoryReturnBytes() and isScheme() cases were described in comments rather than asserted, because pinning down the behaviour would have preserved it. Now that both are fixed, they become tests. Verified to fail against the unfixed code: reverting the two source files alone fails exactly these six tests and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 29, 2026
CoversClass on a trait is not a valid coverage target, and PHPUnit only says so when coverage is actually collected. The suite passed on its own and failed all five ActionTrait cases the moment anyone ran it with --coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Collaborator
Author
|
When this and pr #9317 got merged, i would look into creating tests framework with database stuff |
The Docker environment gained a MySQL service and made it the default engine, so the service list in AGENTS.md was describing a stack that no longer matches: it named PostgreSQL alone, and pointed at a container called smf-dev-db-1 that no longer exists now that the service is named postgres. Lists both engines, and switches the examples to docker compose exec, which addresses services by name and so does not break again if the project or container naming changes. Also states that SMF_DB_TYPE defaults to mysql, and how to re-test on the other engine, because "any raw SQL must work on both" earlier in this document is otherwise an instruction with no stated way to follow it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com> # Conflicts: # AGENTS.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This change was produced by an LLM. The code, the commit messages and this
description were all written by Claude (Anthropic), driven by @albertlast. It has
not yet had human code review.
The suite was run rather than only reasoned about, on both PHP versions CI targets.
Please review it as untrusted work, and as a proposal rather than a fix: whether SMF
wants a test suite, and whether this is the right shape for one, is a project
decision.
Description
SMF has no automated tests. CI proves that the code parses and that it is formatted;
it never executes anything. Every bug in the recent run — #9319, #9320, #9321, #9322,
#9324, #9325 — parsed cleanly and passed every check.
More of 3.0 is testable than the absence of a suite suggests. The 3.0 rewrite into
namespaced classes did most of the work already.
tests/bootstrap.phpdefines theconstants
index.phpdefines, points the autoloader atSources/, and sets the pathsand default language that the Unicode and entity helpers use to find their data files.
That is the whole bootstrap: no
Settings.php, no database, no request.108 tests, 157 assertions, in well under a second.
UtilsTestbuildRegex, entity-awarestrlen/substr/strpos/split,htmlTrim,truncate,shorten,normalize,convertCase,sanitizeChars,sanitizeEntities, JSONIPTestUrlTesttoAscii/toUtf8, IDN, scheme handlingUuidTestcompress/expand, short form, strict parsingSecurityTestSapiTestcanonicalPath, memory size parsingTimeIntervalTesttoSecondsActionTraitTestload()returning the right class, in both orders, two hierarchiesCreatePostNotifyTestgetTimeOffset(), including half and quarter hour zonesPunycodeTestWhat is not covered is anything reaching
Config::$modSettings,User::$meorDb::$db, which is most of the forum. That boundary is deliberate and is stated intests/bootstrap.php: work needing those belongs in an integration suite runningagainst a real install, which the Docker environment in #9317 would be the natural
place to build. Production code should not be contorted to make it reachable from here.
AGENTS.md, added in #9323, said there was no test suite. It now describes what thesuite does and does not cover, so an agent does not read a green run as proof that a
change works.
These are regression tests, not tests written to fit
Each set was checked against the unfixed code:
Sources/ActionTrait.phpandSources/Tasks/CreatePost_Notify.php:Tests: 16, Assertions: 12, Errors: 11. The fourActionTraitTestcases and allseven
CreatePostNotifyTestcases fail; theUtilscases pass, because [3.0] Ensures trailing chars are correctly quoted in Utils::buildRegex() #9318 isalready merged.
Sources/Sapi.phpandSources/Url.php: exactly six failures, threein
SapiTestand three inUrlTest, and nothing else.Two behaviours were found while writing this and are now fixed in #9324 and #9325.
Until they were fixed the suite described them in comments rather than asserting them,
since pinning a defect down in a test preserves it.
One behaviour is pinned down deliberately, in
testAnyFourByteStringIsReadAsAPackedAddress:IP's constructor accepts the packed binary form and cannot tell it apart from anyother four or sixteen byte string, so
new IP('nope')is110.111.112.101and passesvalidation. That is genuine ambiguity in the input rather than a defect, and it is the
sort of thing worth having written down. It caught the test out first: the initial
invalid-input case was
'not an ip at all', which is exactly sixteen bytes andtherefore a valid IPv6 address.
How this was verified
vendor/bin/phpunit: OK (108 tests, 157 assertions) on PHP 8.4.23 and on 8.5,the two versions the existing syntax check targets.
buildjob still pass:check-smf-license,check-smf-index,check-smf-languages,check-version.Found 0 of 1738 files that can be fixed. Thetests/directory is inside the fixer's finder, so the tests follow the same rulesas the rest of the codebase.
Two practical notes for whoever reviews the plumbing:
tests/is already in the license-header ignore list in BuildTools, so the testfiles need no header.
check-smf-indexdoes still walk the directory, sotests/and
tests/Unit/carry the standardindex.php..phpunit.cacherather than the default undercache/,because
check-smf-indexreports a missing index file for it the moment anyone runsthe tests locally. CI would not have caught that, since the jobs do not share a
workspace.
Separately, the custom
SMF/section_commentsfixer is not attribute-aware: when thefirst member of a group carries a PHP attribute, the banner is inserted between the
attribute and the declaration.
tests/Unit/CreatePostNotifyTest.phporders a plainmethod first to avoid it, with a comment saying why. Not addressed here, but worth
knowing as attributes become more common.
Coverage
Measured with pcov on PHP 8.4.23, against the whole of
Sources/, which is what<source>inphpunit.xml.distdeclares:That headline number is deliberately not flattering. The denominator is the entire
codebase, and this suite only reaches the stateless part of it, so 0.31% is a fair
description of how much of SMF is under automated test today. It is reported here
rather than tuned, because a scope narrowed to "the files that happen to have tests"
would produce a much nicer figure that tells nobody anything.
Per class actually exercised:
PunycodeTimeIntervalUrlUuidActionTraitSapiUtilsIPCreatePost_NotifySecurityWhere the line percentage is low but the method percentage is not, the class is mostly
database-bound:
CreatePost_Notifyat 2% meansgetTimeOffset()is covered and theother 385 lines need a running forum.
The number worth watching is not the total. It is whether a class that could be tested
has been.
Three practical notes:
coverage: none, so CI does not collect it. The suite ismeant to stay fast, and there is no threshold to enforce against yet. Collecting it
is a local, on-demand thing.
pecl install pcovin athrowaway
php:8.4-clicontainer is enough.memory_limitwell above the 128M default. Building the coveragetarget map over 200k lines exhausts that; 2G is comfortable.
Running it also turned up a defect in this branch, now fixed.
#[CoversClass]onActionTraitis not a valid target, because it is a trait, and PHPUnit only says sowhen coverage is actually collected. The suite passed on its own and failed all five
ActionTraitTestcases the moment anyone added--coverage:It is
#[CoversTrait]now, which is also whyActionTraitappears in the table above.Relationship to other PRs
This branch is stacked. It contains, and therefore depends on:
AGENTS.mdit amends, which in turn depends on [3.0] Add a Docker development environment for MySQL and PostgreSQL #9317 for theDocker environment it documents.
ActionTraitandCreatePost_Notifytestsrequire. Without them this branch is red by construction.
SapiandUrltests.The four fix PRs are all independent of this one and can be merged on their own. This
one should go last.
Issues References (Fixes|Related|Closes)