Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
OpenTelemetryNodePagePerformanceTest::testNodePageHotCache() should be testing a page cache hit, but according to traces http://grafana.prod.cluster.tag1.io/d/teMVIdjVz/umami?orgId=1&refresh=30..., it's testing a page cache miss (the last query in the traces is setting the page cache).
I tried to add database query assertions to prove this, but ran into #3412556: Allow needs_destruction services to run on page cache hits which results in zero database queries being recorded on page cache hits.
Additionally, after fixing that bug, I'm able to add assertions, but they pass locally and fail on gitlab, so something else is going on on gitlab that's resulting in page cache misses.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3412641
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 #3
catchPushed a branch with the assertions that should make sense, but fail on gitlab ci - stacked on #3412556: Allow needs_destruction services to run on page cache hits which is necessary for the test to record anything at all.
This passes locally for me, so something going on when it runs on gitlab, that also doesn't affect the front page performance test.
Comment #4
mondrakeComment #5
catchHeisenbug - when you check if the cache entry is there, it gets picked up by the test too.
If we extract just that bit and it still works, then this is probably committable in its own right without #3412556: Allow needs_destruction services to run on page cache hits and would fix the open telemetry dashboard.
Comment #6
catchNow that #3412556: Allow needs_destruction services to run on page cache hits is in, trying the basic version of this again.
Comment #7
catchOK that fixed it, looks like the actual issue here was requesting /node/1 instead of node/1 inconsistently. Checked the other performance tests and it makes no difference to those, just the page cache test here - but went ahead and updated anything with forward slashes so everything's consistent.
Would be good to add assertions to all the other test methods too, but we've got #3408713: Add database and cache assertions to OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest open for that already.
Comment #8
smustgrave commentedI find that odd that / caused issues. Wonder if it's worth opening a coding standard issue about if all drupalGet() should have a forward slash or not.
But additional assertions seem fine.
Comment #9
catchYes it is weird, and when trying to debug why it turns into a heisenbug (i.e. generating the URL inside the test would magically make the test start to pass), so a follow-up to standardise sounds good. We can't completely prevent/require forward slashes since it's possible to request arbitrary stuff on the server, so probably can't be an actual coding standard, but standardising one way or the other for Drupal paths would be good.
Comment #10
smustgrave commentedOpened #3440284: Decide on standard for drupalGet forward slash vs no slash
Comment #11
alexpottThis doesn't make any sense calling drupalGet with a leading slash or not results in the same get request... \Drupal\Tests\UiHelperTrait::buildUrl() going to return the same thing....
OHHH... this is what happens for me locally but on gitlab we're installed in a subdirectory... to maybe it's not the same thing. Imo this points us fixing this in \Drupal\Tests\UiHelperTrait::buildUrl() ... I think we should add
after we strip the base path instead of the changes here.
Comment #12
quietone commentedI've only ready the last few comments. Is the issue with the slash something that could be documented at \Drupal\Tests\UiHelperTrait::drupalGet? Just a thought.
Comment #13
catchNice find, I think if we do @alexpott's idea, we don't need a comment since we're properly normalizing the URL regardless of how it comes in then. Updated the MR for that.
Comment #14
catchNope, that passed locally, but it still fails on gitlab, I'm going to revert that commit and open a follow-up and moving this back to RTBC. This change fixes the test to test what it's supposed to be doing, if it wasn't adding additional assertions to prove the fix and making the related tests consistent, it would be a one character change. The drupalGet() behaviour has probably been like this for years and deserves its own issue. Follow-up here #3440424: Inconsistent behaviour with drupalGet() when site is installed in a subdirectory.
Comment #15
alexpottSo the difference between node/1 and /node/1 is down to the fact that gitlabci runs tests in a subdir. When the the path is node/1 this is resolved to subdir/en/recipes/deep-mediterranean-quiche when it is /node/1 this is resolved to subdir/en/node/1. Note that without the basedir both node/1 and /node/1 are resolved to subdir/en/recipes/deep-mediterranean-quiche.
The change here is fine so committing but we should think about the ramifications of this in #3440424: Inconsistent behaviour with drupalGet() when site is installed in a subdirectory
Committed and pushed 0bdb30910e to 11.x and 28bd49e5c3 to 10.3.x. Thanks!