The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
Updating performance tests is tedious, they are slow, and often, you have to update a dozen places, sometimes also the query list and so on. The result is hard to guess/predict, so you have to run the test, wait 30s, then update numbers and repeat until it's green, also resolve merge conflicts and so on.
They are not really tests with fixed expectations, it's a just a a measurement of the current state and we want to know if that changes.
Steps to reproduce
Proposed resolution
This is not fully thought through, but the basic idea is this:
* We store the expectations in .sql and formatted .json files, based on a name given to two new assert methods, something like assertQueriesByName, assertMetricsByName()
* That method loads the file content, and passes it to the existing assert methods (or for queries maybe something new)
* If you run the test with a certain flag (environment variable?), instead of asserting, it just writes back the current values into the files. Rerun once, have fully updated expecatations.
* for merge conflicts, you can throw away all changes and just write it back. Maybe we can even automate that. We do need to read the data to know what to update for json metrics, but we could possible just strip out conflict markers, pick one version and then update?
* Possibly we can even figure out a way to do a mixed mode of asserting and updating, run that on CI and provide an artifact of the changed files?
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3618432
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
berdirComment #3
berdirComment #4
godotislateThe PHPStan job generates a baseline artifact that can be accessed from Gitlab CI UI. If the jobs fails, the artifact can be downloaded and its contents copied into core/.phpstan-baseline.php. It'd be great if we could do something similar for the performance tests.
Comment #5
catchIf we control the write behaviour via an environment variable, we could have a dedicated job that runs only performance tests with that set to generate the artifact.
Will be hard to do both assert and write I think because phpunit halts tests at the first failed assertion.
Comment #6
berdirYes, phpstan was the inspiration for that comment. but I think that should be a separate follow-up issue, I think it's quite a bit more complicated. performance tests run currently as part of the regular tests, we'd need to split as @catch said, and then possibly detect a fail, rerun with the environment variable the ones that failed, and then identify which files have changed and export those?
Comment #7
godotislateI know #1411074: Add a flag to set up test environment only once per test class for kernel tests is about kernel tests, but is it possible to look into something similar (keeping the database state between tests methods in a class) for browser tests? Theoretically the performance tests methods would be split back out again, or well, the
doprotected methods would need to be converted to publictestmethods. That does raise a question though that if one test method fails and the database state is shared, will that affect the next test method due to run?Comment #8
catchWe might be able to do something like #1411074: Add a flag to set up test environment only once per test class for kernel tests for functional tests if we make it fully opt-in.
#7 could theoretically work if we only have one assertion per method. If the only reason for the test fail is number of queries etc. then it ought to work even with 100% shared database state because the actual state of the database would be the same as if the assertion would have passed.
But agreed that this all sounds like good follow-up material, in this issue we could add an environment variable and then either assert or write.
Comment #10
berdirStarted implementing this and converted one test method.
Pretty happy with it already. If you want to rerun, you can either set the env variable (didn't really test that yet) or just empty the directory and then it recreates everything. The only downside with that approach is that it will use a default set of metrics. but I imagine that we'll normalize to the same set of metrics anyway. So far it was a tradeoff between more information/asserts vs. making it more tedious to update. That part isn't really a concern anymore, so we could just as well just always to a more verbose version? There have been quite a few times where it would have been nice to see more info/the queries, because to actually figure out which queries changed, you need to dump them, then diff.
Didn't implement fancier ideas like dealing with merge conflicts yet, I feel like that might be too much magic, it's quite doable to jut resolve those manually or wipe the folder/specific files.
Looking for some feedback before converting the rest of the tests, this will obviously conflict heavily with ongoing performance-related issues, but hopefully it'll be the last set of tedious updates once this once. Also open to not do everything here to make it easier to review. Manually verifying the numbers would of course be tedious, but it's generated and if it passes the tests, the review probably doesn't need to be too careful.
I also committed a wrong expectation that I used locally to test that it fails and that failed on CI as well.
Comment #11
acbramley commentedThis is awesome! Have you thought about using something like https://github.com/spatie/phpunit-snapshot-assertions for the snapshot assertions? It supports passing an environment variable to it to update snapshots
Comment #12
berdirI didn't know about that. The amount of code that's required for this is is pretty limited and we have some unique bits, like using assertMetrics with the range comparison. I think unless we have more instances where that could be useful, I'd stick with our custom solution?
I also prefer how I used named "snapshots/expectations" over the numbered ones that seems to be used there. This seems more stable as we will add/remove cases over time and want meaningful diffs and we tend to do very large tests with many assertions for performance reasons.
Comment #13
berdirI converted all performance tests now. I also added a CR and added the ability to specify the list of default metrics, that's for example convenient for the umami tests that just want to check JS/CSS. With that, all files are fully generated and no manual changes are necessary, which also simplifies updating them.
note to reviewers: The diff stat is +2000/-1000, manually reviewing all of that is obviously tedious. But I think that's not really necessary as it's all generated. What should be reviewed is that the name argument matches up everywhere and isn't repeating, that was the manual part, copy-pasting that from the call above and all the actual code in the trait. You could also pick 1-2 examples of the smaller files and compare those to before.
The reason for the 1000 extra lines is that now all metrics (except those with just assets) include the more verbose CacheGetCountByBin and CacheTagGroupedLookups. I think now that updating is so much easier and they're separated out, it doesn't hurt to have them more verbose, I even considered to add CacheGetCountByBin by default, I think we can also add more queries for the umami tests so its' easier to see why it changes in a follow-up.
I considered adding a comment on top of the file to point to docs on how to update them so people don't try that manually, but that doesn't work well with JSON, would need to use yaml instead?
Comment #14
nicxvan commentedOk I tested this manually as much as I could.
First I pulled this down.
I ran
core/profiles/standard/tests/src/FunctionalJavascriptFirst I ran it by itself and it passed as expected.
I then updated the performance test to create a second node and visit node/2 the assertions failed as expected.
Then I deleted the content of the assertion directory and ran it, it repopulated as expected and passed.
I ran it again to ensure it passed with the new assertions.
I also checked that the assertions were different.
Then I set it back to node/1 and ran and it failed again as expected.
Then I tried regenerating with the environment variable:
PERF_TEST_UPDATEthat didn't work, but I think it's because I am using ddev..I tested by adding it directly to ddev's config.yaml and it worked as expected.
I scanned through the individual assertions and the shape looks right, I didn't review them in detail.
The cspell addition makes sense.
I reviewed the changes in performanceTestTrait carefully.
I think this is pretty much ready.
I was going to ask about if the directory was missing, but you handled that, the dynamic methods are only on the value object so I think it's fine.
This also makes generating new performance tests easy enough that getting some more complex performance coverage might be viable.
The CR looks good.
It doesn't mention
getExpectationsFilenameand to be honest I'm not sure why that is public, I think it can probably be protected as well.Comment #15
berdirChanged getExpectationsFilename to protected.
On the env variable, doing it in phpunit.xml as mentioned in the CR worked well for me in ddev, prepending it to the command will not, yes. phpunit.xml also works without a ddev restart.
And yes, I think it's reasonably safe to just keep the flag on by default. The one thing is that it won't catch things that are dynamic things in the asserts, like the generated role name I had to fix, didn't notice that locally. So when doing new tests, it's probably good to give it a go without it once generated, but when updating/rebasing existing tests, it should be fine.
I also tested yaml and decided to switch to that. It's a bit less verbose, mostly the closing "]"'s, saves about 300 lines in the metrics files. I hope it makes the diffs a bit easier to read too. Still unsure if I should add a a comment on top of those files, but it would be doable now.
Comment #16
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #17
berdirRebased. Currently rebases on HEAD are a bit tedious as will the initial rerolls once this gets in of other issues, if there are only changes to metrics, then all changes can just be ignored and then rerun the affected tests to update the assertions.
Tests failed in a weird way, but I think that's just an infra thing, mysql container went away.
Comment #18
nicxvan commentedI think this is ready, the yml change is actually really nice, it's far easier to read than json which I found surprising.
This is a huge improvement.
The failure is FunctionalJS which I can't rerun, but it looks like one of the randoms:
(Drupal\Tests\ckeditor5\FunctionalJavascript\EmphasisComment #19
godotislateCommenting here that this should probably be a priority (partially as reminder to myself). We'll have a bunch of conflicts in other MRs at first, but better to rip the band aid.
Reran the failing test and it's green now.
Comment #21
catchNothing to complain about, I think we can add a comment to the top of the YAML files if people miss the new process in a follow-up.
Committed/pushed to main, thanks!
This is probably worth backporting to 11.x too, will be a bit of a pain to reroll, but will make all performance test backports easier and we're likely to have a handful at least.
Comment #22
catchJust rebased #3587797: Move views data cache to cache_discovery and wow that's easy.
Comment #24
berdirAt first I thought I'd wait with the rebase until #3590882: Standard profile should not include Olivero anymore, should provide guidance on how to get started lands as this was a pretty heavy conflict with that on a rebase above. But actually, that will conflict anyway, so might as well benefit from this? the promoted node frontpage issue also didn't land on 11.x yet and needs to be updated as well, so NodePerformanceTest doesn't exist yet.
Rebase wasn't that tedious, mostly had to make sure to manually remove the files that shouldn't be there anymore and regenerate. When reviewing, the main thing is probably to check that I didn't delete any extra assertions that were there and that there aren't any assertion files that have no corresponding method. That's possibly one thing to look out for in the future with the update flag, if we remove/move assertions around, we need to delete specific files or the folder and let it regenerate.
Comment #25
nicxvan commentedFailure is the familiar: Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTags.
I didn't pull this one down, but the backport looks good.
I did think about that yesterday, but the truth is eventually we can sync these up just by deleting them, so there is no harm to having an assert file around that isn't used for a bit.
Confirmed the assertions are all there.
As I mentioned above I don't think this is something to really worry about, the next regeneration will populate the correct files.
Comment #27
catchCommitted/pushed to 11.x, thanks!