#2133215: Fix IndexTidDepth views argument plugin and TaxonomyIndexTidDepth views filter plugin performance tweaked the queries for taxonomy depth filtering and now there is a random fail in \Drupal\Tests\taxonomy\Functional\Views\TaxonomyTermFilterDepthTest. See https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/42560/
Fortunately we already fixed this for the kernel test so we can copy the same logic.
Copy the logic from \Drupal\Tests\taxonomy\Kernel\Views\TaxonomyTermFilterDepthTest
Note: this only occurs on 9.3.x because 2133215 has only been committed there
None
None
None
N/a
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 3229665-5.patch | 921 bytes | dww |
| #2 | 3229665-2.patch | 913 bytes | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottComment #4
alexpottComment #5
dwwThanks for finding this, opening the issue, and fixing it!
Mostly looks good. But why keep calling
getRequestTime()inside the loop? Why not do it once outside? Like so.Comment #6
alexpott@dww because it doesn't matter - it's only going to be called 6 or so times and the expense of PHP function calls is exaggerated. Mostly when people measure them the way they measure them gets in the way of measuring anything real. Plus as pointed out in the issue summary this is a copy/paste of existing code in an analogous kernel test so are we going to update both places. Doesn't feel worth it in my opinion. A review and rtbc so someone could commit this would be more valuable because then we'd be able to prevent tests failing because of random fails.
Comment #7
spokjeLet's meet in the middle (and make both of you unhappy... 😈)
I'll RTBC patch #2 since:
_and_ I'll create a follow-up where I create a patch applying the logic of patch #5 on both Kernel- and Functional test, which somebody then can/won't RTBC and we're done :)
Comment #8
spokjeFollow up here: #3229686: Optimize logic in Kernel- and Functional-test of TaxonomyTermFilterDepthTest
Comment #9
dww@alexpott: If I had a nickel for every time someone set a patch for 'needs work' for something minor like this, I'd be rich. 😉 I thought our relentless pursuit of perfection applied to everything, including micro optimizations in tests. I also thought it was better to make such minor suggestions in the form of a patch instead of a dreditor nitpick. I was about to solicit a quick/easy RTBC for this, but @Spokje beat me to it...
But sure. $RTBC++; for #2.
Thanks @Spokje for opening the follow-up. I just downgraded it to minor. 😉
Cheers,
-Derek
Comment #10
larowlanCommitted c63b2cc and pushed to 9.3.x. Thanks!