fix: benchmark comparison #658
Conversation
was comparing self with self
WalkthroughThe pull request modifies the benchmarking process in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/performance/justfile (1)
22-22: The fix correctly addresses the self-comparison issue.The implementation now properly runs the benchmark in the cloned main branch directory, fixing the issue where the system was comparing itself with itself.
However, the relative path navigation (
../../../) could be error-prone. Consider using absolute paths for better reliability.Example improvement:
- cd {{tmpdir}}/test/performance && just run {{bench}} {{p}} {{benchtime}} {{count}} '../../../report/benchmark-output-main.txt' + cd {{tmpdir}}/test/performance && just run {{bench}} {{p}} {{benchtime}} {{count}} "$(pwd)/../../../report/benchmark-output-main.txt"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/performance/justfile(1 hunks)
🔇 Additional comments (2)
test/performance/justfile (2)
20-20: LGTM! Good practice to clean up before benchmarking.Removing the old benchmark output file ensures clean comparison results.
20-22: Verify the benchmark comparison results.The implementation looks correct, but let's verify that the benchmark comparison is now working as expected.
Run this script to check the benchmark output paths and content:
✅ Verification successful
Implementation verified as correct
The benchmark comparison implementation is properly structured:
- Creates report directory on-demand
- Maintains separate output files for local and main branch
- Properly handles relative paths and cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify benchmark output paths and content to ensure proper comparison # Check if the output paths exist and are different echo "Checking benchmark output paths..." ls -l ./report/benchmark-output-*.txt # Verify the content differs between local and main outputs echo "Comparing output content..." if [ -f "./report/benchmark-output-local.txt" ] && [ -f "./report/benchmark-output-main.txt" ]; then diff <(head -n 5 ./report/benchmark-output-local.txt) <(head -n 5 ./report/benchmark-output-main.txt) fiLength of output: 313
Script:
#!/bin/bash # Check directory structure and justfile implementation echo "Checking directory structure..." ls -la echo -e "\nSearching for report directory..." find . -type d -name "report" echo -e "\nSearching for benchmark output files..." find . -type f -name "benchmark-output*" echo -e "\nChecking justfile content..." cat test/performance/justfileLength of output: 3557
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #658 +/- ##
==========================================
- Coverage 81.62% 81.58% -0.05%
==========================================
Files 131 131
Lines 7059 7059
==========================================
- Hits 5762 5759 -3
- Misses 994 996 +2
- Partials 303 304 +1 ☔ View full report in Codecov by Sentry. |
was comparing self with self