Closed (fixed)
Project:
Drupal core
Version:
10.3.x-dev
Component:
phpunit
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
13 Feb 2024 at 14:38 UTC
Updated:
13 Mar 2024 at 04:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #3
kristiaanvandeneyndeSo the MR is a proof of concept.
This allows us to build a list of expected queries and compare it to what was actually tracked. We may need to sort the queries before comparing, but so far on my local they have always been in the same order.
NW because this isn't finished yet.
Comment #4
catchBumping to major - this should allow us to track down the variation in query counts, which if we can go back to assertSame() will make the test coverage much more reliable. Also being able to assert individual queries will be useful in its own right in case we swap one query for another or similar.
Comment #5
kristiaanvandeneyndeSeeing a lot of
SELECT "tag", "invalidations" FROM "cachetags" WHERE "tag" IN. We could perhaps track those separately like we do with cache gets, sets, etc. by checking for the calling class being DatabaseCacheTagsChecksum.Just like we already check for DatabaseBackend to only log queries that didn't come from the DB cache, we could add in DatabaseCacheTagsChecksum to not track those queries either, then decorate
cache_tags.invalidator.checksumand add some tracking in there.Comment #6
kristiaanvandeneyndeOops, cross-posted.
Okay so the final fail I'm seeing is related to a block ID being random (e.g.: "config:block.block.jf547b5k"8). But that only happens in a cache tag invalidation and as I wrote above we can stop tracking those as queries and simply do a count on them without being too specific.
So StandardPerformanceTest looking really good now. Need to apply the same approach to the Umami performance tests. But I'm on vacation as of right now, so will have to wait until next week :)
Comment #7
catchAgreed with this, I thought I had an issue open for it already, but can't find it now. I briefly looked at decorating the cache tags service, but because the checksum implementation isn't part of the actual cache tags API as such, but a shared implementation/trait, the decorator approach didn't seem great. Just separating out the queries seems fine and we can always try a decorator approach later on.
Comment #8
catchOpened #3421881: Track cache tag queries separately in performance tests which should hopefully make things easier here once it's in.
Comment #9
kristiaanvandeneyndeIn the process of setting up a new laptop but once I'm settled in I'll review the other issue and merge it into this one
Comment #10
kristiaanvandeneyndeWill merge in the changes from #3421881: Track cache tag queries separately in performance tests and tag this issue as PP-1 if the other hasn't been committed by then.
Comment #11
kristiaanvandeneyndeMerged in your work, will see what testbot thinks.
Comment #12
kristiaanvandeneyndeTests go green with exact query counts, will try and do the same for Umami tests tomorrow.
Comment #13
kristiaanvandeneyndeAll performance tests go green with exact query counts on my local now. I also removed the query aliasing in favor of parameter aliasing.
However, on one occasion, StandardPerformanceTest::testLoginBlock() had one extra query as the second-to-last query:
Grafana reported this as coming from Drupal\Core\Session\SessionHandler, so this may be related to #3033791: Do explicit session garbage collection on cron
Comment #14
kristiaanvandeneyndeOther issue got committed, so dropping the PP tag and removing draft status from the MR.
Comment #15
smustgrave commentedAppears to need a rebase.
Will this be a replacement for the performance checks where it assets the count between 2 values.
Comment #16
catchYeah that's the plan. #3423329: Prevent session garbage collection during functional tests, which I just opened based on @Kristiaan's report, will cover the one known random test failure (found via this issue), but this issue will help flush any more out.
Comment #17
kristiaanvandeneyndeMade the two methods static
Comment #18
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 #19
kristiaanvandeneyndeComment #20
smustgrave commentedFeedback appears to be addressed and merge conflicts resolved
Excited to see this land.
Comment #23
catchI originally thought something like this would be too much to maintain, but now that we've split out the cache and cache tag operations we're left with only 'real' database queries now, and the argument replacement, while it requires hard-coding some assumptions in PerformanceTestTrait is easy to follow. This will help massively with any future random variation similar to the session gc one and gets us back to precise assertions again.
Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!
Comment #24
wim leers🤩 This is incredibly helpful! Agreed that it wasn't feasible before, but it now is, and that alone is a strong indicator of the progress that's been made 👏
Comment #25
wim leersOh, and it shows already that in real sites, there would be one DB query less:
(that's a test-only DB query, caused by
\Drupal\Core\Test\StackMiddleware\TestWaitTerminateMiddleware.Comment #26
slashrsm commentedComment #27
catch#25 yes that's been annoying me, opened #3423832: Move the test wait terminate service to a test module.
Comment #28
wim leers