[6.x] Stop tests depending on other tests having run first - #15143
Open
jasonvarga wants to merge 8 commits into
Open
[6.x] Stop tests depending on other tests having run first#15143jasonvarga wants to merge 8 commits into
jasonvarga wants to merge 8 commits into
Conversation
The skeleton at vendor/orchestra/testbench-core/laravel persists for the life of a
process, so anything a test writes there is visible to every test that follows it.
198 of our test files leave files behind, which is how a form file containing only
{} ends up crashing CoreNavTest - it only passes today because of lucky ordering,
and that luck runs out as soon as the suite is split across processes.
Snapshot the skeleton once per process and delete anything new after each test.
Directories the framework owns (bootstrap/cache, storage/framework/views and
friends) are left alone, both because deleting them breaks the app and because
walking them gets expensive. A process that starts against an already dirty
skeleton would bake that dirt into its snapshot, so testbench.yaml declares the
paths our tests are known to write and those get cleared before the first boot -
which also means vendor/bin/testbench package:purge-skeleton cleans up after us.
Making an addon with a fieldtype runs 'npm install' from the testbench app's base path. That app has no package.json, so npm walks up and installs against ours, rewriting package-lock.json in the working tree. The other commands that trigger this already fake the process.
The users it makes have no id, so saving them writes tests/__fixtures__/users/.yaml into the repo. Point the stache stores at the throwaway directory like the other tests that save users do.
It was asserting a custom namespace blueprint could be edited without ever creating one, and only passed because StoreCustomBlueprintTest had left one behind in the testbench skeleton.
Glide only makes the directory when it actually processes an image, which by definition never happens here. The test was relying on an earlier one in the file having made it, so make it up front and keep the assertion that nothing lands in it.
Item, ItemWithOrigin and ArrayAccessType were declared at the bottom of PluckTest.php, so SelectTest only found them if PluckTest.php happened to be loaded into the same process first. Tests\ is autoloaded from tests/, so a file each is all they need.
The guarded and allowed path lists on GlobalRuntimeState are only populated as a
side effect of resolving the parser out of the container, and resetGlobalState()
leaves them alone. These tests build parsers by hand, so they were running against
whatever the last test to resolve one left behind. First Antlers test in a process
got empty allow lists, which silently drops every modifier in user content - so
{{ now format="Y" }} rendered a full datetime and a preparsed | upper did nothing.
The mock was compensating for form files other tests had left in the testbench app. Any form adds children to the Forms nav item, which is what broke the assertions - not the missing titles the TODO guessed at. Nothing leaves forms behind now.
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
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.
Cause
Our tests aren't isolated from each other. The suite runs as a single process, so every test file gets loaded, every static stays set, and every file a test writes into the testbench app is still sitting there for the next test. That hides two kinds of coupling.
Tests that leak. The testbench app at
vendor/orchestra/testbench-core/laravelis created once and lives for the whole process. A sweep of all 984 test files found 198 leave files behind — ~50 intoresources/,config/,addons/andpublic/, 69 intostorage/statamic/, 34 leavingStorage::fakeresidue, 29 leavinglaravel.log. Two also wrote into the repo itself:MakeAddonTestrewrote the rootpackage-lock.json, andDuplicateFormTestwrotetests/__fixtures__/users/.yaml.Tests that depend on those leaks. Six files only pass because something else ran first:
SelectTestuses helper classes declared at the bottom ofPluckTest.php, so it needs that file loaded into the same process.ViewBlueprintListingTestasserts a custom namespace blueprint can be edited without ever creating one —StoreCustomBlueprintTesthad left one in the skeleton.AssetTest's non-glideable upload test assertsstorage/statamic/glide/tmpexists, but glide only creates it when it actually processes an image. It was inheriting the directory from an earlier test in the file.NavTestmockedForm::all()away entirely, with a TODO guessing that other tests were leaving behind forms without titles. It was simpler than that: any leftover form at all adds children to the Forms nav item and breaks the assertions.BardFieldtypeTest,MarkdownFieldtypeTestandPreparserTestneed Antlers' guarded/allowed path lists onGlobalRuntimeState, which are only populated as a side effect of resolving the parser out of the container.resetGlobalState()doesn't clear them, soParserTestCase— which builds its parsers by hand — ran against whatever the last test to resolve one left behind. Starting a process with them empty silently drops every modifier in user content, so{{ now format="Y" }}rendered a full datetime and a preparsed| upperdid nothing.This matters now because it blocks splitting the suite across processes, and it has already broken a sharded CI run. A leftover
resources/forms/*.yamlcontaining{}has no title, soCoreNavcallsNav::item(null), getsnullback, andtests/CP/Navigation/CoreNavTest.phpdies withCall to a member function url() on null. In the single-process suite it's masked by lucky ordering.Approach
Give every test a defined starting state rather than patching 198 tests to tidy up after themselves — the same idea as
RefreshDatabase, applied to the other shared mutable state a test can reach.Tests\RestoresTestbenchSkeleton, used byTests\TestCase:bootstrap/cache,storage/framework/{cache,views},vendorandnode_modulesfrom the walk. The framework owns those — deleting them breaks every subsequent test with "Please provide a valid cache path" — and walking compiled views gets expensive.A process starting against an already dirty skeleton would bake that dirt into its snapshot, so a new
testbench.yamldeclares the paths our tests are known to write and those are cleared before the first app boot. That's testbench's ownpurgeconfig, sovendor/bin/testbench package:purge-skeletoncleans up after us too.The individual tests are then made self-sufficient:
MakeAddonTest— making an addon with a fieldtype runsnpm installfrom the testbench app's base path. That app has nopackage.json, so npm walks up and installs against ours. Now fakes the process, as the other commands that trigger this already do.DuplicateFormTest— the users it makes have no id, so saving them writes.yaml. Now points the stache stores at the throwaway directory.ViewBlueprintListingTest— creates the blueprint it's asserting against.AssetTest— creates glide's temp directory up front and keeps the assertion that nothing lands in it.PluckTest—Item,ItemWithOriginandArrayAccessTypemove into their own files.Tests\is autoloaded fromtests/, so that's all they needed.NavTest— theForm::all()mock and its TODO are gone.ParserTestCase— resolves the real parser insetUp, so the Antlers runtime configuration comes from config every time instead of from whichever sibling happened to run first.Worth calling out that
ViewBlueprintListingTestandAssetTeststarted failing when the skeleton restore landed. They were green only on leaked state.Results
The suite got faster rather than slower. The 4 missing assertions are the redundant
assertDirectoryExistsremoved fromAssetTest's 4 data sets. (AssetTestisn't in the isolation count — it passed as a whole file and only broke once tests stopped sharing state within a file.)The one remaining leaver is
tests/Tags/GlideTest.php, which configures a filesystem disk rooted atpublic/glide. The directory is created during the first test's app boot, before the snapshot, so an empty directory survives — and gets cleared at the start of the next process.Also verified directly: planting a
resources/forms/test.yamlcontaining{}in the skeleton no longer breaksCoreNavTest.Two product bugs this turned up —
Nav::create()returningnullfor a null display name, and a title-less form crashing the CP nav — are deliberately out of scope and will get their own PR.