-
Notifications
You must be signed in to change notification settings - Fork 834
Block Delimiter: Harden performance test to reduce flakiness #44896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Delimiter: Harden performance test to reduce flakiness #44896
Conversation
- Centralize tunables in class constants for maintainability - Use competitive benchmark with paired measurements and warmups - Implement trimmed median aggregation for both time and memory - Relax assertions (1.15x → 1.10x ratio, 64KB memory tolerance) - Add retry mechanism for borderline results with graceful fallback - Fully qualify global functions to resolve linter errors - Preserve early-exit advantage with larger test fixture (2050 blocks) - Add always-on correctness test without performance assertions
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
- Rename constants for better readability and understanding of their purpose - Update references in the benchmark tests to match new constant names - Ensure performance assertions are aligned with the new naming conventions
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
- Reduced warmup runs, benchmark iterations, and retry iterations for faster CI execution. - Updated performance assertion thresholds to be more CI-friendly, ensuring no significant regressions. - Added checks for system load to skip tests under high load conditions, enhancing reliability. - Adjusted test content generation to maintain early-exit advantage while improving execution speed.
- Adjusted performance assertion thresholds to ensure the scanner does not become significantly slower. - Removed redundant comments and streamlined the test logic for clarity. - Enhanced CI-friendliness by simplifying performance checks and maintaining consistent assertions.
- Changed assertion from assertGreaterThan to assertGreaterThanOrEqual to enhance the precision of performance tests. - This adjustment aims to ensure that the scanner's performance meets the defined minimum time ratio threshold without being overly strict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests pass - nice! From my limited tests knowledge, code makes sense as well 😅
I would suggest that anything benchmark related (that's actually code that helps with the benchmark, not actual tests) be moved in a separate folder (like lib
or something).
That way the Block_Scanner_Test will only have tests in it.
Can you attach this to an Linear issue? With the conversation on pc9hqz-3Fn-p2 , it does lead me to question if we should tweak this approach or do something else with it. This PR is fine, once @dilirity's comments are resolved, and I'm curious if we should think about a larger effort to have better benchmark and long-term verification against benchmarks than this test. |
- Introduced Performance_Benchmark_Utils class to centralize benchmarking logic and improve test reliability. - Moved performance-related constants and methods from Block_Scanner_Test to the new utility class. - Updated Block_Scanner_Test to utilize the new utility methods for performance assertions and content generation. - Enhanced test structure for clarity and maintainability, ensuring consistent performance evaluation across tests.
Good point, I've updated the tests to now have a utility class.
Ah I didn't add the Linear issue within the description before. It is now.
I agree also 100%. I have created this ticket to investigate a performance benchmarking strategy in this general domain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes HOG-307: Harden Block_Scanner performance test to further remove flakiness
Proposed changes:
Other information:
Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Verify the test passes and demonstrates Scanner performance advantage