- Issue created by @catch
- 🇧🇬Bulgaria pfrenssen Sofia
Implemented a cold/warm test. Testing a range of values for the cold test so we don't fail too quickly on minor changes.
- First commit to issue fork.
- 🇺🇸United States smustgrave
1 super nitpicky void return, added just straight to the MR.
Pipeline was appearing green
Looking at the number, to the best of my knowledge, seem like good ranges
Believe this is good coverage - 🇧🇬Bulgaria pfrenssen Sofia
Thanks! I'll do another iteration on this. I managed in another issue to get a fully stable result ( 📌 Add assertions to OpenTelemetryFrontPagePerformanceTest::testFrontPageColdCache() Active ), I will use the same tactic here.
- 🇺🇸United States smustgrave
Cool, I remember looking at one of these months ago and think maybe the ranges just needed to be expanded slightly
- 🇺🇸United States smustgrave
Came here from 📌 Add performance test coverage for menu UI Active and since it probably would be best to get this in first I'll review this first.
Cleaned up the issue summary to show a test trait is also being updated.
Left some comments on the MR.
- 🇺🇸United States smustgrave
Tons of noice but I took 1 comment back and addressed OpenTelemetryNodePagePerformanceTest replacement.
- 🇬🇧United Kingdom catch
Ran the test locally and got:
1) Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAdminContentPerformanceTest::testAdminContentWarmCache Failed asserting that 234 matches expected 235.We might need a range there too?
Would be nice to reduce the ranges on the cold cache method if we're able to make that more reliable, but that could happen in a follow-up.
The class name does not need to have OpenTelemetry on it - that only applies to tests that send tracing data to open telemetry, which this one doesn't. Just AdminContentPerformanceTest seems fine.