Add performance test coverage to the term listing admin page

Starting to try this one.

I have only added a hot/cold cases, no cool nor warm.

I'm not sure if that would apply.

Also, the listing is using Drupal\Core\Entity\EntityListBuilder, so it may be worth to generalize, or maybe that is more than we want to do now.

Feedback is highly welcomed.

I think this is now ready to be reviewed again.
It has been a nice experience to get to know a bit more how performance tests are structured.

I have changed the setup phase to remove a views block at the start, that shows taxonmy terms on every pages, to have less noise.

I have also added a general cache bin remove method, to the main related trait for these tests.
It sounds like a great candidate to have there, since many tests will use it.

I also added a cool cache case, since it may also be relevant for performance tracking purposes.

I have not added a warm cache case, since I could not really think about an analogous set of steps for this case.
E.g. on node case it makes sense because there are sibling nodes, but for the term listing case there is not such a case.

Commits since last update on the 3476439-term-list-performance-test branch, and related MR !9595

9ad0e4e8f9 Actually look for things at the right place 😅
6bc0ea4a31 Make linter happy
709d5b6f2d Do not use the umami block with tags vocabulary terms as a block in the footer
19f8eb2d6e Unify the approach to match the term
a357447a30 Add cache bins clear to PerformanceTestTrait
2aef88e121 Add a cool cache case for vocabulary admin performance test

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

I'm wondering what needs-review-queue-bot is thinking :-)
I see pipelines in green: https://git.drupalcode.org/project/drupal/-/merge_requests/9595/pipelines

Back to NR.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

vinai_katiyar → changed the visibility of the branch 3476439-term-list-performance-test to hidden.

I will check for pipeline failure.

shalini_jha → changed the visibility of the branch 3476439-term-list-performance-test to active.

I've rebase the MR to address the pipeline failure,so pipeline is fixed now. and as noted in #6, the changes are now ready for review. Moving this to 'Needs Review.

Saving credit for marvil07 and pfrenssen for getting MR 99% there minus the void return.

Cleaned up the issue summary just slightly

The numbers in the test make sense I believe, but pipeline is good so believe this one to be good.

This test added here failed locally so I restarted the test on gitlab and it failed there as well. https://git.drupalcode.org/issue/drupal-3476439/-/jobs/3610653

I re-ran the failed job, and it passed. It seems to be a random test failure. I am verifying this locally to confirm.

I have run this locally and this failing same as mentioned #17, i am working on it.

I’ve updated the test to expect a cache get count of 115 instead of 116. After debugging, I found that getCacheGetCount returned 115, which is different from the expected 116. Since I’m new to this type numbers count test, I’m not sure about the cause of the discrepancy, so I’ve adjusted the test based on what I observed and run the test locally and it is passed. I would appreciate any guidance on whether this change is correct, or if further adjustments are needed.
Moving this to NR, Kindly review.

Believe the 115 should be fine.

Saving credit for shalini_jha for the continued work on it and debugging.

Needs to be rebased

Re base & fixed conflicts. moving to NR.

Rebase seems fine

This should use ::assertMetrics() now, see 📌 Add PerformanceTestTrait::assertMetrics() so it is easier to write performance tests Active .

Thank you for the feedback and reference. I've updated the code to use ::assertMetrics() as suggested and applied the same change to VocabularyAdminPerformanceTest. I also re-ran the test, and the query count is now updated to 12, so I have updated the test accordingly.
Moving this to Need review , Kindly review.

left a comment on MR

I am a bit unsure about the last feedback. Could you please clarify if we need to remove this ?
moving this for NR to get the feedback.

What I mean is adding clearCaches() to the testTrait. There are other current performance tickets that could benefit from such a change. So think a follow up should be opened to update the testTrait and all current performance tests to use it.

@smustgrave Thank you so much for your detailed explanation!

The clearCaches() method needs to be removed from the TestTrait. Yes, I found several performance test classes that could benefit from this change, so you are absolutely right—we should create a follow-up ticket to update all performance test methods accordingly.

For now, I have added the clearCaches() method within the same test, following the pattern used in other performance tests.Pipeline is also green , so moving this back to review.

Perfect. Also please open the follow up to update all with trait update please

Added follow up ticket for this : https://www.drupal.org/project/drupal/issues/3504566 📌 Move clearCaches() Method to PerformanceTestTrait for Reusability Across All Performance Tests Active

Couple of comments on the MR.