OpenTelemetryNodePagePerformanceTest::testNodePageHotCache() is not hot enough

Pushed a branch with the assertions that should make sense, but fail on gitlab ci - stack on 📌 Allow needs_destruction services to run on page cache hits Needs review which is necessary for the test to record anything at all.

Heisenbug - 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 📌 Allow needs_destruction services to run on page cache hits Needs review and would fix the open telemetry dashboard.

Now that 📌 Allow needs_destruction services to run on page cache hits Needs review is in, trying the basic version of this again.

OK that fixed it, looks like the actual issue here was requesting /node/1 instead of node/1 in consistently. Checked the other performance tests and it makes no difference to those, just the page cache test here - but went ahead and updated so everything's consistent.

Would be good to add assertions to all the other test methods too, but we've got 📌 Add database and cache assertions to OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest Active open for that already.

I 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.

Yes 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, but standardising one way or the other for Drupal paths would be good.

Opened #3440284: Decide on standard for drupalGet forward slash vs no slash →

This 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

      if (strlen($path) > 1) {
        $path = ltrim($path, '/');
      }

after we strip the base path instead of the changes here.

I'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.

Nice 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.

Nope, 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 🐛 Inconsistent behaviour with drupalGet() when site is installed in a subdirectory Active .

So 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 🐛 Inconsistent behaviour with drupalGet() when site is installed in a subdirectory Active

Committed and pushed 0bdb30910e to 11.x and 28bd49e5c3 to 10.3.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.